Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add setting to have the volume of NVDA sounds follow the voice volume. #14896

Merged
merged 7 commits into from
Jun 14, 2023

Conversation

jcsteh
Copy link
Contributor

@jcsteh jcsteh commented May 2, 2023

Link to issue number:

Fixes #1409.

Summary of the issue:

Users would like to be able to control the volume of NVDA sounds. With the WASAPI implementation in #14697, it is possible to separately control the volume of NVDA sounds using the Windows Volume Mixer. However, it is often useful to link the volume of NVDA sounds to the volume of the voice.

Description of user facing changes

It is now possible to have the volume of NVDA sounds and beeps follow the volume setting of the voice you are using. This option can be enabled in Advanced settings.

Description of development approach

When this option is enabled and a WasapiWavePlayer is using soundsSession, the open() and stop() methods of WasapiWavePlayer set the session volume to the synth's voice volume. Note that we do this in stop() because some players keep the device open; e.g. tones.

Testing strategy:

  1. Enabled the new setting.
  2. Decreased the voice volume.
  3. Switched between browse and focus modes. Observed that the sound volume was lower.
  4. Moved the mouse with mouse beeps enabled. Observed that the beep volume was lower.
  5. Increased the voice volume to maximum.
  6. Switched between browse and focus modes. Observed that the sound volume was maximum.
  7. Moved the mouse with mouse beeps enabled. Observed that the beep volume was maximum.
  8. Disabled the new setting.
  9. Decreased the voice volume.
  10. Switched between browse and focus modes. Observed that the sound volume was still maximum.
  11. Moved the mouse with mouse beeps enabled. Observed that the beep volume was still maximum.
  12. Enabled the new setting.
  13. Switched between browse and focus modes. Observed that the sound volume was lower.
  14. Disabled the new setting.
  15. Switched between browse and focus modes. Observed that the sound volume was maximum.
  16. Enabled the new setting.
  17. Disconnected the default audio device (a headset).
  18. Moved the mouse with mouse beeps enabled. Observed that the beep volume was lower.

