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

Default output device detection does not work correctly #17530

Closed
Danstiv opened this issue Dec 15, 2024 · 6 comments · Fixed by #17532
Closed

Default output device detection does not work correctly #17530

Danstiv opened this issue Dec 15, 2024 · 6 comments · Fixed by #17532
Assignees
Labels
component/audio NVDA's audio output (nvWave, issues with usb audio etc). p2 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority release/blocking this issue blocks the milestone release triaged Has been triaged, issue is waiting for implementation.
Milestone

Comments

@Danstiv
Copy link
Contributor

Danstiv commented Dec 15, 2024

Steps to reproduce:

Required at least two output devices to test.

  1. Make the second device the default device in windows settings.
  2. Make the first device the default device in nvda settings.

Actual behavior:

Sound is outputted to the default device selected in windows settings.

Expected behavior:

Sound should be outputted to the device selected in nvda settings.

NVDA logs, crash dumps and other attachments:

I found out that this behavior is caused by the WasapiWavePlayer._isDefaultDevice method.
return name == next(_getOutputDevices())[1] always returns true if the first device is the default device in nvda, which causes player to switch to the default device in windows.
This is probably leftover code from WinMM, but I haven't dived into the specifics of nvda's audio implementation, so I'm not sure what the best way to fix this is.
@SaschaCowley, maybe you have some ideas.

System configuration

NVDA installed/portable/running from source:

Installed

NVDA version:

alpha-34782,82fefa5a (2025.1.0.34782)

Windows version:

Windows 11 23H2 (AMD64) build 22631.4602

Name and version of other software in use when reproducing the issue:

Windows control panel.

Other information about your system:

Other questions

Does the issue still occur after restarting your computer?

Yes

Have you tried any other versions of NVDA? If so, please report their behaviors.

2024.4 doesn't have this problem

If NVDA add-ons are disabled, is your problem still occurring?

Yes

Does the issue still occur after you run the COM Registration Fixing Tool in NVDA's tools menu?

Yes

@Adriani90
Copy link
Collaborator

Cc: @jcsteh

@OzancanKaratas
Copy link
Collaborator

No, the right person is @SaschaCowley. Because this issue has been occured after #17496 is merged. See the #17496 (comment).

@jcsteh
Copy link
Contributor

jcsteh commented Dec 16, 2024

Yeah, that code is definitely wrong now. It should just be a single line:

return name in (WAVE_MAPPER, cls.DEFAULT_DEVICE_KEY)

On a related note, given that we're removing other stuff in nvwave, is there any reason we haven't removed these?

  1. WAVE_MAPPER, in favour of DEFAULT_DEVICE_KEY.
  2. outputDeviceNameToID and outputDeviceIDToName, since WasapiWavePlayer never uses these and thus there's no point in callers using them either.
  3. The closeWhenIdle and buffered arguments to the WasapiWavePlayer constructor, which are unused now.
  4. WasapiWavePlayer itself, which can be renamed to WavePlayer.
  5. CALLBACK_NULL, CALLBACK_FUNCTION, CALLBACK_EVENT, waveOutProc and WOM_DONE, all of which are now unused and some of which are already commented out.

@jcsteh
Copy link
Contributor

jcsteh commented Dec 16, 2024

(And if we remove WAVE_MAPPER, can we get rid of _isDefaultDevice altogether and just compare with DEFAULT_DEVICE_KEY directly.)

@SaschaCowley
Copy link
Member

@jcsteh part of the reason some of these functions weren't removed is because they were used elsewhere in code. This has caused problems like #17516 and #17512. I was clearly over-optimistic in my assumption that whatever code was using these IDs would just cope with the move from winmm integer IDs to mmdevice's string IDs.

@SaschaCowley SaschaCowley self-assigned this Dec 16, 2024
@jcsteh
Copy link
Contributor

jcsteh commented Dec 16, 2024

Oh, sorry, I missed the sapi4 usage somehow when I was coming up with this list. Durp. But the id approach won't work there anyway, so once #17516 is fixed, we can probably remove these as described above.

@SaschaCowley SaschaCowley added p2 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority component/audio NVDA's audio output (nvWave, issues with usb audio etc). triaged Has been triaged, issue is waiting for implementation. release/blocking this issue blocks the milestone release labels Dec 16, 2024
@SaschaCowley SaschaCowley added this to the 2025.1 milestone Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/audio NVDA's audio output (nvWave, issues with usb audio etc). p2 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority release/blocking this issue blocks the milestone release triaged Has been triaged, issue is waiting for implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants