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

when sound split is disabled, the feature as it is in NVDA core now interferes with everything else that changes channel volumes, whether an add-on or not. #16287

Closed
paulber19 opened this issue Mar 10, 2024 · 30 comments · Fixed by #16331
Milestone

Comments

@paulber19
Copy link

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

Describe the solution you'd like

Some add-ons allow you to modify application channel volumes individually.
Since the introduction of the "sound split" feature, NVDA systematically returns sound to both channels of applications to hear sound equally on both channels when "Disabled" mode is selected.
In this case, the modifications made by the add-ons are canceled

Describe alternatives you've considered

The "disabled" mode should mean that NVDA does nothing in this case and works as before the feature was introduced.

Additional context

This concerns alpha versions of NVDA.

@Adriani90
Copy link
Collaborator

Cc: @mltony

@paulber19
Copy link
Author

paulber19 commented Mar 10, 2024 via email

@CyrilleB79
Copy link
Collaborator

@paulber19 could you indicate the names of these add-ons as examples? Thanks.

@mltony
Copy link
Contributor

mltony commented Mar 11, 2024

I implemented sound split in a generic enough way, so that add-on authors can reuse it to customize functionality - use audio.soundSplit.applyToAllAudioSessions function with a custom lambda. I think add-ons should go through this way, because pycaw is now included in NVDA and it's not a good idea to bring another version of pycaw in an add-on.

@XLTechie
Copy link
Collaborator

XLTechie commented Mar 11, 2024 via email

@paulber19
Copy link
Author

paulber19 commented Mar 11, 2024 via email

@paulber19
Copy link
Author

paulber19 commented Mar 11, 2024 via email

@CyrilleB79
Copy link
Collaborator

@paulber19
what you are describing just seems another sound split feature; am I correct?
If yes, there is no point in keeping it enabled in the add-on for NVDA 2024.2 since the feature is in core. You may though want to keep it in the add-on for older versions of NVDA though.

However, if @paulber19's add-on only uses functions in the API (i.e. public) for this feature, the introduction in NVDA core of Sound Split feature would be an API breaking change, what should not be allowed for 2024.2.
I did not check though which functions are used by the add-on.

@mltony do you know if the introduction of Sound Split feature in core has changed the behaviour of existing public functions?

@Adriani90
Copy link
Collaborator

An API breaking change can be allowed in 2024.3 though, is this correct?

cc: @gerald-hartig, in case sound split needs to be reverted and rescheduled to 2024.3 including documentation adjustments.

@paulber19
Copy link
Author

paulber19 commented Mar 11, 2024 via email

@mltony
Copy link
Contributor

mltony commented Mar 11, 2024

@paulber19,
so it seems that you can modify your add-on to make it to work with sound split by making use of applyToAllAudioSessions function - I don't understand why that's not enough.
@Adriani90 ,
I wouldn't call this API breaking change as sound split just inroduced some new API. The problem is that @paulber19 's addon had some functionality that is conflicting with sound split even back in the days when sound split was an add-on. I would argue that we shouldn't aim at the attitude of never breaking a single add-on, this attitude would be counterproductive, especially given the fact that there is no well-defined set of NVDA API that can be called addon API, so if we adopt that attitude, any change in nVDA core might be at risk. Any internal NVDA API might be used by an add-on. In this case, new API is not even the root cause. The root cause is that @paulber19 is using the same wasapi api as sound split. And this conflict can be resolved on the add-on side using the way I described above. So I guess the ultimate question is given such incompatibility between an add-on and a new feature, is that the responsibility of add-on or the author of new feature to resolve this? I would argue that addon author should resolve this - at least when my addons were broken by new features (like tab QuickNav command) - that was the case - I was expected to fix my addon. But LMK if you think otherwise.

@paulber19
Copy link
Author

paulber19 commented Mar 11, 2024 via email

@XLTechie
Copy link
Collaborator

XLTechie commented Mar 11, 2024 via email

@seanbudd
Copy link
Member

Closing this issue for reasons @mltony outlined. This is just a consequence of NVDA development. As already discussed, the API hasn't changed here, just the nature of NVDA's behaviour.

@seanbudd seanbudd closed this as not planned Won't fix, can't repro, duplicate, stale Mar 11, 2024
@XLTechie
Copy link
Collaborator

XLTechie commented Mar 11, 2024 via email

@seanbudd
Copy link
Member

@XLTechie Is the difference in expected behaviour just limited to interacting with add-ons? What has changed here is a new feature has been implemented to track and manage the state of stereo output. My understanding is that without add-ons, the disable state is identical to the "do not touch" case for users. Now that NVDA has an API for stereo manipulation, I think it is the responsibility of add-ons to utilise the API to maintain/respect the "disabled" state.

@cary-rowen
Copy link
Contributor

I totally agree with @mltony regarding API and 3rd party add-on compatibility.

I wouldn't call this API breaking change as sound split just inroduced some new API. The problem is that @paulber19 's addon had some functionality that is conflicting with sound split even back in the days when sound split was an add-on. I would argue that we shouldn't aim at the attitude of never breaking a single add-on, this attitude would be counterproductive, especially given the fact that there is no well-defined set of NVDA API that can be called addon API, so if we adopt that attitude, any change in nVDA core might be at risk. Any internal NVDA API might be used by an add-on. In this case, new API is not even the root cause. The root cause is that @paulber19 is using the same wasapi api as sound split. And this conflict can be resolved on the add-on side using the way I described above. So I guess the ultimate question is given such incompatibility between an add-on and a new feature, is that the responsibility of add-on or the author of new feature to resolve this? I would argue that addon author should resolve this - at least when my addons were broken by new features (like tab QuickNav command) - that was the case - I was expected to fix my addon. But LMK if you think otherwise.

