Skip to content

Comments

Play silence to keep audio device awake#16099

Merged
seanbudd merged 29 commits intonvaccess:masterfrom
mltony:silence2
Feb 8, 2024
Merged

Play silence to keep audio device awake#16099
seanbudd merged 29 commits intonvaccess:masterfrom
mltony:silence2

Conversation

@mltony
Copy link
Contributor

@mltony mltony commented Jan 27, 2024

Link to issue number:

Fixes #14386

Summary of the issue:

Play silence in order to keep audio device open

Description of user facing changes

By default most users won't notice anything as default volume is 0 (playing silence).
I added two config options in the advanced panel: silence duration (default = 30 seconds) and white noise volume (default is set to 0). Playing white noise would be helpfulin order to debug any user issues related to audio dropout.

Description of development approach

Most of heavylifting has been done by @jcsteh.
In wasapi.cpp there has been added class SilencePlayer that spawns a new thread, that just plays silence/white noise when requested. Requests are coming from python code in nvwave.py from WasapiWavePlayer.feed() function.

Testing strategy:

Tested with white noise that it is being played as expected. Tested that config changes reflect immediately.
Couldn't test with silence as apparently my bluetooth headphones are not suffering from dropout problem in the first place.

Known issues with pull request:

N/A

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@AppVeyorBot
Copy link

See test results for failed build of commit 9a8fbb01d5

@cary-rowen
Copy link
Contributor

I'll test this after you fix the CI build fail

Copy link
Contributor

Choose a reason for hiding this comment

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

Please re-initialize nvwave when user restores config to saved / defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to reinitialize since it reloads current settings for duration and volume automatically.

@CyrilleB79
Copy link
Contributor

To me, these options seem too specific / technical for most of the users, including some users impacted by bluetooth device audio cut-off. Not every user knows what a white noise is, and from a user point of view, playing silence or with noise is not very meaningful.

Playing white noise would be helpfulin order to debug any user issues related to audio dropout.

First of all, if white noise is only for debug purpose, it should not be in audio settings IMO, maybe just in advanced settings, or better, in an add-on.
If instead white noise is required by some device or sound cards, I think that we should just enforce white noise for everybody. In any case this white noise should not be audible, so it should not make a difference with full silence.

Personally, I would see just one option "Duration of audio inactivity (in seconds) before closing the audio stream". Or even simpler: a checkbox "Keep the audio device/stream always open". Then the in the User Guide, give information to the user to help setting this value, i.e.:

  • too low = audio may be cut-off more often with a Bluetooth device
  • too high = the battery discharges faster.

mltony and others added 2 commits January 27, 2024 12:46
Co-authored-by: Łukasz Golonka <lukasz.golonka@mailbox.org>
@mltony
Copy link
Contributor Author

mltony commented Jan 27, 2024

@cary-rowen,
Feel free to test this now. installer NVDA test ids failing for everyone.

@CyrilleB79

these options seem too specific / technical for most of the users
it should not be in audio settings

I already put both settings in advanced panel. Most users should be happy with default values.

Some users complained to Jamie that this patch didn't fix their audio issue. We can either try to guess what happened to those users. Or alternatively we can ask them to switch to white noise and this will give us more information, because:

  1. If white noise is not audible - something is really broken.
  2. If white noise is audible but from another audio output device, then there is a problem with synchronization.
  3. If white noise is audible from correct audio output, then something is wrong with auido
    If you take away white noise option, then there won't be a good way to debug those issues.

• too low = audio may be cut-off more often with a Bluetooth device
• too high = the battery discharges faster.

Updated doc.

@CyrilleB79
Copy link
Contributor

Sorry, @mltony, I had overlooked your PR and have not checked where the options were located.

@cary-rowen
Copy link
Contributor

Hi @mltony
I tested this and it worked with my bluetooth headphones, and in the past I needed to use the bluetooth audio add-on.

Thanks

@beqabeqa473
Copy link
Contributor

beqabeqa473 commented Jan 28, 2024 via email

@mltony
Copy link
Contributor Author

mltony commented Jan 28, 2024

@beqabeqa473

Silence duration and white noise volume are both configurable.

most of the wireless headsets have a possibility to turn them all upon inactivity time.

Not sure I understand this comment. Most Bluetooth headphones enter standby mode soon after each utterance which causes speech dropout. As far as I know there is typically no way to configure standby mode on vast majority of Bluetooth headphones.

@beqabeqa473
Copy link
Contributor

beqabeqa473 commented Jan 28, 2024 via email

@seanbudd
Copy link
Member

