-
-
Notifications
You must be signed in to change notification settings - Fork 646
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
Sound split #16071
Sound split #16071
Conversation
See test results for failed build of commit 6b4c666ae8 |
See test results for failed build of commit 9df913ae92 |
See test results for failed build of commit e523884049 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sound split only works in wasapi mode.
While this is obviously fine I'd be nice to have a more technical explanation as to why in the PR description. In particular it is important to know if supporting this is impossible, too difficult to be worth doing or just a intentional design decision.
multiple lint errors are coming from pycaw interface definitions. I hope an exemption can be made here, since we don't want to make potential updating of these files to require manual reformatting every time just to make linter happy.
These can easily be ignored in tests\lint\flake8.ini
. Since we do the same for UIAutomation COM interface I don't think that will be problematic.
During shotdown we try to restore audio volume of all applications. However if an app is currently not playing any sounds, it doesn't have an active audio session and therefore there is no way to update its volume. As a result some applications might be playing sounds only in 1 channel long after NVDA is shut down. This is a known issue and there appears to be no work around.
I have no experience with the audio stuff, but my initial intuition will be to register to a notification for destroying sessions and restore the original volume there. Is that possible?
source/audio.py
Outdated
result.append(SoundSplitState.NVDA_LEFT) | ||
if 'RIGHT' in self.name: | ||
result.append(SoundSplitState.NVDA_RIGHT) | ||
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this method be simplified to:
return [SoundSplitState.OFF, self]
Glad to see this PR has been drafted. But I sincerely hope you will consider the following request. Sometimes, when we are watching a movie, we often want to appreciate the sound effects of this movie, but the sound of NVDA is relatively not so important. |
@mltony, a big thank you for taking the time to integrate this feature in core! I think that it will be a real plus in NVDA's features. I'll take time tomorrow and next week to test it, and will provide review comments (if needed), probably more on the UX point of view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice and useful feature!
I have written a first set of comments, but will continue testing later.
Also a general note:
Implementing Sound Split for legacy audio mode is technically possible since you have done it in the add-on. But IMO, it's not worth doing it since NVDA moves to WASAPI.
Also one small issue: If it is not possible for technical reasons or other, this request should not block this PR though, since the feature as developed in this PR is already more than useful, e.g. in meetings where stereo is of no use. |
|
I believe this is not possible - in coreAudio API I can only set volumes for left and right channels. What you are talking about would be mixer functionality I guess. I mentioned this both in this PR and documentation. |
@lukaszgo1 , @CyrilleB79, I addressed all your comments. Also @cary-rowen, I implemented your idea as well. |
Great, thanks @mltony for the great work. |
See test results for failed build of commit db34b63cd0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some more review items, if you want to have a look.
I have to stop now; maybe there will be other ones tomorrow.
Key: ``NVDA+alt+s`` | ||
|
||
This option allows you to make NVDA sound output to be directed to either left or right channel and application volume to be directed to the other channel. This could be useful for users of headphones or stereo speakers. | ||
- Disabled sound split: both NVDA and other applications output sounds to both left and right channels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list is now incomplete.
If you remove the first section, you should put in this paragraph all the information that was in the first one.
user_docs/en/userGuide.t2t
Outdated
@@ -546,6 +546,27 @@ A gesture allows cycling through the various speech modes: | |||
|
|||
If you only need to switch between a limited subset of speech modes, see [Modes available in the Cycle speech mode command #SpeechModesDisabling] for a way to disable unwanted modes. | |||
|
|||
++ Sound split modes ++[SpeechModes] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph is not needed here.
Its content should be moved in the paragraph describing the sound split control in the audio settings panel, and duplicate information (e.g. NVDA+alt+S gesture) should be merged.
You have probably made something similar to speech mode. But speech mode has no control in the GUI to set a specific speech mode (off, beeps, etc.); that's why its description cannot be done along with settings descriptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then perhaps I should remove the combobox from settings as well - if speech mode doesn't have a combo box then why sound split needs one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then perhaps I should remove the combobox from settings as well - if speech mode doesn't have a combo box then why sound split needs one?
Usually, NVDA settings are reachable either through the GUI (settings dialog) or through gestures (assigned or unassigned). The features that are not modifiable through settings dialogs are the exception.
Having the feature controllable in the settings dialog allows:
- saving the config on exit and restoring it at NVDA startup
- using different profiles
- a better discoverability of the feature since its control is visible in the GUI; someone can use it even without knowing the shortcut NVDA+alt+S
IMO, this is suitable for sound split:
- maybe this is not very common but we cannot exclude that someone wants to have sound split always enabled, so also at NVDA startup
- profiles may be useful in the following cases:
- VLC profile using: NVDA left and application in both channels
- Teams / Skype / any conferencing application using : NVDA left and application right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge this section into SelectSoundSplitMode
and CustomizeSoundSplitModes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as cyrille outlined here, the paragraph is not necessary. The documentation in settings should be sufficient
if gui.messageBox( | ||
_( | ||
# Translators: Warning shown when 'OFF' sound split mode is disabled in settings. | ||
"You did not choose 'Off' as one of your sound split mode options. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this warning is not needed. I do not know how often someone has only one channel broken... The risk to lose speech is very low and not due to NVDA configuration in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but would like to hear more feedback on this. The warning certainly doesn't hurt much, but I don't imagine anyone wanting remove "Off" from the list.
# Translators: Message shown when no sound split modes are enabled. | ||
_("At least one sound split mode has to be checked."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least two modes for a cycling command:
# Translators: Message shown when no sound split modes are enabled. | |
_("At least one sound split mode has to be checked."), | |
# Translators: Message shown when less than two sound split modes are enabled. | |
_("At least two sound split mode have to be checked."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again I see no point in forcing user to select the second mode if they don't want it. It is still going to cycle through 1 mode, nothing is going to break. Let's not add artificial restrictions on the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again I see no point in forcing user to select the second mode if they don't want it. It is still going to cycle through 1 mode, nothing is going to break. Let's not add artificial restrictions on the user.
First of all, note that the speech mode has the same type of configuration of available values in a cycling command. And the choice has been done to keep two values. So it would be consistent to make the same choice for both commands. Still, we can re-discuss it here if you think that the choice made for speech mode was unadapted.
Second, there is no use case that I know where the user would configure only one sound split value in the command. If the user configures it with only one value, it's very likely unintended. If the user only wants one value, they will just not use the cycling command, so there is no point in configuring the available values for this command.
Thus, if only one value is present in the cycling command, it's better to warn the user as early as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My opinion is that in this case cycling by a single option is justified. I don't think comparing this with the speech modes cycling makes sense, since the only way to change the mode is by a keyboard command, or restart of NVDA. For sound split you can have a default preferred mode and a second one which you need rarely, but switching to it has to be done fast. In that case you use the cycle command as a single way toggle and enable whatever mode you use as your main one from the GUI afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So let's take an example to check we understand well each other:
- default preferred mode = disabled
- second mode rarely needed, but switching to it has to be done fast = NVDA left and other apps right.
The user uses the second mode when receiving a call, so that switching can be done very fast.
If there are two modes available in the cycling command, I do not know why switching would be less fast:
- if the user is already in split mode when the call arrives, there is nothing to do, so it's as fast as possible
- if the user is in default mode when the call arrives, just pressing NVDA+alt+S is needed, which is as fast as with only one mode in the cycling command.
Have I missed something?
I maintain that a cycling command with only one possible value is a confusing user experience. So if there is no use case where a one-value cycling command is more efficient than a two-value cycling command, there is no point in allowing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanbudd, could you give your opinion about allowing or not to have only one mode in the cycling command?
My opinion is that there is no use case for a cycling command with only one mode. And if such use case is exhibited, I think that a cycling command with only one value is a bad UX; in this case the UX should be thought differently to support this use case more clearly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CyrilleB79 - I mentioned here: #16071 (comment)
I agree, but would like to hear more feedback on this. The warning certainly doesn't hurt much, but I don't imagine anyone wanting remove "Off" from the list.
See test results for failed build of commit b4f60f4746 |
You can add |
# Translators: Message shown when no sound split modes are enabled. | ||
_("At least one sound split mode has to be checked."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My opinion is that in this case cycling by a single option is justified. I don't think comparing this with the speech modes cycling makes sense, since the only way to change the mode is by a keyboard command, or restart of NVDA. For sound split you can have a default preferred mode and a second one which you need rarely, but switching to it has to be done fast. In that case you use the cycle command as a single way toggle and enable whatever mode you use as your main one from the GUI afterwards.
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
See test results for failed build of commit 7c0845a988 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mltony
Closes #16269 Fixup of #16071 Summary of the issue: Exception _ctypes.COMError is thrown at startup on AppVeyor CI runner because hno sound device is present. Description of user facing changes N/A Description of development approach Catching and logging exception. Returning from soundSplit.initialize function so that initialization can continue. Testing strategy: Not sure how to test on AppVeyor, but judging by the exception providded - this should fix the error. Tested on my local computer to avoid syntax errors.
Related to nvaccess#16071 Summary of the issue: When linting we exclude certain directories where the code is auto generated. These exclusions are not working (this probably regressed with the Flake8 update, though I haven't checked). Description of user facing changes When linting exclusions in Flake8 configuration are once again respected. Description of development approach Flake8 considers that paths in the exclusion list are provided relative to the config file location not to the CWD, our exclusions were modified to account for this After the file was modified I started getting errors due to usage of inline comments, apparently this was never supposed to work, as per this quote from the documentation: Following the recommended settings for Python’s configparser, Flake8 does not support inline comments for any of the keys. So while this is fine:... Therefore we no longer use inline comments in the config.
Fixes nvaccess#12985 Summary of the issue: Feature request: sound split. Splits system sound into two channels: NVDA speaks in one channel (e.g. left), while all other applications play their sound in the other channel (e.g. right). Description of user facing changes Added global command NVDA+alt+s that toggles sound split between off, NVDA on the left and NVDA on the right (default behavior). Added combo box on Audio panel in NVDA settings that also allows to switch between Sound split modes. Added list of checkboxes in Audio panel, that allows to change behavior of NVDA+alt+s command: it allows to select all modes that the global command will cycle through. Description of development approach Added pycaw library as a dependency. Created file source\audio\soundSplit.py where I implemented all logic. Contrary to what I said before, I managed to implement sound split without creating an extra monitor thread. It works like this: When sound split is toggled, it uses IAudioSessionEnumerator to set volume in all currently active audio sessions. Then it does sessionManager.RegisterSessionNotification() to create a callback that listens for any new audio sessions being created, an it executes the the same volume updating function upon creation. On the next call or on shutdown we unregister the previous notification callback.
…ess#16270) Closes nvaccess#16269 Fixup of nvaccess#16071 Summary of the issue: Exception _ctypes.COMError is thrown at startup on AppVeyor CI runner because hno sound device is present. Description of user facing changes N/A Description of development approach Catching and logging exception. Returning from soundSplit.initialize function so that initialization can continue. Testing strategy: Not sure how to test on AppVeyor, but judging by the exception providded - this should fix the error. Tested on my local computer to avoid syntax errors.
What is the reason for soundSplit to be limited to only "Wasapi" mode?
THANKS.
Le 20/01/2024 01:55, mltony a écrit :
…
Link to issue number:
##12985 <#12985>
Summary of the issue:
Feature request: sound split. Splits system sound into two channels:
NVDA speaks in one channel (e.g. left), while all other applications
play their sound in the other channel (e.g. right).
Description of user facing changes
* Added global command |NVDA+alt+s| that toggles sound split between
off, NVDA on the left and NVDA on the right (default behavior).
* Added combo box on Audio panel in NVDA settings that also allows
to switch Sound split between three options mentioned in the
previous bullet point.
* Added another combo box in Audio panel, that allows to change
behavior of |NVDA+alt+s| command: it can either cycle between all
three modes, or it can skip NVDA on the left, or it can skip NVDA
on the right.
Description of development approach
1. Added relevant COM interfaces from pycaw library in
|source\comInterfaces\coreAudio |. I updated space indentations to
tab to match NVDA style and added a notice in the header stating
that these files come from pycaw library and are distributed under
a different license.
2. Added |IChannelAudioVolume| interface to
|source\comInterfaces\coreAudio\audioclient\__init__.py| - as
apparently pycaw doesn't contain all necessary COM interfaces.
3. Created file |source\audio.py| where I implemented all logic. I
didn't want to call this file soundSplit.py since there are two
more feature requests (#16052
<#16052>, #16056
<#16056>)that I am going to
implement once this is merged, that would make sense to implement
in the same file and that are not directly related to sound split.
4. Contrary to what I said before, I managed to implement sound split
without creating an extra monitor thread. It works like this:
* When sound split is toggled, it uses |IAudioSessionEnumerator|
to set volume in all currently active audio sessions.
* Then it does |sessionManager.RegisterSessionNotification()| to
create a callback that listens for any new audio sessions
being created, an it executes the the same volume updating
function upon creation.
* On the next call or on shutdown we unregister the previous
notification callback.
Testing strategy:
* Performed thorough manual testing on Windows 10 and Windows 11.
Known issues with pull request:
* Sound split only works in wasapi mode.
* multiple lint errors are coming from pycaw interface definitions.
I hope an exemption can be made here, since we don't want to make
potential updating of these files to require manual reformatting
every time just to make linter happy.
* During shotdown we try to restore audio volume of all
applications. However if an app is currently not playing any
sounds, it doesn't have an active audio session and therefore
there is no way to update its volume. As a result some
applications might be playing sounds only in 1 channel long after
NVDA is shut down. This is a known issue and there appears to be
no work around.
* There is a very unlikely race condition that might occur when an
application starts playing sound between the call
|sessionManager.GetSessionEnumerator()| and
|sessionManager.RegisterSessionNotification()|. In this case sound
split volume will not be applied to that application. I don't see
a way to fix this in code since this is rather a problem of COM
API. However turning sound split off and on again is an easy and
intuitive workaround in this case.
* Sound split won't work with applications playing sounds in mono
mode or outputting to more than 2 channels (e.g. Dolby Surround).
In practice I never encountered such applications.
Code Review Checklist:
* Documentation:
o Change log entry
o User Documentation
o Developer / Technical Documentation
o Context sensitive help for GUI changes
* Testing:
o Unit tests
o System (end to end) tests
o Manual testing
* UX of all users considered:
o Speech
o Braille
o Low Vision
o Different web browsers
o Localization in other languages / culture than English
* API is compatible with existing add-ons.
* Security precautions taken.
------------------------------------------------------------------------
You can view, comment on, or merge this pull request online at:
#16071
Commit Summary
* 781d93a
<781d93a>
Sound split
File Changes
(27 files <https://github.com/nvaccess/nvda/pull/16071/files>)
* *A* source/audio.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-0dee3ef61284f2a603ef1c1165b587fb3fa4f1dee077570de0a0b170e67d4c99>
(210)
* *A* source/comInterfaces/coreAudio.bak/__init__.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-9b9e0bfdf7ea711a7bc9d038aa37ab3c2414b7dc9bfea72b6ef7b26eec12c008>
(0)
* *A* source/comInterfaces/coreAudio.bak/audioclient/__init__.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-9db6cf442b808422859a90ddda6c61740eec3ea2a9a39f582b3b3fbc76f057e6>
(155)
* *A* source/comInterfaces/coreAudio.bak/audioclient/depend.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-4a25ca798f2977f8e2acb6980855e3bc3b073afb3734d83acc4da6fc2c939cb5>
(18)
* *A* source/comInterfaces/coreAudio.bak/audiopolicy/__init__.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-26b564dc225a702c9f533dee5eaee497077e0ea0bee7290400e63a3c071184ee>
(299)
* *A* source/comInterfaces/coreAudio.bak/constants.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-7437b4fc93e120f3ac91815aed500b6e02d4228379a46f4b262803a824afe912>
(56)
* *A* source/comInterfaces/coreAudio.bak/endpointvolume/__init__.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-9d7df8259d20dd30d967366934227381918108a40c8407b2bccdc0705a920c15>
(182)
* *A* source/comInterfaces/coreAudio.bak/endpointvolume/depend.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-c9c7c558c3c8ad712b04d3533b7444ed91239084dd45b6dd0702c6cb1ba68c2a>
(22)
* *A* source/comInterfaces/coreAudio.bak/mmdeviceapi/__init__.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-20d0ba42acc0ce0c80ba0401ff21453ef275c2a2daadfcdc0c6a220b0db152ad>
(186)
* *A*
source/comInterfaces/coreAudio.bak/mmdeviceapi/depend/__init__.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-92ec6c9a4eed7bda7f73afa68043e432710d421d20ef4f4066422fb1dcb756eb>
(52)
* *A*
source/comInterfaces/coreAudio.bak/mmdeviceapi/depend/structures.py <https://github.com/nvaccess/nvda/pull/16071/files#diff-a9acc530c054faeaffb41952187063b253ca9565ce0373f8fb68fd85901f361d>
(59)
* *A* source/comInterfaces/coreAudio/__init__.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-5e289f6b7a92845081e41565466cf68202bc3834f9b6b4bfd3235d6ca3477baa>
(0)
* *A* source/comInterfaces/coreAudio/audioclient/__init__.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-16cd9d008ca7d6b1352493a3e5056f778747d3dc91ec4fd9243595afe48ffd12>
(155)
* *A* source/comInterfaces/coreAudio/audioclient/depend.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-14d2bf8e4dec2409d1a0bbd17a336b500e8f0f208f365313f9cd16650510649d>
(22)
* *A* source/comInterfaces/coreAudio/audiopolicy/__init__.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-20c11f6d662c19163de9345046ca492575e7673f0bccb1cd16641b10cd382927>
(301)
* *A* source/comInterfaces/coreAudio/constants.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-f80ed1d9242c9403014cf6d5f1d97eb09d5ecc453507593c5a586f421bc64f8f>
(60)
* *A* source/comInterfaces/coreAudio/endpointvolume/__init__.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-6ca28310038589aa13cedd6945092dca490d12a42a1bff74bd6ef8ef170846fa>
(184)
* *A* source/comInterfaces/coreAudio/endpointvolume/depend.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-840ebad435c45cf418b694e3889715f6858d3094637c9e6639ff279c2405d6f1>
(24)
* *A* source/comInterfaces/coreAudio/mmdeviceapi/__init__.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-072da30af78872575854653c14236589f3595e93553bb460333f98de28cd9518>
(188)
* *A* source/comInterfaces/coreAudio/mmdeviceapi/depend/__init__.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-5e5bc9dc44bd08deda299f01c9d6285cbfaff9e9317850890ce1c0b5d990674a>
(54)
* *A*
source/comInterfaces/coreAudio/mmdeviceapi/depend/structures.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-d00a64fdd43d5e569cb4961096b0fdbfd4dbfae862c12d1175a3180b4822fcd6>
(61)
* *M* source/config/configSpec.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-63eadb2c933d4403ec73ca9e97c4314a4f89ed9f3d8fde080bfc11315583d348>
(2)
* *M* source/core.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-dbe862167add4fbe8e502757f792615f714c7603621c03d0c60c08a6a613ad68>
(3)
* *M* source/globalCommands.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-145e3f65de034e69a428ad8bc3aef21707ab8b1011c91a174ca2a616b1a99609>
(16)
* *M* source/gui/settingsDialogs.py
<https://github.com/nvaccess/nvda/pull/16071/files#diff-6486e3db41b2272c03b84758b460cc4269d5ca218874391f58633f5d9b3968b1>
(31)
* *M* user_docs/en/changes.t2t
<https://github.com/nvaccess/nvda/pull/16071/files#diff-3e39e8a2211be1bdc6228c141cb9d7e88c0698037337db440f7957609b672395>
(1)
* *M* user_docs/en/userGuide.t2t
<https://github.com/nvaccess/nvda/pull/16071/files#diff-5290814766111d44cf9670be8301f5af9457dba7a8f936dbbbf207883000a4b0>
(13)
Patch Links:
* https://github.com/nvaccess/nvda/pull/16071.patch
* https://github.com/nvaccess/nvda/pull/16071.diff
—
Reply to this email directly, view it on GitHub
<#16071>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZLFFGOVILIDT4VA4YKHY3YPMIYHAVCNFSM6AAAAABCCXXSWSVHI2DSMVQWIX3LMV43ASLTON2WKOZSGA4TCNZXGU2DIMA>.
You are receiving this because you are subscribed to this
thread.Message ID: ***@***.***>
|
Because its technical implementation is not the same with and without WASAPI. Since WASAPI is the default mode and should remain the only one possible in the future, there was no point in spending time to also support sound split without WASAPI. |
Reverts PR Reverts #16273 Issues fixed Fixes #16409 Fixes #16402 Issues reopened #16052 Brief reason for revert We started with mltony creating: #16051 Feature request: Sound split Which was a duplicate of: #12985 Audio settings with stereo headset (or speakers) - Send NVDA sounds on one side and the rest of Windows sounds on the other side And then implemented by: #16071 Sound split mltony also created: #16052 Feature request: add command to adjust volume of all applications except for NVDA Which was implemented in: #16273 Keystrokes to adjust applications volume and mute This PR was approved and merged and was then found to cause issues: #16402 Unmuting other apps does not work as expected #16409 Apps mute and volume features work very unexpectedly with WASAPI disabled Due to these issues and the considerable debate on the approach, the above PR #16273 was reverted by: #16440 As an alternative to the revert #16440 to resolve the 2 issues (#16402, #16409) and keep #16273, mltony created: #16404 The question now becomes, how do we proceed from here? NV Access's position is that the sound split functionality (#16051) is a useful feature to add into core. However, due to the following reasons, we believe that further work on the volume adjustment features (#16273, #16404) to improve the UX is required on a branch (off master/alpha) before it can be added back in: Windows sound mixer has reasonable accessibility. Sound split on its own provides value to users. The UX of swapping between NVDA volume control and windows volume control needs to be resolved. The UX of resolving volume issues due to NVDA crashes needs to be resolved. As one of the contributors on the threads said, "So now there are two mixers in the chain, one of which can be invisible, and overrides the other, or makes its settings relative instead of absolute."
Various issues found in Sound Split feature description in the User Guide and in the GUI: The User Guide does not explain the difference between "Sound split disabled" mode and "NVDA in both channels and applications in both channels" mode. In the User Guide, the default option is called "Disabled sound split", but "Sound split disabled" in the GUI. In the User Guide, the description of the way to restore the sound in both channels after a crash is not always correct, especially now that the "disabled" option does not modify audio processing at all. The GUI warning when unchecking the Disabled mode is not correct since there are various other possible modes with speech in both channels (among them the new "NVDA both and app both"). The removal of this warning had been discussed and approved in Sound split #16071 (comment) but never completed. Description of user facing changes User Guide updated for 3 first items. Warning removed from the GUI
Link to issue number:
Fixes #12985
Summary of the issue:
Feature request: sound split. Splits system sound into two channels: NVDA speaks in one channel (e.g. left), while all other applications play their sound in the other channel (e.g. right).
Description of user facing changes
NVDA+alt+s
that toggles sound split between off, NVDA on the left and NVDA on the right (default behavior).NVDA+alt+s
command: it allows to select all modes that the global command will cycle through.Description of development approach
source\audio\soundSplit.py
where I implemented all logic.IAudioSessionEnumerator
to set volume in all currently active audio sessions.sessionManager.RegisterSessionNotification()
to create a callback that listens for any new audio sessions being created, an it executes the the same volume updating function upon creation.Testing strategy:
Known issues with pull request:
sessionManager.GetSessionEnumerator()
andsessionManager.RegisterSessionNotification()
. In this case sound split volume will not be applied to that application. I don't see a way to fix this in code since this is rather a problem of COM API. However turning sound split off and on again is an easy and intuitive workaround in this case.Code Review Checklist: