Skip to content

Remove winmm support from NVDA#17496

Merged
SaschaCowley merged 28 commits into
masterfrom
i16080
Dec 13, 2024
Merged

Remove winmm support from NVDA#17496
SaschaCowley merged 28 commits into
masterfrom
i16080

Conversation

@SaschaCowley

@SaschaCowley SaschaCowley commented Dec 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Link to issue number:

Closes #16080
Closes #2067

Summary of the issue:

The support for WASAPI (and Windows' Core Audio APIs more broadly) in NVDA is now quite mature, and maintenance of our winmm support is becoming untenable.

Description of user facing changes:

The option to use WASAPI for audio output has been removed from NVDA's advanced settings. This has been on by default for some time, so it is unlikely to affect most users.

As the Mmdevice API does not have the concept of an ID to refer to the default output device, "Microsoft Sound Mapper" is no longer an option in the output device selection in Audio Settings. Users can instead choose "Default output device".

Description of development approach

Removed all references to winmm in nvwave.py. Rewrote the device enumeration to use the Mmdevice API, via pycaw. Slightly modified the device selection logic in the settings GUI in light of Mmdevice not having an equivalent to Microsoft Sound Mapper.

Testing strategy:

System and unit tests, as well as running NVDA, and changing output devices, including plugging and unplugging headphones while NVDA was running.

Known issues with pull request:

None

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

@SaschaCowley SaschaCowley requested a review from a team as a code owner December 10, 2024 06:30
@SaschaCowley SaschaCowley linked an issue Dec 10, 2024 that may be closed by this pull request
Remove the "use WASAPI for audio output" setting from the GUI #16080
Closed
@SaschaCowley

SaschaCowley commented Dec 10, 2024

Copy link
Copy Markdown
Member Author

@coderabbitai please open an issue related to this one, for switching to using endpoint IDs rather than friendly names for identifying the selected output device in user configuration.

@SaschaCowley SaschaCowley requested a review from a team as a code owner December 10, 2024 06:32
@coderabbitai coderabbitai Bot mentioned this pull request Dec 10, 2024
Closed
@coderabbitai

coderabbitai Bot commented Dec 10, 2024

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@SaschaCowley: I've opened a new GitHub issue as requested: #17497

🔗 Analysis chain
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Open a new GitHub issue with the requested details
gh issue create \
  --title "Switch to using endpoint IDs for identifying selected output device in user configuration" \
  --body "As per [PR #17496](https://github.com/nvaccess/nvda/pull/17496) and [this comment](https://github.com/nvaccess/nvda/pull/17496#issuecomment), we need to switch to using endpoint IDs rather than friendly names for identifying the selected output device in user configuration. Requested by @SaschaCowley." \
  --assignee "SaschaCowley"

Length of output: 499

✏️ Learnings added 
Learnt from: SaschaCowley
URL: https://github.com/nvaccess/nvda/pull/17496

Timestamp: 2024-12-10T06:33:03.209Z
Learning: When identifying selected output devices in NVDA's user configuration, use endpoint IDs rather than friendly names.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

CyrilleB79
CyrilleB79 suggested changes Dec 10, 2024
View reviewed changes

@CyrilleB79 CyrilleB79 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This PR seems incomplete:

  • Various audio features (e.g. sound split, mute other apps, etc.) only work if WASAPI is enabled:
    • in settings dialog, the check of WASAPI to enable or disable controls for such settings should be removed (see (nvwave.usingWasapiWavePlayer calls)
    • in the User Guide, the documentation of these settings mention WASAPI requirement; this should be removed
  • In config spec, WASAPI should be removed from audio section, and this removal should be documented in API-breaking changes. Or in case you want to keep it as a no-effect parameter, you should at least document it in the change log and remove all of its usage in NVDA's code.
  • I also wonder if the removal of AdvancedPanelControls.wasapiComboBox should be documented in the API-breaking changes.

@SaschaCowley SaschaCowley marked this pull request as draft December 10, 2024 22:18
@SaschaCowley SaschaCowley marked this pull request as ready for review December 11, 2024 03:42
@SaschaCowley

SaschaCowley commented Dec 11, 2024

Copy link
Copy Markdown
Member Author

@CyrilleB79 thanks for the thorough notes. I believe I've addressed all of your points now.

@SaschaCowley SaschaCowley mentioned this pull request Dec 11, 2024
Closed
seanbudd
seanbudd reviewed Dec 11, 2024
View reviewed changes

@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.

Thanks @SaschaCowley - mostly looks good to me!

Comment thread source/audio/appsVolume.py
Comment thread source/nvwave.py Outdated
Comment thread source/nvwave.py Outdated
Comment thread source/nvwave.py Outdated
seanbudd
seanbudd approved these changes Dec 11, 2024
View reviewed changes
Comment thread source/nvwave.py Outdated
Comment thread source/nvwave.py
seanbudd
seanbudd approved these changes Dec 12, 2024
View reviewed changes

@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.

Thanks @SaschaCowley

Qchristensen
Qchristensen approved these changes Dec 13, 2024
View reviewed changes

@Qchristensen Qchristensen 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.

Reads well, good work.

@SaschaCowley SaschaCowley merged commit 6d02759 into master Dec 13, 2024
@SaschaCowley SaschaCowley deleted the i16080 branch December 13, 2024 00:25
@github-actions github-actions Bot added this to the 2025.1 milestone Dec 13, 2024
@Adriani90

Adriani90 commented Dec 13, 2024

Copy link
Copy Markdown
Collaborator

Thanks Sascha this is very appreciated cleanup.

@SaschaCowley SaschaCowley mentioned this pull request Dec 13, 2024
Merged
5 tasks
@CyrilleB79

CyrilleB79 commented Dec 13, 2024

Copy link
Copy Markdown
Contributor

@SaschaCowley, after this PR, if I plug an USB headset, but select my PC speaker for NVDA audio output, the sound still comes in the headset rather than from PC sdpeakers.

@Adriani90

Adriani90 commented Dec 14, 2024

Copy link
Copy Markdown
Collaborator

@SaschaCowley it seems the NVWave advanced logging channel is still there, I mean the one introduced in #11582 and #11614. Does it have to be removed as well? Or is it not related?

@OzancanKaratas OzancanKaratas mentioned this pull request Dec 15, 2024
Closed
SaschaCowley added a commit that referenced this pull request Dec 15, 2024
@SaschaCowley
Fixed SAPI5 initialisation (#17513)
782c3d8 
Closes #17512
Fix-up of #17496

Summary of the issue:
After the removal of winmm support, SAPI5 synthesisers failed to initialise.
This is because we switched from integer-based IDs as used by winmm, to ID strings as used by Windows Core Audio.

Description of user facing changes
SAPI5 synthesisers now initialise correctly.

Description of development approach
Rather than calling `outputDeviceNameToID` to index into the audio outputs returned by SAPI, iterate over them and look for one whose `Description` matches the friendly name of the output device to use as stored in the user's config.

Testing strategy:
Tested loading SAPI5 with a number of output devices selected, and changing output devices with SAPI5 loaded.

Known issues with pull request:
None.
@nishimotz nishimotz mentioned this pull request Apr 14, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@Qchristensen Qchristensen Qchristensen approved these changes

+1 more reviewer

@CyrilleB79 CyrilleB79 CyrilleB79 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2025.1

Development

Successfully merging this pull request may close these issues.

Remove the "use WASAPI for audio output" setting from the GUI Audio output device names truncated to 32 characters

5 participants

@SaschaCowley @Adriani90 @CyrilleB79 @seanbudd @Qchristensen