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

Allow the volume of NVDA sounds to be set within NVDA. #15038

Merged
merged 4 commits into from Jun 22, 2023

Conversation

jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Jun 20, 2023

Link to issue number:

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:

  1. 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.
  2. 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.
  3. 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.
  4. Having the volume of NVDA sounds controlled outside of NVDA is a little cumbersome.

Description of user facing changes

  1. There will now only be one entry for NVDA in the Windows Volume Mixer which will control all NVDA audio.
  2. There will be a slider in NVDA Advanced Settings to control the volume of sounds.

Description of development approach

  1. Removed the ability to set a WASAPI session and volume.
  2. 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.
  3. 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.

Testing strategy:

All the tests from #14896, plus:

  1. Disabled "Volume of NVDA sounds follows voice volume".
  2. Lowered the NVDA sounds volume to 10%.
  3. Switched between focus and browse modes to verify that sounds played quieter.
  4. Increased the NVDA sounds volume to 100%.
  5. Switched between focus and browse modes to verify that sounds played louder.
  6. Enabled "Volume of NVDA sounds follows voice volume".
  7. Set the voice volume to 50%.
  8. Switched between focus and browse modes to verify that sounds played quieter.

Known issues with pull request:

No user facing issues. However:

  1. I'm wondering whether I should make AudioPurpose, the purpose argument to WavePlayer and the setVolume method private (underscore prefixed) for now. I don't anticipate any further changes, but nor did I anticipate changes to the previous session API. Making it private would allow us to ensure this is stable internally before making it part of the public API. This might be overkill though.
  2. This does change the WavePlayer API in beta (but not in release). I don't think that's considered API breaking if this gets merged to beta, but it's worth noting. It will be a problem if we don't merge this to beta.

Change log entries:

In this entry in New features:

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

The second line should be adjusted to say:
This can be done in Advanced settings. (#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 June 20, 2023 11:26
@brunoprietog
Copy link

Thanks!

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.

Generally looks good to me

source/config/configSpec.py Outdated Show resolved Hide resolved
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@seanbudd seanbudd self-requested a review June 21, 2023 07:40
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

@CyrilleB79
Copy link
Collaborator

@jcsteh what about config spec and profiles?

When merging WASAPI I had warned about the "audio" section not being in BASE_ONLY_SECTIONS. For WASAPI on/off, it did not matter since this parameter is only looked at when starting NVDA.

But what about other parameters of audio section? Are they meant to be profile dependant?

Also, is there any plan to move all those audio settings out of the advanced settings panel? I think that we should not wait for years before moving them in their own section. I understand however that WASAPI checkbox and all parameter depending of it remain in Advanced settings for NVDA 2023.2.

@jcsteh
Copy link
Contributor Author

jcsteh commented Jun 21, 2023

But what about other parameters of audio section? Are they meant to be profile dependant?

I don't see any reason they should be profile independent, though I'm not sure whether they all currently behave properly when you switch profiles. I can't see any reason the volume settings shouldn't switch properly across profiles.

Also, is there any plan to move all those audio settings out of the advanced settings panel? I think that we should not wait for years before moving them in their own section.

I don't have a specific plan to do this, but yes, I agree they should probably be moved out of Advanced Settings once WASAPI is no longer a configurable option; i.e. once WASAPI is always enabled.

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.

Looks good. This issue comes up from time to time, so it will make a number of users happy, thanks!

@seanbudd seanbudd merged commit 30e00a9 into nvaccess:master Jun 22, 2023
1 check was pending
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Jun 22, 2023
@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.
@CyrilleB79 CyrilleB79 mentioned this pull request Nov 9, 2023
5 tasks
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.

None yet

6 participants