Skip to content

Conversation

gexgd0419
Copy link
Contributor

@gexgd0419 gexgd0419 commented Aug 12, 2025

Link to issue number:

Fixes #18674. Fixes #18676. Fixes #18760.

Summary of the issue:

NVDA becomes unresponsive for a small period of time when speaking some text in SAPI5 voices.

#18658 leaves more audio data for the EndStream handler to be played, which causes NVDA to be frozen when the EndStream handler is feeding the remaining audio chunks.

Description of user facing changes:

The above issue should be fixed.

Description of developer facing changes:

None

Description of development approach:

Moves the feeding logic in EndStream to the dedicated speak thread, avoiding blocking the SAPI5 thread that EndStream runs on.

The main thread no longer waits for cancellation to complete in order to prevent freezing.

The dedicated speak thread for WASAPI will be stopped and restarted when WASAPI is turned on or off, in order to prevent errors accessing the player when WASAPI is turned off and the player no longer exists.

Testing strategy:

Known issues with pull request:

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

@seanbudd seanbudd added this to the 2025.3 milestone Aug 13, 2025
@seanbudd seanbudd changed the base branch from master to beta August 13, 2025 05:10
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Aug 14, 2025
@gexgd0419
Copy link
Contributor Author

I moved WavePlayer.feed call to a dedicated thread to avoid blocking the main thread. In order to cancel the speech, WavePlayer.stop will be called on the main thread to break out of possible feed or idle call, then the main thread will wait for cancellation to finish.

Then there's a racing problem: if the main thread calls WavePlayer.stop before the speech thread calls WavePlayer.feed, but after the speech thread checks the cancellation flag when the flag hasn't been set, the stop call may miss the feed call, so the audio chunk will still be played while the main thread waits for the cancellation, causing delay.

As the speech thread itself can do nothing when calling feed or idle, calling stop in another thread is needed. But how should I prevent stop from missing the feed call that I want to cancel, aside from just calling stop again and again in a loop until cancellation completes?

Maybe I can use some kind of synchronization, but inside WavePlayer.stop and WavePlayer.feed, there's more code out of my control before the actual call to WASAPI is issued. Even if stop and feed is called at the same time, the actual IAudioClient::Stop may still come too early and miss the later feed.

@jcsteh I wonder if you have better idea about how to handle this. Also I wonder if it's a general problem that stop may be called before the next feed during cancellation, resulting in one more audio chunk being fed and played after stopping.

In this case, SAPI5 will send the final audio chunk flushed out of the SonicStream before the speech stream completes, and the final chunk might be big enough to become a noticeable delay.

@jcsteh
Copy link
Contributor

jcsteh commented Aug 19, 2025

I moved WavePlayer.feed call to a dedicated thread to avoid blocking the main thread.

Can you explain your threading model now? How many threads are they and what do they do at various points?

In order to cancel the speech, WavePlayer.stop will be called on the main thread to break out of possible feed or idle call, then the main thread will wait for cancellation to finish.

I can't stress this enough: you must not have the main thread wait for anything. The main thread should call WavePlayer.stop and return, no waiting. Any waiting in the main thread is going to cause problems in some way or another.

Then there's a racing problem: if the main thread calls WavePlayer.stop before the speech thread calls WavePlayer.feed, but after the speech thread checks the cancellation flag when the flag hasn't been set,

Ideally, you would set the cancellation flag in the main thread just before you call WavePlayer.stop. When the feeding thread picks up the cancellation, it should do whatever it needs to do to prevent any more chunks being fed, then clear the flag.

Also I wonder if it's a general problem that stop may be called before the next feed during cancellation, resulting in one more audio chunk being fed and played after stopping.

It isn't. When the feed call returns, synth drivers check whether cancellation has occurred and abandon any remaining chunks if so.

@jcsteh
Copy link
Contributor

jcsteh commented Aug 19, 2025

As an example, you can look at how this is handled in eSpeak. See the callback and stop functions.

@seanbudd
Copy link
Member

@gexgd0419 - do you think we can get this to a mergable state for monday next week (or sooner)? it seems we've got positive feedback from testers of the earlier commit

@gexgd0419
Copy link
Contributor Author

I think that the real problem here is that I got the duration calculation wrong, which was fixed in 0ec85e4, and I'm waiting for feedback from #18674. This caused the waiting time for the first chunk to be too long, so there's a delay when speaking longer text. But yes, the previous commit should fix the NVDA frozen problem.

I'll mark this ready when the rest of the work is done. Should I mark this PR to "fix 18674" before there's feedback from the user?

@seanbudd
Copy link
Member

Yep - for the PR documentations sake, you can say this PR closes the following issues: #18674 #18676 #18760, as long as we can confirm they do before this is merged.

@gexgd0419
Copy link
Contributor Author

Should synthDoneSpeaking be notified when the speech is stopped because it's cancelled? Should it always be notified or never be notified in this case, or it does not matter?

@seanbudd
Copy link
Member

No we use speechCanceled for that

@gexgd0419
Copy link
Contributor Author

If multiple utterances are queued, should synthDoneSpeaking be notified for each utterance, or after everything is done?

@seanbudd
Copy link
Member

I think we do it once everything is done

@gexgd0419 gexgd0419 marked this pull request as ready for review August 21, 2025 05:13
@gexgd0419 gexgd0419 requested a review from a team as a code owner August 21, 2025 05:13
@gexgd0419 gexgd0419 requested a review from seanbudd August 21, 2025 05:13
@gexgd0419 gexgd0419 marked this pull request as draft August 21, 2025 05:42
@gexgd0419
Copy link
Contributor Author

@seanbudd Does this need a changelog entry?

@seanbudd
Copy link
Member

Unless this fixes anything for 2025.2, no. If it's just fixing issues introduced in the 2025.3 release cycle there's no need for a change log

@gexgd0419 gexgd0419 marked this pull request as ready for review August 21, 2025 07:27
@seanbudd seanbudd requested a review from Copilot August 21, 2025 07:37
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes NVDA unresponsiveness issues with SAPI5 voices by addressing synchronization problems in the EndStream handler. The changes move audio processing logic to a dedicated thread to prevent blocking and improve the handling of WASAPI thread lifecycle.

  • Moves EndStream audio processing logic to the dedicated speak thread to prevent blocking
  • Removes main thread waiting for cancellation completion to prevent freezing
  • Improves WASAPI thread lifecycle management with proper start/stop handling

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks @gexgd0419

@seanbudd seanbudd merged commit eb2bcd8 into nvaccess:beta Aug 22, 2025
30 checks passed
@gexgd0419 gexgd0419 deleted the fix-sapi5 branch August 22, 2025 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants