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.

Sign up for GitHub

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

Jump to bottom

Remove the "use WASAPI for audio output" setting from the GUI #16080

Closed
Adriani90 opened this issue Jan 22, 2024 · 28 comments · Fixed by #17496
Closed

Remove the "use WASAPI for audio output" setting from the GUI #16080

Adriani90 opened this issue Jan 22, 2024 · 28 comments · Fixed by #17496
Labels
api-breaking-change blocked p5 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority triaged Has been triaged, issue is waiting for implementation.
Milestone
2025.1

Comments

@Adriani90
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

The advanced settings GUI of NVDA is packed with some settings that are already widely tested by the community and per policy should not appear under advanced settings or in the GUI at all since their default behavior should not be changed by the user. Letting these hard settings changeable overcomplicates investigations especially when most significant issues were already fixed and the community already tested most use cases.
This applies also for use WASAPI setting, in most cases we have to find out who has manually changed this setting and who not. My understanding if the original implementation is that The users are expected to use WASAPI by default without being able to change it.

Describe the solution you'd like

Remove the "use WASAPI for audio output" setting from the GUI and enforce WASAPI to enabled for everyone.

Describe alternatives you've considered

Leave as is but we will never be able to discover all the issues that come with WASAPI since many people might use both and switch back and forth to compensate for issues deriving from WASAPI or nvWave. For true testing we should remove this setting completely from the GUI.

cc: @jcsteh what do you think? Are any drawbacks which come from only using WASAPI?
@seanbudd
Copy link
Member

Blocked by #14386 #15483

@seanbudd seanbudd added p5 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority triaged Has been triaged, issue is waiting for implementation. labels Jan 22, 2024
@Adriani90
Copy link
Collaborator Author

@seanbudd #14386 should not block this because this issue was present before introducing WASAPI and as Jamie stated it is easier to solve in WASAPI than in NVWave.
I agree #15483 still blokcs this.

@CyrilleB79
Copy link
Collaborator

Also blocked by #12985 / #16071, or alternatively by opensourcesys/soundSplitter#5 (cc @XLTechie) or any other add-on implementing this feature alone without using private API functions.

Note that the Sound Split feature compatible with WASAPI is already available in NVDAExtensionGlobalPlugin. However, my opinion is that this add-on contains so many things that it cannot be seen as a solution for 100% of the people.

Fortunately, if things go well, #16071 will land in NVDA 2024.2.

@Adriani90
Copy link
Collaborator Author

@CyrilleB79 the sound split feature should not block this, because there is no restriction people would have when using WASAPI forcefully by default. My understanding is that users will be able to choose if they use sound split or not. So this does not depend on using WASAPI or NVWave except that for NVWave would have sound split feature available only through an add-on. So yeah, I agree though forcing WASAPI introduces API breaking changes but if we don't do this people will never see progress because there will not be an incentive to change old approaches.

@mltony
Copy link
Contributor

mltony commented Jan 23, 2024

Sound Split PR doesn't block this as in fact Sound Split only works in wasapi mode.
Re #14386: In my understanding @jcsteh has a fix for #14386 - see this comment and this branch - but it's not pushed to master. I can help preparing a PR and I can also write gui setting to adjust the duration of silence if @jcsteh thinks the code is stable enough.

https://github.com/jcsteh/nvda/tree/silence

@CyrilleB79
Copy link
Collaborator

Here is the reason why I have indicated Sound Split as a blocker:
Today, the only possibility to have Sound Split feature with WASAPI is to use NVDA Extension Global Plugin add-on. For a user who wants only Sound Split feature though, it may not be an option since they have to disable all the features of this add-on, and there are several dozen of them. I do not even know if it is only possible to disable all the other features.

Thus today, some people need to disable WASAPI to use the sound split feature through the Sound Splitter add-on.

@jcsteh
Copy link
Contributor

jcsteh commented Jan 24, 2024

The code should be stable, but I think it needs more testing by others to see if it resolves the issues they're experiencing. I haven't had much feedback on it. #15483 (comment) suggests that it resolves some issues, but not others for that user.

I also vaguely recall that the various Bluetooth audio add-ons can (perhaps optionally) use something other than true digital silence, but I never really understood the use case. Has this been solidly proved to work in cases where pure digital silence does not? If so, we may want to consider changing my C++ code to push whatever that is instead, though that will be slightly less efficient because it will require memory copies.

@XLTechie
Copy link
Collaborator

XLTechie commented Jan 24, 2024 via email

@mltony
Copy link
Contributor

mltony commented Jan 24, 2024

@jcsteh
You must be talking about Bluetooth Audio add-on, that I happen to be the author of. The use case for playing white noise there is purely for debugging as in one of the early versions I discovered a bug that made silence stop yet there was no way to tell that - apart from bluetooth headphones missing beginnings of the words. Maybe I can add a new checkbox on advanced panel that would play white noise, but not sure if advanced panel is already exploding from the excess of barely useful settings there.
Ok than, I'll try to find some time to prepare a PR with your change. One more reason is that this PR would make bluetooth Audio add-on obsolete.

@Adriani90
Copy link
Collaborator Author

Adriani90 commented Jan 24, 2024 via email

@OzancanKaratas
Copy link
Collaborator