I believe the white noise parameter should not be settable from the GUI, as its for dev debugging only, but keeping it has a hidden setting in nvda.ini may be appropriate

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jan 29, 2024
@mltony
Copy link
Contributor Author

mltony commented Jan 30, 2024

@seanbudd, I just dropped white noise volume from settings panel.

@mltony mltony marked this pull request as ready for review January 30, 2024 22:47
@mltony mltony requested review from a team as code owners January 30, 2024 22:47
@cary-rowen
Copy link
Contributor

If the white noise volume is only used for development and debugging, it should be recorded in an appropriate doc. Otherwise, even developers will not know how to use it because it is too hidden.

@cary-rowen
Copy link
Contributor

If I remember correctly, reporting progress also has a hidden interval that can be configured.
This information should be documented, such as in the "Advanced Topics" section of the user guide.

Co-authored-by: Cyrille Bougot <cyrille.bougot2@laposte.net>
@mltony
Copy link
Contributor Author

mltony commented Feb 7, 2024

Renamed to "keep audio device awake" in UX as suggested. In user guide I renamed the section as well, but I left explanation that this is achieved by playing silence. Hope that's ok.

@mltony mltony marked this pull request as ready for review February 7, 2024 19:32
@Adriani90
Copy link
Collaborator

@mltony and @jcsteh thanks very much for this nice work guys.

in the user guide, I think it makes sense to replace the word utterance with "speech sequence". This is easier to understand especially for people who need easier language. @mltony what do you think?
Also I would mention that this does not affect only bluetooth devices positively, but also systems with lower performance, and virtual machines such as virtual desktops in Citrix environments.

@mltony
Copy link
Contributor Author

mltony commented Feb 7, 2024

@Adriani90, updated utterance -> speech sequence.
Re other use cases, what evidence do we have that silence positively affects systems with lower performance? My wife is running NVDA on a modest 8-year old laptop and she doesn't experience any problems with NVDA audio. I guess there must be users who are running NVDA on even older computers. But then I'd think just being low performance wouldn't necessarily imply bad audio quality - NVDA in general doesn't consume much resources at all. Maybe there is some old technology like a specific brand of motherboards that suffers from this problem? I don't know whether we should go into that rabbithole of investigating which old equipment causes dropout in NVDA speech, but I would prefer to have some evidence (like user testimonial) that silence indeed helps on their old equipment.
Same thing for citrix. I previously ran NVDA in VMWare Player and VirtualBox and I recall audio quality wasn't that perfect, but it didn't improve from Bluetooth Audio add-on. And the quality problems wer different: every few seconds there was some audible glitch in sound instead of beginnings of each utterance dropping, which would have been a sign of audio device being closed. My guess was that there was high latency between host and guest that caused occasional dropout of some small chunk of audio buffer. And in my understanding this own't be fixed by this PR.

@Adriani90
Copy link
Collaborator

Adriani90 commented Feb 7, 2024 via email

@mltony
Copy link
Contributor Author

mltony commented Feb 8, 2024

@Adriani90 , fair enough, I added a line about that in the user guide. Now I also vaguely remember someone on NVDA mailing list was saying their laptop's built-in audio was suffering from audio cutoffs and Bluetooth add-on indeed helped, so I mentioned that as well.

Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

All reads well, good work.

@seanbudd seanbudd changed the title Playing silence or white noise Play silence to keep audio device awake Feb 8, 2024
@seanbudd seanbudd merged commit 3999407 into nvaccess:master Feb 8, 2024
@nvaccessAuto nvaccessAuto added this to the 2024.2 milestone Feb 8, 2024
@jcsteh
Copy link
Contributor

jcsteh commented Feb 8, 2024

@mltony Thanks very much for driving this to completion. :)

@XLTechie
Copy link
Collaborator

XLTechie commented Feb 8, 2024 via email

CyrilleB79 added a commit to CyrilleB79/nvda that referenced this pull request Feb 8, 2024
Nael-Sayegh pushed a commit to Nael-Sayegh/nvda that referenced this pull request Feb 15, 2024
Fixes nvaccess#14386

Summary of the issue:
Play silence in order to keep audio device open

Description of user facing changes
By default most users won't notice anything as default volume is 0 (playing silence).
I added two config options in the advanced panel: silence duration (default = 30 seconds) and white noise volume (default is set to 0). Playing white noise would be helpfulin order to debug any user issues related to audio dropout.

Description of development approach
Most of heavylifting has been done by @jcsteh.
In wasapi.cpp there has been added class SilencePlayer that spawns a new thread, that just plays silence/white noise when requested. Requests are coming from python code in nvwave.py from WasapiWavePlayer.feed() function.
seanbudd pushed a commit that referenced this pull request Feb 21, 2024
Fix-up of #16099