Known issues with pull request:

  1. This is in Advanced settings because it depends on WASAPI and WASAPI is in Advanced settings. Once WASAPI is the only option, we can move this to Voice settings. Or should we just move it now?
  2. The volume of a sound isn't adjusted while it is playing if the voice volume is changed. NVDA sounds usually aren't (and shouldn't be) very long, so I don't think this is a real issue, but I thought it worth noting anyway.

Change log entries:

New features

It is now possible to have the volume of NVDA sounds and beeps follow the volume setting of the voice you are using. This option can be enabled in Advanced settings. (#1409)

Note that I forgot to mention that it's possible to separately control the volume of NVDA sounds in the What's New for #14697. We could add that too:

You can now separately control the volume of NVDA sounds. This can be done using the Windows Volume Mixer. (#1409)

Code Review Checklist:

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

@jcsteh jcsteh requested review from a team as code owners May 2, 2023 12:03
@jcsteh jcsteh requested review from Qchristensen and seanbudd May 2, 2023 12:03
@XLTechie
Copy link
Collaborator

XLTechie commented May 2, 2023 via email

user_docs/en/userGuide.t2t Outdated Show resolved Hide resolved
@jcsteh
Copy link
Contributor Author

jcsteh commented May 3, 2023

One other test case that should be here, I think: decrease the volume with the
setting enabled, and confirm sounds are low. Then disable the setting and
confirm that the voice is still low, but the sounds are high.

I intentionally didn't include this test case, as I know it won't work. Whether it should is an interesting question. Strictly speaking, disabling the setting should prevent the sound volume from following the voice volume, but it doesn't say anything about snapping the volume back to maximum. The user might have had the sound volume set to something lower before they enabled follow. However, it also doesn't really make sense to me to keep track of the level the user set before enabling follow just so we can restore it later.

@jcsteh
Copy link
Contributor Author

jcsteh commented May 3, 2023

If we did want to snap back to maximum, we'd need to do that when applying the disable change somehow.

@XLTechie
Copy link
Collaborator

XLTechie commented May 3, 2023 via email

@jcsteh
Copy link
Contributor Author

jcsteh commented May 3, 2023

Yeah, I'm slightly less bothered by storing it in the session, but the UX isn't great, since then the behaviour is inconsistent depending on whether the user enabled and disabled in the same session or not.

The other challenge is how to apply the volume change when disabling. One way is to create a temporary WavePlayer using the sounds session just to adjust the session volume. We'd need to do this when applying in the dialog, when switching config profiles and when reverting the config. Perhaps a simpler way I'm considering is just to track the last state in the WavePlayer class.

@seanbudd seanbudd marked this pull request as draft June 5, 2023 03:56
@jcsteh jcsteh marked this pull request as ready for review June 10, 2023 04:20
@AppVeyorBot
Copy link

See test results for failed build of commit 2bcbc8b972

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Looks almost ready, thanks @jcsteh

source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
source/gui/settingsDialogs.py Show resolved Hide resolved
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks @jcsteh

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.

Docs changes read well, good work!

@seanbudd seanbudd merged commit e6dce42 into nvaccess:master Jun 14, 2023
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Jun 14, 2023
@brunoprietog
Copy link

Hi, I am trying this, but I feel that the volume of the tones does not go down linearly. With the option disabled, if I lower the volume of NVDA Sounds and NVDA to the same level using the Windows sound mixer, the sound is heard at a similar intensity. On the other hand, if I enable the option and lower the volume of the NVDA voice, the sounds remain louder than the voice at all times, except when the voice volume is 100.

@jcsteh
Copy link
Contributor Author

jcsteh commented Jun 15, 2023

What voice are you using? This is going to depend on the voice somewhat, since the voice volume is controlled by the voice, not NVDA or Windows. If the voice volume isn't entirely linear, then there is going to be a mismatch. It seems to be fairly linear with eSpeak.

@brunoprietog
Copy link

Yes, I think that is the cause. I just tried Espeak and it works fine. Is there a way to make the NVDA sound unified? That is, if I lower the volume of NVDA in the Windows sound mixer, it lowers the volume of the voice and the volume of the tones equally, just as it works when you don't have Wasapi enabled. Thanks!

@jcsteh
Copy link
Contributor Author

jcsteh commented Jun 17, 2023

There isn't a way to do that, no. They are separate sessions so that users can adjust them separately, but the flip side of that is that they are... well... adjustable separately.

@brunoprietog
Copy link

Sure, I understand. But isn't there a way I can be combined in one session? Just like it works in previous versions. That allows you to reduce the volume of NVDA from the Windows sound mixer, without having to worry about lowering the volume of NVDA and NVDA sounds. I understand that's the point of this PR, but as I was telling you, it doesn't work well with other synthesizers. Thanks!

@jcsteh
Copy link
Contributor Author

jcsteh commented Jun 19, 2023

The point of this PR was that users wanted a way to do this which used the voice volume, since the voice volume is easier to control than the Windows sound mixer volume; e.g. you can use the synth settings ring. I don't really see a way we can support all of these different things at once when some of them are in conflict. I guess a setting might be possible, but these settings would have to be mutually exclusive, which is confusing at best.

What synth are you using? I wonder whether the correct solution is to fix the synth driver to set the voice volume in a more useful way.

@brunoprietog
Copy link

Shure. I'm using the IBM TTS synthesizer.

@jcsteh
Copy link
Contributor Author

jcsteh commented Jun 19, 2023

Okay. As a third party synth, we have no control over that.

@brunoprietog
Copy link

Exactly, I completely understand. That's why I was asking about a possibility to have the tones and the voice in the same session, so as not to have this kind of problems. Instead of having each session separately

@jcsteh
Copy link
Contributor Author

jcsteh commented Jun 19, 2023

Which takes us back to people not being able to control them separately.

seanbudd pushed a commit that referenced this pull request Jun 22, 2023
Better solution for #1409. Addresses #14896 (comment). Also addresses feedback I received on Mastodon regarding the NVDA sounds volume not appearing in the Windows Volume Mixer in some versions of Windows.

Summary of the issue:
In Support for audio output using WASAPI #14697, I split NVDA speech and sounds into two separate Windows audio sessions to allow independent control of their levels. However, some users want these levels to be linked.
In Add setting to have the volume of NVDA sounds follow the voice volume. #14896, I made it possible to have the sounds volume follow the voice volume. However, as per Add setting to have the volume of NVDA sounds follow the voice volume. #14896 (comment), some synths don't set the volume in a linear way, so the volumes don't match.
It was reported to me on Mastodon that in Windows 11 Insider builds, the "NVDA sounds" volume doesn't show up in the Windows Volume Mixer unless you actively play a sound while the Volume Mixer is running. This is not very intuitive, and if this is behaviour Microsoft intends to keep, it isn't going to work well for our users.
Having the volume of NVDA sounds controlled outside of NVDA is a little cumbersome.
Description of user facing changes
There will now only be one entry for NVDA in the Windows Volume Mixer which will control all NVDA audio.
There will be a slider in NVDA Advanced Settings to control the volume of sounds.
Description of development approach
Removed the ability to set a WASAPI session and volume.
Added a method to set the volume of a stream. This also allows you to set the volume of individual channels, though that's not used yet.
Added a slider in Advanced Settings to control the volume of NVDA sounds. This is disabled if the sound volume is following the voice volume.
@seanbudd seanbudd mentioned this pull request Jul 21, 2023
6 tasks
seanbudd added a commit that referenced this pull request Jul 25, 2023
Changes to handle #15150
Follow up to #14697, #14896, #15038, #15097, #15145

Summary of the issue:
WASAPI usage is known to cause intermittent crashing in #15150.
Generally, WASAPI code has not been proven to be stable.
Due to this, it should not be enabled by default in 2023.2.
WASAPI can be re-enabled by default once it is proven to be stable.

Description of user facing changes
Disable WASAPI by default, preventing intermittent crashing #15150

Description of development approach
Turn the WASAPI checkbox into a feature flag, so that it can easily be re-enabled in future.

Testing strategy:
Manual testing
Upgrading the profile: Test starting NVDA with the WASAPI config value set to "True/False" instead of a "enabled/disabled/default".

Test the various controls related to WASAPI - ensure they are saved, applied and respected correctly.
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.

Ability to adjust volume of sounds
7 participants