Skip to content

Make SAPI4 voices use WASAPI#17718

Merged
seanbudd merged 23 commits into
nvaccess:masterfrom
gexgd0419:sapi4-wasapi
Feb 25, 2025
Merged

Make SAPI4 voices use WASAPI#17718
seanbudd merged 23 commits into
nvaccess:masterfrom
gexgd0419:sapi4-wasapi

Conversation

@gexgd0419

@gexgd0419 gexgd0419 commented Feb 21, 2025

Copy link
Copy Markdown
Contributor

Link to issue number:

None

Summary of the issue:

This PR makes the built-in SAPI4 synthesizer use WASAPI to output audio, so that old code related to WinMM can be removed entirely.

Description of user facing changes

SAPI4 voices should work as usual.

Features supported by WavePlayer, such as audio ducking, leading silence trimming, and keeping audio device awake (#17571) will be able to work with SAPI4 voices.

Description of development approach

Create a class to implement IAudio and IAudioDest, so that it can be used as an audio output destination to replace the SAPI4 built-in MMAudioDest which uses WinMM.

SAPI4 performs audio data output on the main thread. SAPI4 expects audio data writes to be a non-blocking operation, and it should return AUDERR_NOTENOUGHDATA when the buffer is full. Unfortunately, that's not how WavePlayer works, and WavePlayer.feed blocks the current thread until there's enough space in the buffer. So a dedicated thread is created to feed data to WavePlayer, and audio data from SAPI4 will be put in a queue first to prevent blocking the thread. Bookmarks from SAPI4 will be put in the same queue.

WavePlayer.feed returns before the audio is finished playing, but WavePlayer can only check and invoke callback functions when WavePlayer.feed or WavePlayer.sync is called. If we just keep on waiting for the next audio chunk in the queue, WavePlayer will have no chance to call the callback functions when there is no chunk. So WavePlayer.feed should be called periodically, regardless of whether there's audio or bookmark in the queue.

Testing strategy:

Further tests with different SAPI4 synthesizers are needed.

Known issues with pull request:

Some SAPI4 voices do not support custom audio destinations, and this is allowed by the SAPI4 spec. They choose their own way to output the audio, and therefore bypass the WASAPI WavePlayer. In fact, they also don't use MMAudioDest before this PR. This means that:

  • WASAPI-related features, such as audio ducking, still cannot work with them.
  • They do not follow the audio output device setting in NVDA's settings dialog, and will always output audio on the device they select.

Such voices have TTSFEATURE_FIXEDAUDIO in the feature flag.

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

@SaschaCowley SaschaCowley added the merge-early Merge Early in a developer cycle label Feb 21, 2025
@AppVeyorBot

Copy link
Copy Markdown
  • PASS: Translation comments check.
  • PASS: License check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 8fc0fd4c63

@gexgd0419 gexgd0419 marked this pull request as ready for review February 22, 2025 06:54
@gexgd0419 gexgd0419 requested a review from a team as a code owner February 22, 2025 06:54
@cary-rowen

Copy link
Copy Markdown
Contributor

This is a great work, and if we can merge this in the 2025.1 dev cycle, we don't have to announce to the community that Sapi4 will be deprecated, thus avoiding possible confusion.

@gexgd0419 gexgd0419 marked this pull request as draft February 23, 2025 14:29
@gexgd0419 gexgd0419 marked this pull request as ready for review February 23, 2025 16:43
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Feb 24, 2025

@seanbudd seanbudd 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 @gexgd0419! We recently decided to extend the life of SAPI 4 for the immediate future anyway, but this change is still much appreciated

Comment thread source/synthDrivers/_sapi4.py Outdated
Comment thread source/synthDrivers/sapi4.py Outdated
Comment thread source/synthDrivers/sapi4.py Outdated
Comment thread source/synthDrivers/sapi4.py
Comment thread source/synthDrivers/sapi4.py Outdated
Comment thread source/synthDrivers/sapi4.py Outdated
Comment thread source/synthDrivers/sapi4.py
Comment thread source/synthDrivers/sapi4.py
Comment thread source/synthDrivers/sapi4.py Outdated
Comment thread source/synthDrivers/sapi4.py Outdated
@gexgd0419 gexgd0419 marked this pull request as draft February 25, 2025 02:13
@seanbudd

Copy link
Copy Markdown
Member

Could you also remove any mention of the deprecation of SAPI4 including the warning message?

@seanbudd seanbudd added this to the 2025.1 milestone Feb 25, 2025
@gexgd0419 gexgd0419 marked this pull request as ready for review February 25, 2025 04:16
@gexgd0419 gexgd0419 requested a review from a team as a code owner February 25, 2025 04:16
Comment thread projectDocs/design/synthesizers.md Outdated
Comment thread source/synthDrivers/sapi4.py
Comment thread source/synthDrivers/sapi4.py Outdated
Comment thread source/synthDrivers/sapi4.py
Comment thread source/synthDrivers/sapi4.py Outdated

@seanbudd seanbudd 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 @gexgd0419

@seanbudd seanbudd added the release/blocking this issue blocks the milestone release label Feb 25, 2025

@Qchristensen Qchristensen 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.

Great work and will be appreciated by SAPI 4 users.

@seanbudd seanbudd merged commit 5402cd6 into nvaccess:master Feb 25, 2025
@gexgd0419 gexgd0419 deleted the sapi4-wasapi branch February 25, 2025 05:40
@hozosch

hozosch commented Feb 28, 2025

Copy link
Copy Markdown

Currently, there's the following Issue in Alpha versions. I don't know if it's known yet, because it persists in the last few versions already, so I'm pointing it out here.
Traceback (most recent call last):
File "comtypes_comobject.pyc", line 176, in call_without_this
File "synthDrivers\sapi4.pyc", line 220, in IAudio_LevelSet
AttributeError: 'NoneType' object has no attribute 'setVolume'
I think this is what causes sapi4 to not work at all. At least, that's what happens on my end.

@cary-rowen

Copy link
Copy Markdown
Contributor

@hozosch
Can you open an issue separately?

@hozosch

hozosch commented Mar 1, 2025

Copy link
Copy Markdown

Yes, but I didn't know if that was such a good idea at first. I didn't want too many unnecessary issues for one topic. But if you so wish, I'll do it.

@gexgd0419 gexgd0419 mentioned this pull request Mar 4, 2025
5 tasks
SaschaCowley pushed a commit that referenced this pull request Mar 6, 2025
Summary of the issue:
In #17718, I changed the SAPI4 synthesizer code, so that SAPI4 can now use WASAPI WavePlayer for audio output. This was done by creating a custom audio output destination class, and let the SAPI4 engines use the custom destination instead of the built-in `MMAudioDest` to output audio.

The engines interact with the audio destination object directly, and each engine may have its own way to use the audio destination object. So while the current implementation works with some of the SAPI4 engines, it may not work well with some other engines. As different engines have different behaviors, it may be impossible to replicate the same issue with another voice.

It would be better if I can get access to the exact same voice and study its behavior. But many of those voices are commercial products, which require purchasing and activating, so I cannot easily get the voices.

The second option is to get the log file from the user and study the logs. While I am using logs to debug on my side, the logs may be too chatty for regular users, because every audio chunk would be logged, so I decided to leave the logging part out. But now I find it difficult to diagnose SAPI4-related problems without logs when I can't get access to the voice.

Description of user facing changes
When the debug log category `synthDriver` in the advanced settings page is enabled, additional verbose SAPI4 logs will be enabled, which includes most of the interactions between the SAPI4 engine and the custom audio destination object, including every audio data write.

This can log many lines when using SAPI4 voices, so it's recommended to keep the log disabled when not necessary.

Description of development approach
Added logging in the SAPI4 module, which can be enabled or disabled by the `synthDriver` debug log category.

Testing strategy:
Tested manually.
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. merge-early Merge Early in a developer cycle release/blocking this issue blocks the milestone release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants