Skip to content

Use endpoint IDs instead of device friendly names to store user's preferred output device#17547

Merged
SaschaCowley merged 22 commits into
masterfrom
endpointIds
Jan 6, 2025
Merged

Use endpoint IDs instead of device friendly names to store user's preferred output device#17547
SaschaCowley merged 22 commits into
masterfrom
endpointIds

Conversation

@SaschaCowley

@SaschaCowley SaschaCowley commented Dec 18, 2024

Copy link
Copy Markdown
Member

Link to issue number:

Closes #17497

Summary of the issue:

NVDA currently stores the friendly name of the user's preferred audio output device.

Description of user facing changes

None.

Description of development approach

In nvdaHelper/local/wasapi.cpp, rewrote getPreferredDevice to fetch the preferred device directly via MMDeviceEnumerator.GetDevice.
Added manual checks that the fetched device is a render device, and that its status is active, since these conditions were guaranteed to be met since the previous code only iterated over devices which met those prerequisites.
Renamed deviceName to endpointId.

In source/mmwave.py, added a parameter to _getOutputDevices to return a value representing the system default output device.
Also made the type hints more self-documenting by using a NamedTuple.

In source/gui/settingsDialogs.py, used nvwave._getOutputDevices rather than nvwave.getOutputDeviceNames to fetch the available output devices.
When saving, used the selection index of AudioSettingsPanel.deviceList to index into the tuple of IDs to get the value to save to config.

In source/config/configSpec.py, moved the outputDevice key from speech to audio, and incremented the schema version to 14. Added an associated profile upgrade function in profileUpgradeSteps.py, and tests for same in tests/unit/test_config.py. Updated all occurrences of this config key that I am aware of to point to the new location.

In source/synthDrivers/sapi5.py, rewrote the device selection logic again to work with endpoint IDs.

Testing strategy:

Built from source and ensured that changing output devices works as expected.
Ensured that saving the config worked, and that the output device was selected correctly when restarting NVDA.

Tested activating SAPI5 with different output devices selected.

Known issues with pull request:

SAPI4 still doesn't work (#17516 ), this will be fixed in a future PR.

Endpoint ID strings are not human-readable.

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.

@coderabbitai summary

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 6c9a566a08

Comment thread source/config/configSpec.py
Comment thread source/config/profileUpgradeSteps.py
Comment thread source/config/profileUpgradeSteps.py Outdated
Comment thread source/config/profileUpgradeSteps.py
Comment thread source/config/profileUpgradeSteps.py Outdated
Comment thread source/gui/settingsDialogs.py Outdated
Comment thread source/nvwave.py Outdated
Comment thread source/nvwave.py Outdated
@SaschaCowley SaschaCowley marked this pull request as ready for review December 19, 2024 06:04
@SaschaCowley SaschaCowley requested a review from a team as a code owner December 19, 2024 06:04
@gexgd0419

Copy link
Copy Markdown
Contributor

What will be the recommended way to get the WinMM device index of the preferred output device, if a third-party synthesizer plugin cannot be migrated to WASAPI?

@SaschaCowley

Copy link
Copy Markdown
Member Author

@gexgd0419 said

What will be the recommended way to get the WinMM device index of the preferred output device, if a third-party synthesizer plugin cannot be migrated to WASAPI?

See this Microsoft code sample on how to get the waveform ID from an Endpoint ID string.

@gexgd0419

Copy link
Copy Markdown
Contributor

There are still some plugins which use nvwave.outputDeviceNameToID to convert the name stored in config to get a WinMM device index.

Will there be a new helper function to convert endpoint IDs to WinMM device indexes? Or will they be encouraged to do conversion themselves instead of relying on helper functions in nvwave?

Seems that in NVDA, the SAPI4-related code is the only place where outputDeviceNameToID is used.

@SaschaCowley

Copy link
Copy Markdown
Member Author

@gexgd0419 the plan is to remove outputDeviceNameToID and outputDeviceIDToName, and keep this functionality internal only, and only in the SAPI4 driver. Add-on authors are encouraged to migrate to using Windows core audio APIs, and to manage backward compatibility however is appropriate for them.

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice, only a couple lint fixes

Comment thread nvdaHelper/local/wasapi.cpp Outdated
Comment thread tests/unit/test_config.py
Co-authored-by: Sean Budd <sean@nvaccess.org>
@SaschaCowley SaschaCowley merged commit 0f7e961 into master Jan 6, 2025
@SaschaCowley SaschaCowley deleted the endpointIds branch January 6, 2025 06:25
@github-actions github-actions Bot added this to the 2025.1 milestone Jan 6, 2025
@seanbudd

seanbudd commented Jan 6, 2025

Copy link
Copy Markdown
Member

@SaschaCowley - has this been announced on the API mailing list?

@josephsl

josephsl commented Jan 6, 2025 via email

Copy link
Copy Markdown
Contributor

SaschaCowley added a commit that referenced this pull request Jan 7, 2025
Fixes #17587
Fixes #17588

Summary of the issue:
The profile upgrade steps introduced in #17547 would cause NVDA to crash, and the user's configuration to be reset, if the user had an audio output device set in their NVDA configuration.

Description of user facing changes
Upgrading from configuration version 13 to 14 no longer crashes NVDA and resets the user's configuration.

Description of development approach
The issue was caused because `config.profileUpgradeSteps.upgradeConfigFrom_13_to_14` attempts to import `nvwave`, which, through its own imports, eventually attempts to use gettext. Since gettext has not yet been initialised, this raises an exception. As we are so early in NVDA initialisation, this exception causes the rest of NVDA's initialisation to fail critically.

To resolve this, `_getOutputDevices` and `_AudioOutputDevice` have been moved to the new `utils.mmdevice`. I initially thought to move them to `audio.utils`, but initialisation of `audio` runs into the same problem.

Testing strategy:
Checked out `a7fa0d6`, made sure the config version was `13`, ran NVDA and selected an audio output device. Checked out `fixProfileUpgrade`, ran NVDA, and ensured that it ran as expected, the profile was correctly updated, there was no loss of settings, and the correct output device was in use.
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.

Switch to using endpoint IDs for identifying selected output device in user configuration

6 participants