@paulber19
Copy link
Author

paulber19 commented Mar 12, 2024 via email

@cary-rowen
Copy link
Contributor

Hi Paul,

Regarding the issues you mentioned:

sound split is still active even in "Off" mode.

This is already tracked by #16292.

The reason you find this discussion confusing is that I think you added to the confusion in your answer to @CyrilleB79's question.

Anyway, this issue has been closed, let us focus on the discussion of #16292.

Thanks
Cary

@LeonarddeR
Copy link
Collaborator

LeonarddeR commented Mar 16, 2024

Reopening as per #16292 (comment)

@Adriani90
Copy link
Collaborator

@paulber19 can you not achieve what you want with the current API which is added in NVDA core now?
I mean, what i understand is that your add-on adds the application by application basis feature instead of affecting all applications.

Can you not just raise a pull request for making the core feature of sound split profile based? Then a user just needs to create a new profile for the app and it can adjust the sound split settings as needed. Is it not currently possible?

Also if you still want to preserve your add-on for older NVDA versions, you can do it by stating to the users which NVDA version they should use in order for the add-on to work.

@Adriani90
Copy link
Collaborator

In my view there is no add-on for this needed anymore, if people invest a bit of energy to enhance what we have already. In this case it is probably a very small change to achieve what your add-on already can do.
Installing an add-on just for this small change is an overkill in my view.

@LeonarddeR
Copy link
Collaborator

@Adriani90 I think the title of the current issue is a bit confusing. The underlying issue here is that Sound Split touches the channel volume of applications when the feature is disabled. This means that even when sound split is disabled, the feature as it is in NVDA core now interferes with everything else that changes channel volumes, whether an add-on or not.
@paulber19 could you please fix the title of this issue?

@paulber19 paulber19 changed the title Some add-ons allow you to modify application channel volumes individually. Since the introduction of the "sound split" feature, NVDA systematically returns sound to both channels of applications to hear sound equally on both channels when "Disabled" mode is selected. In this case, the modifications made by the add-ons are canceled This mode should mean that NVDA does nothing in this case and works as before the feature was introduced.Rename the mode "disabled" to "both NVDA and other applications output sounds to both left and right" and add the mode "Disabled" so that NVDA does not modify application channels's volume. when sound split is disabled, the feature as it is in NVDA core now interferes with everything else that changes channel volumes, whether an add-on or not. Mar 20, 2024
@paulber19
Copy link
Author

paulber19 commented Mar 20, 2024 via email

@Adriani90
Copy link
Collaborator

Thanks @LeonarddeR I think I got the issue now.
@mltony what you introduced as "disabled" is actually rather enforcing "everything in both channels". I propose renaming the currently "disabled" state to "everything in both channels" and add an additional state called "disabled" which entirely disables this feature and sets the underlying API to "off".

So for example if I change my apps audio settings via an external tool such as eartrumpet
https://github.com/File-New-Project/EarTrumpet
, then the sound split feature of NVDA will not honor this when setting to disabled because it enforces its own both channels setting.

@seanbudd I think this needs a quite high priority to be fixed.

@mltony
Copy link
Contributor

mltony commented Mar 21, 2024

I stilll agree with Sean here.
When @LeonarddeR says this feature interferes with everything - that's confusing, since there are no known programs that adjust channel volume. So I hesitate to add unnecessary complexity for a completely imaginary (unless proven otherwise) use case. The only known program is @paulber19's add-on, which was incompatible with Tony's enhancements in the first place, and as I mentioned before, I provided API for @paulber19 to fix that on his side.

@LeonarddeR
Copy link
Collaborator

@mltony with respect, but do you really not want to understand what is going wrong here? Just because you are not aware of another program that adjusts channel volumes does not mean that NVDA is free to unnecessarily mess with channel volumes of third party applications without the end user's permission. If sound split is enabled, the end user explicitly chooses to do so. If sound split is disabled (default) the feature should simply be off as @Adriani90 explained in #16287 (comment) .
In fact, there is a similar audio mixer program created by Babbage (my former employee) that does similar things as sound split.

CC @seanbudd: Do you acknowledge that the sound split feature when in disabled state should not change channel volumes of other applications by any means?

@paulber19
Copy link
Author

paulber19 commented Mar 21, 2024 via email

@Adriani90
Copy link
Collaborator

@mltony are there any technical limitations that hold you back from introducing an additional state in the combo box called "disabled" which entirely disables this feature and the underlying API as I proposed above?
What is the reason not to do it? In general, any external tool that changes something in terms of audio on the system should be possible even while running NVDA. It is not an option to restrict external tools to make changes to the audio channels. If there are any technical limitations for implementing an additional "disabled" state, then this feature must be reverted unfortunately. The impact would be too negative for users if this makes it into the stable release.
If there is no technical limitation but it needs more discussion, then I propose to make this feature available only in the alpha releases and hold it back from stable or beta releases until a solution is found.

@XLTechie
Copy link
Collaborator

XLTechie commented Mar 21, 2024 via email

michaelDCurran pushed a commit that referenced this issue Apr 8, 2024
Closes #16287

Summary of the issue:
Disabling sound split still could touch channel volumes.

Description of development approach
1. If sound split mode is disabled at startup, we're not touching any volumes.
2. If user toggles to disabled mode, then first we set mode to NVDA_BOTH_APPS_BOTH to effectively restore the volumes, then set sound pslit state to off.
@nvaccessAuto nvaccessAuto modified the milestone: 2024.2 Apr 8, 2024
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 a pull request may close this issue.

9 participants