Skip to content

WasapiPlayer: Call IAudioClient::Reset in completeStop() rather than in stop().#18434

Merged
SaschaCowley merged 3 commits into
nvaccess:betafrom
jcsteh:resetInFeeder
Jul 14, 2025
Merged

WasapiPlayer: Call IAudioClient::Reset in completeStop() rather than in stop().#18434
SaschaCowley merged 3 commits into
nvaccess:betafrom
jcsteh:resetInFeeder

Conversation

@jcsteh

@jcsteh jcsteh commented Jul 9, 2025 •
edited by SaschaCowley
Loading

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #18101
May address #17131, though I'm not sure and I think there's still a thread safety bug in that third party synth driver even if it does.

Summary of the issue:

Sometimes, speech is silent on the local computer when using NVDA Remote Access and a UAC screen appears on the remote computer. This occurs because NVDA tries to play a sound, which calls WavePlayer.stop, which throws an AUDCLNT_E_BUFFER_OPERATION_PENDING exception. That exception isn't caught, so it prevents speech handlers from being registered.

Description of user facing changes:

When using NVDA Remote Access, speech from User Account Control screens on the remote computer now works reliably.

Description of development approach:

AUDCLNT_E_BUFFER_OPERATION_PENDING is being returned from IAudioClient::Reset() called by WasapiPlayer::stop(). Per the documentation, Reset can return AUDCLNT_E_BUFFER_OPERATION_PENDING if "The client is currently writing to or reading from the buffer."

It's indeed slightly possible that the feeder thread is still in WasapiPlayer::feed() when we call WasapiPlayer::stop() on the main thread. This is why stopping is split into two phases: the stop() call itself, generally called on the main thread, and completeStop(), called by the feeder thread when it sees that stop() has been called.

Thus, we fix this problem by moving the IAudioClient::Reset() call from WasapiPlayer::stop() to WasapiPlayer::completeStop(), ensuring that it is called on the feeder thread and therefore not while the buffer is being written to. Audio still stops immediately when WasapiPlayer::stop() is called due to the call to IAudioClient::Stop(), which does not care about whether another thread is writing to the buffer.

We also call IAudioClient::Stop() before setting playState. Otherwise, the feeder thread might see the playState change and call client->Reset() before client->Stop() runs, causing AUDCLNT_E_NOT_STOPPED.

Testing strategy:

Followed steps in #18101. Verified the expected result 30 times.

Known issues with pull request:

None.

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.

@coderabbitai summary

jcsteh
jcsteh commented Jul 9, 2025
View reviewed changes
Comment thread nvdaHelper/local/wasapi.cpp Outdated
@michaelDCurran

michaelDCurran commented Jul 10, 2025

Copy link
Copy Markdown
Member

From a user point of view, the try build with LOG_DEBUGWARNING is working fine. Even when reset fails.
On the remote machine I here:

  • The connection sound
  • The connection message
  • The user account control dialog
    On the local machine, I here:
  • The user account control dialog.

Here is a log fragment showing the reset error:

INFO - _remoteClient.client.onConnectedAsFollower (13:35:36.021) - MainThread (21220):
Control connector connected
INFO - _remoteClient.session.RemoteSession.handleClientConnected (13:35:36.032) - MainThread (21220):
Client connected: {'id': 34, 'connection_type': 'master'}
DEBUGWARNING - NVDAHelperLocal (13:35:36.032) - nvwave.playWaveFile(controlled.wav) (20456):
Thread 20456, build\x86\local\wasapi.cpp, WasapiPlayer::completeStop, 613:
Couldn't reset stream: -2004287483

Error 0x88890005 | AUDCLNT_E_NOT_STOPPED

@seanbudd seanbudd added this to the 2025.2 milestone Jul 10, 2025
@SaschaCowley

SaschaCowley commented Jul 11, 2025

Copy link
Copy Markdown
Member

@michaelDCurran I think what is happening is that on one thread, playState is being set to PlayState::stopping then IAudioClient::Stop is being called, but on the background thread, feed is being called before the IAudioClient::Stop call completes, so IAudioClient::Reset is happening before ``Stop. @jcsteh is there a reason we set playState` to `stopping` before we call `IAudioClient::Stop`?

@jcsteh

jcsteh commented Jul 11, 2025

Copy link
Copy Markdown
Contributor Author

Nah, just my incompetence. Sorry. I'll fix it.

@SaschaCowley

SaschaCowley commented Jul 11, 2025

Copy link
Copy Markdown
Member

Please don't beat yourself up. It does no-one any good.

@jcsteh

jcsteh commented Jul 11, 2025

Copy link
Copy Markdown
Contributor Author

Thread safety is such fun, innit?

@jcsteh

jcsteh commented Jul 11, 2025

Copy link
Copy Markdown
Contributor Author

I'll push another try build and verify this still fixes the remote problem just in case. If so, we can mark this as ready for review.

@AppVeyorBot

AppVeyorBot commented Jul 11, 2025

Copy link
Copy Markdown

See test results for failed build of commit 8a335bfc0b

@SaschaCowley

SaschaCowley commented Jul 11, 2025

Copy link
Copy Markdown
Member

@jcsteh or @michaelDCurran have you had a chance to test the latest try build yet? I have been unable to reduce the bug regardless of NVDA version

SaschaCowley
SaschaCowley reviewed Jul 11, 2025
View reviewed changes
Comment thread nvdaHelper/local/wasapi.cpp
@jcsteh

jcsteh commented Jul 11, 2025

Copy link
Copy Markdown
Contributor Author

I tested the latest build and can no longer reproduce the remote problem. I'll mark this as ready for review.

@jcsteh jcsteh marked this pull request as ready for review July 11, 2025 07:00
@jcsteh jcsteh requested a review from a team as a code owner July 11, 2025 07:00
@jcsteh jcsteh requested a review from seanbudd July 11, 2025 07:00
@michaelDCurran

michaelDCurran commented Jul 13, 2025

Copy link
Copy Markdown
Member

I've been running the latest try build remotely for quite some time, successfully bringing up UAC dialogs remotely with out issue. I think it is good to go.

SaschaCowley
SaschaCowley approved these changes Jul 14, 2025
View reviewed changes

@SaschaCowley SaschaCowley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, @jcsteh

@SaschaCowley SaschaCowley linked an issue Jul 14, 2025 that may be closed by this pull request
Remote: Sometimes no speech from UAC screen on remote computer #18101
Closed
@SaschaCowley SaschaCowley merged commit 7437c80 into nvaccess:beta Jul 14, 2025
43 of 45 checks passed
@jcsteh jcsteh deleted the resetInFeeder branch July 21, 2025 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelDCurran michaelDCurran michaelDCurran left review comments

@SaschaCowley SaschaCowley SaschaCowley approved these changes

@seanbudd seanbudd Awaiting requested review from seanbudd seanbudd is a code owner automatically assigned from nvaccess/developers

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2025.2

Development

Successfully merging this pull request may close these issues.

Remote: Sometimes no speech from UAC screen on remote computer

5 participants

@jcsteh @michaelDCurran @SaschaCowley @AppVeyorBot @seanbudd