Fix SAPI4#18262
Conversation
|
@gexgd0419 Please keep in mind that whatever you fix with those voices can potentially break others. Sapi4 has turned out to be kind of a minefield. |
|
This is yet another issue I could not reproduce (in a Windows 10 virtual machine). But as the user could reproduce with a fresh copy, and 2024.4 did not have this issue, I am trying to gradually revert to the previous implementation to see what went wrong. |
|
I'm starting to think that maybe it's better to isolate the SAPI4 voices in a separate process where WinMM APIs are hooked, and send the audio data back via IPC channel. Although hooking every WinMM API may not be easy, this can keep the old SAPI4-related code intact and (maybe) reduce compatibility-related problems. As NVDA will be migrated to 64-bit, an IPC approach will eventually be used. Is there any plan or direction about what kind of IPC approach will be used in 64-bit version of NVDA? |
|
Can you confirm if this affects #17964 |
|
Given the potentially positive impact of this change, why was this moved from beta to master again? |
|
@LeonarddeR - it is a risky change (see merge early label). We want this thoroughly tested so it doesn't delay the 2025.2 release. The current state of WASAPI 4 is quite stable compared to SAPI 5, with only 2 known synthesizer issues. We don't want to risk breaking other synthesizers this late in a beta cycle. |
Link to issue number:
Fixes #18259.
Summary of the issue:
Digalo 2000 voices (SAPI4) do not work and cause memory access violation errors.
Description of user facing changes:
The issue should be fixed.
Description of developer facing changes:
None.
Description of development approach:
For some reason, the IAudio object was destroyed before it was even being used, causing its COM pointers to be reset to zero and access violations afterwards. So code are added to guard the SAPI4 IAudio object from being released prematurely.
Prevent joining the audio thread if the audio object is destroyed before starting.
Testing strategy:
Users reported that this works on their system.
Known issues with pull request:
SynthDriverAudio was released too many timesmight still be logged when working correctly.Code Review Checklist:
@coderabbitai summary