I think a “keep audio device always on” checkbox instead of white noise might help fix the issues. @jcsteh, what do you think? After ensuring that the above issues are fixed, MMWave support can be easily removed.

@beqabeqa473
Copy link
Contributor

beqabeqa473 commented Jan 25, 2024 via email

@beqabeqa473
Copy link
Contributor

beqabeqa473 commented Jan 25, 2024 via email

@OzancanKaratas
Copy link
Collaborator

I think MMWave should be removed completely. There are still some limitations due to MMWave. For example, we could not store the connection status of the sound cards using the configuration upgrade steps. See comments in pull request #15781.

@jcsteh
Copy link
Contributor

jcsteh commented Mar 12, 2024

For example, we could not store the connection status of the sound cards using the configuration upgrade steps.

We don't want to store the connection status. My suggestion there was to store the id of the device instead of the name. That would be nice, but it isn't strictly necessary and I managed to find another way around that problem that is less disruptive to existing configurations.

@zersiax
Copy link

zersiax commented Mar 12, 2024

I really don't love the idea of outright removing this checkbox. The original comment states that people switch between these two audio sources a lot and it makes it hard to debug. Fair enough, but doesn't that itself indicate that people actually use this feature?
Audio on Windows can be brittle, there can be all sorts of reasons why WASAPI falls over particularly if we mix ASIO interfaces, applications on both WASAPI and non-WASAPI endpoints concurrently etc.
It sounds to me like this is an "advanced" setting in every sense of the word, e.g., change this if you know what you're doing, leave it alone otherwise. Completely agree that we should probably be using WASAPI by default, but the justification of "hard to debug issues" doesn't really seem to ring true for me given the wide variety of configs people run NVDA on. Can't we just have an extra question added to the issue template where users indicate the status for this setting, if applicable, and leave the setting as is?

@seanbudd
Copy link
Member

Eventually we want to remove WinMM completely and move fully to WASAPI, but we can't do that while known issues exist.

@zersiax
Copy link

zersiax commented Mar 13, 2024

Again, fair enough, but wouldn't removing the checkbox essentially face the same problem?

@seanbudd
Copy link
Member

Removing WinMM/the checkbox is the same idea/issue

@jcsteh
Copy link
Contributor

jcsteh commented Mar 13, 2024

To clarify, I don't think we should be removing the checkbox unless we're also removing the WinMM code completely. That is, we remove both or neither.

@jcsteh
Copy link
Contributor

jcsteh commented Mar 13, 2024

I don't think #15483 should be a blocker now. After #16099, the behaviour should be equivalent between WASAPI and WinMM, as confirmed in #15483 (comment):

• Using your test build it seems like when WASAPI is enabled it is comparable to when WASAPI is disabled.

@seanbudd
Copy link
Member

It would be good to have final confirmation from @mwhapples that alpha has equivalent winMM and WASAPI behaviour.
If so, I don't think there are any issues blocking this any more correct?
We could probably schedule removal of winMM to 2025.1

@seanbudd seanbudd added this to the 2025.1 milestone Aug 12, 2024
@Adriani90 Adriani90 mentioned this issue Aug 22, 2024
Closed
@OzancanKaratas
Copy link
Collaborator

Now that we are in 2025.1, we can bring this up again, right?

@mwhapples, please give your information about #15483.

@seanbudd
Copy link
Member

seanbudd commented Sep 30, 2024 •
edited
Loading

@OzancanKaratas - I think we reached a resolution in that issue that WASAPI wasn't really the cause of this issue and that as far as we know, it affects only that device. We intend to remove this in 2025.1 due to discovered maintenance issues with WinMM* and we cannot maintain it long term.

@Adriani90
Copy link
Collaborator Author

@seanbudd are you saying that after 2025 NVDA will go back to WinMM? This means soundsplit feature, as well as other audio related issues will need to be reopened.
Can you elaborate more on the issues you discovered? As an user there is no issue when WASAPI is enabled which would make NVDA unusable.

@seanbudd
Copy link
Member

No - winMM will be fully removed.

@CyrilleB79
Copy link
Collaborator

@seanbudd I wonder if #16125 is blocking this issue, given the freeze and my request to raise its priority.

@seanbudd
Copy link
Member

@CyrilleB79 - I wouldn't consider this blocking. It's unfortunate, but rare enough. The maintenance of WinMM code is no longer tenable (more to come on this), so we have to accept whatever problems come with WASAPI.

@SaschaCowley SaschaCowley mentioned this issue Dec 10, 2024
Merged
5 tasks
@SaschaCowley SaschaCowley linked a pull request Dec 10, 2024 that will close this issue
Remove winmm support from NVDA #17496
Merged
5 tasks
SaschaCowley added a commit that referenced this issue Dec 13, 2024
@SaschaCowley
Remove winmm support from NVDA (#17496)
6d02759 
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 `mdevice 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api-breaking-change blocked p5 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority triaged Has been triaged, issue is waiting for implementation.
Projects
None yet
Milestone
2025.1
Development

Successfully merging a pull request may close this issue.

Remove winmm support from NVDA nvaccess/nvda
9 participants
@beqabeqa473 @jcsteh @zersiax @seanbudd @XLTechie @OzancanKaratas @CyrilleB79 @Adriani90 @mltony