Skip to content

nvwave: make idle close delay configurable#11481

Closed
codeofdusk wants to merge 1 commit into
nvaccess:masterfrom
codeofdusk:configurable-nvwave-timer
Closed

nvwave: make idle close delay configurable#11481
codeofdusk wants to merge 1 commit into
nvaccess:masterfrom
codeofdusk:configurable-nvwave-timer

Conversation

@codeofdusk

@codeofdusk codeofdusk commented Aug 10, 2020

Copy link
Copy Markdown
Contributor

Link to issue number:

#11024 (comment)

Summary of the issue:

In #11024, some users asked for the ability to increase the default timeout of 10 seconds for holding open the audio device. This could improve performance on systems with newer Realtek audio chipsets at the cost of some battery power. Others asked to decrease or disable the delay, as their systems do not exhibit this issue and there is concern that holding open the audio device in this way might use more power.

Description of how this pull request fixes the issue:

Adds a new advanced setting for configuring the idle close delay, and replaces the IDLE_CLOSE_DELAY constant in nvwave with references to this new setting.

Testing performed:

Added a beep in nvwave._close and observed that it played according to user-configured values of the idleCloseDelay setting.

Known issues with pull request:

None.

Change log entry:

Add a second sub-point to the #11024 entry:

  • This timeout is 10 seconds by default, but it is configurable in advanced settings.

@codeofdusk

Copy link
Copy Markdown
Contributor Author

@Adriani90

Copy link
Copy Markdown
Collaborator

This needs also documentatiom in the user guide, including a statement that chamging it can shorten the batery time.

# Audio settings
[audio]
audioDuckingMode = integer(default=0)
idleCloseDelay = integer(default=10000,min=0,max=86400000)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, a day is a bit long isn't it? May be leave out the max, or set it somewhere around an hour.

self.idleCloseDelaySpinControl = audioGroup.addLabeledControl(
label,
nvdaControls.SelectOnFocusSpinCtrl,
min=0,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is logic to get this from config. Search for usage of config.conf.getConfigValidation

Comment thread source/nvwave.py
self._closeTimer.Start, self.IDLE_CLOSE_DELAY_MS,
wx.TIMER_ONE_SHOT
)
if config.conf["audio"]["idleCloseDelay"]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be made one if statement with the closeWhenIdle check

==== Caret move timeout (in MS) ====[AdvancedSettingsCaretMoveTimeout]
==== Idle close delay (in MS) ====[AdvancedSettingsIdleCloseDelay]
Some audio drivers, notably for newer Realtek sound cards, take a long time to open the audio device causing the start or end of speech to be truncated.
By default, NVDA keeps the device open for 10,000 ms (10 seconds) after a speech utterence to improve performance on these systems.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SHouldn't Utterence be utterance?

Suggested change
By default, NVDA keeps the device open for 10,000 ms (10 seconds) after a speech utterence to improve performance on these systems.
By default, NVDA keeps the device open for 10,000 ms (10 seconds) after a speech utterance to improve performance on these systems.

By default, NVDA keeps the device open for 10,000 ms (10 seconds) after a speech utterence to improve performance on these systems.
However, it may be desirable to increase this timeout, to possibly further improve performance at the cost of battery power.
It might also be desirable in some cases to decrease this timeout for power savings on systems without this issue.
Setting the delay to 0 causes NVDA to close the device immediately after each utterence, matching the behaviour of version 2020.2 and earlier.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Setting the delay to 0 causes NVDA to close the device immediately after each utterence, matching the behaviour of version 2020.2 and earlier.
Setting the delay to 0 causes NVDA to close the device immediately after each utterance, matching the behaviour of version 2020.2 and earlier.

@Brian1Gaff

Brian1Gaff commented Aug 11, 2020 via email

Copy link
Copy Markdown

@feerrenrut

Copy link
Copy Markdown
Contributor

Introducing configuration here increases the maintenance and support costs of NVDA. It's another way that end users can introduce variability into their installation. Instead we should aim to get the default value right in this case.

It only seems to be an assumption that releasing the audio device will save power. I suspect what the operating system does with physical devices (for power saving) is quite separate from what NVDA does here (calling winmm.waveOutOpen). Even if NVDA didn't call winmm.waveOutClose until termination, I wouldn't be surprised if the operating system is able to optimize power usage significantly. In addition, I suspect that the act of industrializing libraries, allocating memory etc while calling open/close regularly uses more power than just doing it once. In the absence of data we are just guessing, and possibly doing more harm than good.

Without supporting evidence to show that this does indeed save power, or solve some other problem, I propose that this PR be closed.

Typically an issue should be created so that we can have this discussion before the work is completed.

@feerrenrut feerrenrut closed this Aug 11, 2020
@Neurrone

Copy link
Copy Markdown

I don't think we're going to be able to have a default that will suit everyone due to the nature of the problem.

10 seconds is far from optimal, because it is easy to encounter a situation where no speech is spoken for more than 10 seconds, then have the first part of speech being cut off the next time that NVDA speaks.

If we want to choose a default that will definitely work, it should be set to a much longer value, especially if there are doubts about whether leaving the audio device on consumes more power.

Another point to consider is that for bluetooth headphones, leaving it on does cause its battery to drain faster.

@Neurrone

Copy link
Copy Markdown

The ideal solution would to get Microsoft to either fix this themselves if they can, or put their foot down regarding this behaviour and make it a hard failure when certifying audio drivers for example. Sadly I don't ever see this happening, not sure who would even be the right person to contact.

This is a plague and screen reader users are far from the only users who're affected.

@jcsteh

jcsteh commented Aug 12, 2020 via email

Copy link
Copy Markdown
Contributor

@feerrenrut

Copy link
Copy Markdown
Contributor

While that does fix one class of problem, I make
no guarantees that it fixes the Bluetooth headset problem, since I don't
have such a device to test.

I can confirm, this change does NOT solve the problem you describe with my BT headset.

@jcsteh

jcsteh commented Aug 13, 2020 via email

Copy link
Copy Markdown
Contributor

@codeofdusk

Copy link
Copy Markdown
Contributor Author

We could probably change the current implementation to play silence instead
of just keeping the device open.

Sounds good to me, but in that case I strongly vote to reopen this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants