Make SAPI5 & MSSP voices use WavePlayer (WASAPI)

[gexgd0419]

Link to issue number:

#13284

Summary of the issue:

Currently, SAPI5 and MSSP voices use their own audio output mechanisms, instead of using the WavePlayer (WASAPI) inside NVDA.

This may make them less responsive compared to eSpeak and OneCore voices, which are using the WavePlayer, or compared to other screen readers using SAPI5 voices, according to my test result.

This also gives NVDA less control of audio output. For example, audio ducking logic inside WavePlayer cannot be applied to SAPI5 voices, so additional code is required to compensate for this.

Description of user facing changes

SAPI5 and MSSP voices will be changed to use the WavePlayer, which may make them more responsive (have less delay).

According to my test result, this can reduce the delay by at least 50ms.

This haven't trimmed the leading silence yet. If we do that also, we can expect the delay to be even less.

Description of development approach

Instead of setting self.tts.audioOutput to a real output device, do the following:

• create an implementation class SynthDriverAudioStream to implement COM interface IStream, which can be used to stream in audio data from the voices.
• Use an SpCustomStream object to wrap SynthDriverAudioStream and provide the wave format.
• Assign the SpCustomStream object to self.tts.AudioOutputStream, so SAPI will output audio to this stream instead.

Each time an audio chunk needs to be streamed in, ISequentialStream_RemoteWrite will be called, and we just feed the audio to the player. IStream_RemoteSeek can also be called when SAPI wants to know the current byte position of the stream (dlibMove should be zero and dwOrigin should be STREAM_SEEK_CUR in this case), but it is not used to actually "seek" to a new position. IStream_Commit can be called by MSSP voices to "flush" the audio data, where we do nothing. Other methods are left unimplemented, as they are not used when acting as an audio output stream.

Previously, comtypes.client.GetEvents was used to get the event notifications. But those notifications will be routed to the main thread via the main message loop. According to the documentation of ISpNotifySource:

Note that both variations of callbacks as well as the window message notification require a window message pump to run on the thread that initialized the notification source. Callback will only be called as the result of window message processing, and will always be called on the same thread that initialized the notify source. However, using Win32 events for SAPI event notification does not require a window message pump.

Because the audio data is generated and sent via IStream on a dedicated thread, receiving events on the main thread can make synchronizing events and audio difficult.

So here SapiSink is changed to become an implementation of ISpNotifySink. Notifications received via ISpNotifySink are "free-threaded", sent on the original thread instead of being routed to the main thread.

• To connect the sink, use ISpNotifySource::SetNotifySink.
• To get the actual event that triggers the notification, use ISpEventSource::GetEvents. Events can contain pointers to objects or memory, so they need to be freed manually.

Finally, all audio ducking related code are removed. Now WavePlayer should be able to handle audio ducking when using SAPI5 and MSSP voices.

There should be no change to public APIs.

Testing strategy:

Tested the delay of some built-in SAPI5 voices.

Audio ducking seemed to be working.

Stability of this is not proven yet, which needs further tests.

Known issues with pull request:

None yet

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

[LeonarddeR]

This is marvelous work! Great job

[LeonarddeR]

Strictly spoken, this is breaking backwards compatibility. That said, I think it is safe to mark this as such in the changelog.

[gexgd0419]

I think that the implementation of SapiSink should be internal and not considered as part of the public API, so it shouldn't be a breaking change. I couldn't find references to SapiSink outside sapi5.py in NVDA's code.

Also, the function parameter list of StartStream, Bookmark and EndStream is mostly the same.

But it should be noted that event notification can be sent on any thread, instead of always on the main thread, which is also the reason why I chose to use ISpNotifySink directly.

Do I need to mention this in the "Changes for Developers" section?

[seanbudd]

yes, any removed public code (not prefixed with _), and other breaking API changes must be listed in API breaking changes under changes for developers. It doesn't seem like the API for SapiSink (e.g functions you listed) has changed, can you confirm?

[gexgd0419]

I removed the if synth is None checks in them, because the events are now dispatched in ISpNotifySink_Notify and the check has been done there. The usage of SapiSink is also changed, although the parameter lists of those functions are the same.

[seanbudd]

Thanks @gexgd0419 ! Mostly minor review items

[seanbudd]

yes, any removed public code (not prefixed with _), and other breaking API changes must be listed in API breaking changes under changes for developers. It doesn't seem like the API for SapiSink (e.g functions you listed) has changed, can you confirm?

[seanbudd]

please add type information. Can more descriptive variable names be used?

[gexgd0419]

Those names are from the original interface definition from Microsoft, which use the Hungarian naming convention. For example, "cb" stands for "count of bytes". But yes, I can make it more descriptive.

[gexgd0419]

pv is of type POINTER(c_ubyte), but this is not accepted as a type hint because POINTER is a function. Can I use c_void_p instead?

[seanbudd]

I think we would typically use c_ubyte here

[gexgd0419]

But pv is a pointer, not a single byte. And its type is neither c_ubyte nor c_void_p, if you use isinstance to test it.

[LeonarddeR]

I think I prefer the parameter names to resemble the interface definition as much as possible. The params can be described in a doc string.

Re typing, what should probably work, is defining a type variable at the top of the file and then use that in your annotation. So for example, somewhere at the top:

LP_c_ubyte = POINTER(c_ubyte)  # This follows comtypes naming convention for pointer type names

then:

Suggested change

	def ISequentialStream_RemoteWrite(self, pv, cb):
	def ISequentialStream_RemoteWrite(self, pv: LP_c_ubyte, cb: typename):

That said, there's some comtypes magic under the hoot, especially when using the high level implementation. Have you ever logged the type of pv after it entered the function?

[gexgd0419]

The type of pv is comtypes.gen._1EA4DBF0_3C3B_11CF_810C_00AA00389B71_0_1_1.LP_c_ubyte.

And the type of cb is int, although defined as c_ulong.

[gexgd0419]

isinstance(pv, POINTER(c_ubyte)) returns True.

So I think it's safe to assume that they are the same type.

[gexgd0419]

Re typing, what should probably work, is defining a type variable at the top of the file and then use that in your annotation.

Pylance still tells me that variables are not allowed in a type expression. Guess that the type has to be "static".

[LeonarddeR]

I wonder whether you could trick it with:
class LP_c_ubyte(POINTER(c_ubyte)): ...