nvaccess / nvda

NVDA, the free and open source Screen Reader for Microsoft Windows
Other
2.07k stars 624 forks source link

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

Open Adriani90 opened 7 months ago

Adriani90 commented 7 months ago

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 commented 7 months ago

Blocked by #14386 #15483

Adriani90 commented 7 months ago

@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 commented 7 months ago

Also blocked by #12985 / #16071, or alternatively by https://github.com/opensourcesys/soundSplitter/issues/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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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. https://github.com/nvaccess/nvda/issues/15483#issuecomment-1815706378 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 commented 7 months ago

CC @davidacm I believe you have done some work in this area, and might have some useful commentary.

mltony commented 7 months ago

@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 commented 7 months ago

I think there is no checkbox needed for playing white noise. If this makes sense and is required in order to solve the issues raised in connection with the sound being cut or interupted, I think it makes sense to just introduce it and then see what the feedback from the community is. If there are significant bugs encountered on the way, it can be reverted.

But Jamie already expressed some concerns there, maybe we can discuss them on a corresponding PR. His solution seems promising already, so maybe we take that path first and see how far we come.

Anyway, this should not block the removal of the WASAPI setting from the GUI.

Von: mltony @.> Gesendet: Mittwoch, 24. Januar 2024 23:08 An: nvaccess/nvda @.> Cc: Adriani90 @.>; Author @.> Betreff: Re: [nvaccess/nvda] Remove the "use WASAPI for audio output" setting from the GUI (Issue #16080)

@jcsteh https://github.com/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.

— Reply to this email directly, view it on GitHub https://github.com/nvaccess/nvda/issues/16080#issuecomment-1908995129 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AGVCP4L4ZJ2DAA6E3NAZN2DYQGA5TAVCNFSM6AAAAABCFUZW62VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBYHE4TKMJSHE . You are receiving this because you authored the thread. https://github.com/notifications/beacon/AGVCP4NZG6YKUW6QZQIQHT3YQGA5TA5CNFSM6AAAAABCFUZW62WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTRZD2DS.gif Message ID: @. @.> >

OzancanKaratas commented 7 months ago

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 commented 7 months ago

i think playing white noise should be configurable because not everyone need this feature. User will turn it on if they need it.

On 1/25/24, Ozancan Karataş @.***> wrote:

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.

-- Reply to this email directly or view it on GitHub: https://github.com/nvaccess/nvda/issues/16080#issuecomment-1909753118 You are receiving this because you are subscribed to this thread.

Message ID: @.***>

-- with best regards Beqa Gozalishvili Tell: +995593454005 Email: @.*** Web: https://gozaltech.org Skype: beqabeqa473 Telegram: https://t.me/gozaltech facebook: https://facebook.com/gozaltech twitter: https://twitter.com/beqabeqa473 Instagram: https://instagram.com/beqa.gozalishvili

beqabeqa473 commented 7 months ago

otherwise this would affect battery life.

On 1/25/24, Beqa Gozalishvili @.***> wrote:

i think playing white noise should be configurable because not everyone need this feature. User will turn it on if they need it.

On 1/25/24, Ozancan Karataş @.***> wrote:

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.

-- Reply to this email directly or view it on GitHub: https://github.com/nvaccess/nvda/issues/16080#issuecomment-1909753118 You are receiving this because you are subscribed to this thread.

Message ID: @.***>

-- with best regards Beqa Gozalishvili Tell: +995593454005 Email: @.*** Web: https://gozaltech.org Skype: beqabeqa473 Telegram: https://t.me/gozaltech facebook: https://facebook.com/gozaltech twitter: https://twitter.com/beqabeqa473 Instagram: https://instagram.com/beqa.gozalishvili

-- with best regards Beqa Gozalishvili Tell: +995593454005 Email: @.*** Web: https://gozaltech.org Skype: beqabeqa473 Telegram: https://t.me/gozaltech facebook: https://facebook.com/gozaltech twitter: https://twitter.com/beqabeqa473 Instagram: https://instagram.com/beqa.gozalishvili

OzancanKaratas commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

zersiax commented 5 months ago

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

seanbudd commented 5 months ago

Removing WinMM/the checkbox is the same idea/issue

jcsteh commented 5 months ago

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 commented 5 months ago

I don't think #15483 should be a blocker now. After #16099, the behaviour should be equivalent between WASAPI and WinMM, as confirmed in https://github.com/nvaccess/nvda/issues/15483#issuecomment-1815706378:

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

seanbudd commented 5 months ago

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