Skip to content

Restore channel volume of applications upon exit#16312

Merged
michaelDCurran merged 1 commit into
nvaccess:masterfrom
mltony:soundSplitFix3
Apr 8, 2024
Merged

Restore channel volume of applications upon exit#16312
michaelDCurran merged 1 commit into
nvaccess:masterfrom
mltony:soundSplitFix3

Conversation

@mltony

@mltony mltony commented Mar 15, 2024

Copy link
Copy Markdown
Contributor

Link to issue number:

Closes #16292

Summary of the issue:

When sound split is enabled, if an application is closed while NVDA is still running, its channel volume would be preserved even after the app is restarted later without NVDA.

Description of user facing changes

N/A

Description of development approach

Catching on_state_changed notification and when new_state == "Expired" - this means that the applciation just exited - we restore both left and right channel to full volume.

Testing strategy:

Manual.

Known issues with pull request:

N/A

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.

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I have reopened #16287. I'd prefer this pr to fix that as well, i.e. ensure that soundsplit disabled really means that it is disabled.

Looking at the code of sound split in general, I have another concern. It looks like that Sound SPlit ignores the volume of every channel, simply resetting it to 1.0. What if an app prefers to change channel volumes to a lower level? I think sound split should respect that by using the average channel volume rather than a hardcoded value of 1.0

f"Audio session for pid {self.pid} has {channelCount} channels instead of 2 - cannot set volume!"
)
return
channelVolume.SetChannelVolume(0, 1.0, None)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why should the volume be 1.0? What if the application itself lowered the volume of its channels?
I think the volume of both channels should be restored to the volume of the active channel. SO if soundsplit was active with NVDA left, application right and the application lowered its volume to 0.5, both channels should be set to 0.5 instead of 1.0.

@mltony

mltony commented Mar 16, 2024

Copy link
Copy Markdown
Contributor Author

In practice I've never seen applications to lower down their volume using IChannelAudioVolume interface. So restoring to 1.0 seems to be good enough. Moreover, the volume set via ISimpleAudioVolume is different from IChanelAudioVolume. If we assume that apps adjust their channel volume using simple volume, then their volume will be preserved. If apps adjust their volume via channel volume, then it is going to interfere with sound split no matter what, but again, I haven't seen this.

@paulber19

paulber19 commented Mar 17, 2024 via email

Copy link
Copy Markdown

@LeonarddeR

Copy link
Copy Markdown
Collaborator

In practice I've never seen applications to lower down their volume using IChannelAudioVolume interface. So restoring to 1.0 seems to be good enough.

Honestly, I don't care much about practice. I think it is common sense that NVDA should only change/touch things that are absolutely necessary. In other words, NVDA should leave my system as it was before after stopping it, especially when running as a portable copy. This is a fundamental principle for me, and the current implementation violates that.

@mltony

mltony commented Mar 19, 2024

Copy link
Copy Markdown
Contributor Author

Sound split already restores all volumes to 1 upon exit. I don't see why in this feature the behavior should be different.
In theory it would be possible to remember previous volume and try to restore to this volume when application exits. But:

  1. I haven't observed any applications adjusting channel volume, so testing is going to be impossible.
  2. This would make code more complicated - and I hesitate to add unnecessary complexity just for an imaginary and untestable scenario.
  3. The only scenario I can imagine is when another application manages channel volumes. In this case that application would conflict with sound split no matter what. I can imagine corner cases where NVDA and said application adjust volumes at the same time. Race condition would be unavoidable. So Given utility of sound split I would say we should claim that sound split is not compatible with said application if one is ever to be discovered, because making it compatible is going to be hard if ever possible.

@mltony mltony marked this pull request as ready for review March 19, 2024 19:47
@mltony mltony requested a review from a team as a code owner March 19, 2024 19:47
@mltony mltony requested a review from michaelDCurran March 19, 2024 19:47

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sound split already restores all volumes to 1 upon exit. I don't see why in this feature the behavior should be different.

We are talking about Sound SPlit here. So if in this change the volume shouldn't be restored to 1.0. it should also be applied to sound split in general.

3. The only scenario I can imagine is when another application manages channel volumes. In this case that application would conflict with sound split no matter what. I can imagine corner cases where NVDA and said application adjust volumes at the same time. Race condition would be unavoidable. So Given utility of sound split I would say we should claim that sound split is not compatible with said application if one is ever to be discovered, because making it compatible is going to be hard if ever possible.

This actually emphasizes my concern that Sound Split touches channel volumes in the off state. I'm happy to accept that sound split is incompatible with some applications and even understand that these situations could be imaginary. But don't unnecessarily fiddle with my system parameters please.

@michaelDCurran michaelDCurran merged commit 3eaafef into nvaccess:master Apr 8, 2024
@nvaccessAuto nvaccessAuto added this to the 2024.2 milestone 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 this pull request may close these issues.

Sound split: closing NVDA doesn't restore panning for closed applications

5 participants