Fixes #16148

Summary of the issue:
Error when disabling WASAPI and reopening advanced settings:
ERROR - unhandled exception (11:14:54.349) - MainThread (6028):
Traceback (most recent call last):
  File "gui\settingsDialogs.pyc", line 4517, in onCategoryChange
  File "gui\settingsDialogs.pyc", line 693, in onCategoryChange
  File "gui\settingsDialogs.pyc", line 675, in _doCategoryChange
  File "gui\settingsDialogs.pyc", line 603, in _getCategoryPanel
  File "gui\settingsDialogs.pyc", line 362, in __init__
  File "gui\settingsDialogs.pyc", line 372, in _buildGui
  File "gui\settingsDialogs.pyc", line 3514, in makeSettings
  File "gui\settingsDialogs.pyc", line 3259, in __init__
  File "gui\settingsDialogs.pyc", line 3352, in _onWasapiChange
  File "gui\guiHelper.pyc", line 232, in Enable
TypeError: Window.Enable(): argument 1 has unexpected type 'FeatureFlag'
ERROR - unhandled exception (11:14:54.361) - MainThread (6028):
Traceback (most recent call last):
  File "gui\guiHelper.pyc", line 218, in _onEnableChanged
TypeError: Window.Enable(): argument 1 has unexpected type 'FeatureFlag'
Oversight in AdvancedPanelControls.haveConfigDefaultsBeenRestored in settingsDialogs.py.

In settingsDialogs.py, rename variable to be consistent with the GUI ("silence" -> "keep audio awake")

Documentation (User Guide and Change log) updates

Description of user facing changes
The option is enabled or disabled depending on the real current WASAPI state, not the state defined in the configuration for the next restart of NVDA.
The "keep audio awake" option is moved to audio settings
The documentation does not mention "playing silence" at all anymore
Description of development approach
For point 1: use real WASAPI state instead of what is in the config for the next restart, as it was already done in other audio settings depending on WASAPI.
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
Fixes nvaccess#14386

Summary of the issue:
Play silence in order to keep audio device open

Description of user facing changes
By default most users won't notice anything as default volume is 0 (playing silence).
I added two config options in the advanced panel: silence duration (default = 30 seconds) and white noise volume (default is set to 0). Playing white noise would be helpfulin order to debug any user issues related to audio dropout.

Description of development approach
Most of heavylifting has been done by @jcsteh.
In wasapi.cpp there has been added class SilencePlayer that spawns a new thread, that just plays silence/white noise when requested. Requests are coming from python code in nvwave.py from WasapiWavePlayer.feed() function.
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
Fix-up of nvaccess#16099

Fixes nvaccess#16148

Summary of the issue:
Error when disabling WASAPI and reopening advanced settings:
ERROR - unhandled exception (11:14:54.349) - MainThread (6028):
Traceback (most recent call last):
  File "gui\settingsDialogs.pyc", line 4517, in onCategoryChange
  File "gui\settingsDialogs.pyc", line 693, in onCategoryChange
  File "gui\settingsDialogs.pyc", line 675, in _doCategoryChange
  File "gui\settingsDialogs.pyc", line 603, in _getCategoryPanel
  File "gui\settingsDialogs.pyc", line 362, in __init__
  File "gui\settingsDialogs.pyc", line 372, in _buildGui
  File "gui\settingsDialogs.pyc", line 3514, in makeSettings
  File "gui\settingsDialogs.pyc", line 3259, in __init__
  File "gui\settingsDialogs.pyc", line 3352, in _onWasapiChange
  File "gui\guiHelper.pyc", line 232, in Enable
TypeError: Window.Enable(): argument 1 has unexpected type 'FeatureFlag'
ERROR - unhandled exception (11:14:54.361) - MainThread (6028):
Traceback (most recent call last):
  File "gui\guiHelper.pyc", line 218, in _onEnableChanged
TypeError: Window.Enable(): argument 1 has unexpected type 'FeatureFlag'
Oversight in AdvancedPanelControls.haveConfigDefaultsBeenRestored in settingsDialogs.py.

In settingsDialogs.py, rename variable to be consistent with the GUI ("silence" -> "keep audio awake")

Documentation (User Guide and Change log) updates

Description of user facing changes
The option is enabled or disabled depending on the real current WASAPI state, not the state defined in the configuration for the next restart of NVDA.
The "keep audio awake" option is moved to audio settings
The documentation does not mention "playing silence" at all anymore
Description of development approach
For point 1: use real WASAPI state instead of what is in the config for the next restart, as it was already done in other audio settings depending on WASAPI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Play silence to prevent missing information at start of speech