-
-
Notifications
You must be signed in to change notification settings - Fork 738
Support for 32 bit sapi4 and sapi5 via a 32 bit synthDriver runtime #19432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: beta
Are you sure you want to change the base?
Conversation
…api' module, so that they can be imported separte to NvDAHelper.
…or at least also x86 with the 32 bit shim to access wasapi functions.
…ill be needed by the 32 bit shim. The x86 sonic dll will be placed in source/synthDrivers32.
…2 Job object, which allows automaticlaly killing any process associated with it. Will be required for the NVDA bridge.
…hDriver 32 bit shim process.
…ns synthDriverHost32Runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied directly from NVDA before sapi4 was removed, no changes made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied directly from NVDA before sapi4 was removed, no changes made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines copied directly from nvdaHelper/localLib.py, no changes made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this an API breaking change? i.e. will consumers need to change from using nvdaHelper to wasapi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a minimal static config to have sapi4 / 32 bit sapi5 to work. Really this should be proxied to NVDA.
There was a problem hiding this 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 adds support for 32-bit SAPI4 and SAPI5 synthesizers in 64-bit NVDA by implementing a 32-bit synth driver host runtime that runs in a separate 32-bit process. Communication between NVDA and the 32-bit process occurs via RPYC over standard pipes.
Changes:
- Implements a bridge architecture with services, proxies, and connections to support remote synth drivers
- Extracts WASAPI functions into a standalone module for use by both NVDA and the 32-bit runtime
- Adds job object support to ensure child processes are terminated when NVDA exits
Reviewed changes
Copilot reviewed 41 out of 47 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| source/winBindings/kernel32.py | Added Win32 API bindings for job objects and GetCurrentProcessId |
| source/winBindings/jobapi2.py | New module defining job object structures and constants |
| source/wasapi.py | Extracted WASAPI function definitions from NVDAHelper for standalone use |
| source/jobObject.py | New module providing Job class wrapper for Windows job objects |
| source/_bridge/* | New bridge infrastructure for client-server architecture |
| source/synthDrivers32/* | 32-bit SAPI4 and SAPI5 synth driver implementations |
| source/synthDrivers/sapi4_32.py | Proxy driver for 32-bit SAPI4 |
| source/synthDrivers/sapi5_32.py | Proxy driver for 32-bit SAPI5 |
| nvdaHelper/archBuild_sconscript | Build changes to compile nvdaHelperLocal and sonic for all architectures |
| runtime-builders/synthDriverHost32/* | Build setup for 32-bit runtime executable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…iver host runtime, rather than having them committed twice.
…r may not be there depending on how Proxy might be used as a minxin.
Co-authored-by: Copilot <[email protected]>
…ll is loaded by scons for compiling dictionaries etc.
| - name: Prepare source code | ||
| shell: cmd | ||
| run: scons source %sconsArgs% %sconsCores% | ||
| run: scons source synthDriverHost32Runtime %sconsArgs% %sconsCores% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be added to sconsArgs instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could, but I feel that isn't right semantically, as synthDriverHost32Runtime is a target (just like source) and not a flag or variable like the rest of the content in sconsArgs).
| @@ -0,0 +1,1260 @@ | |||
| # A part of NonVisual Desktop Access (NVDA) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are adding this back to the API, we need to update the changelog. My preference would be to keep this private if possible, so we can tear out support later if needed.
I think generally this PR needs to update the change log and user guide to undo #19290. Maybe we should revert #19290 in a separate PR first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I can open a pr to revert the sapi4 removal. Once merged then move sapi4.py and _sapi4.py from synthDrivers to synthDrivers32 (unchanged). And I guess I can also rename synthDrivers32 to _synthDrivers32 if we want to keep it private.
| @@ -0,0 +1 @@ | |||
| cpython-3.13.11-windows-x86-none No newline at end of file | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line at EOF
| cpython-3.13.11-windows-x86-none | |
| cpython-3.13.11-windows-x86-none | |
| "rpyc", | ||
| "configobj", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we pin versions here?
| _conn: rpyc.Connection | None | ||
| _localService: LocalService_t | None | ||
|
|
||
| def __init__(self, stream, localService: LocalService_t = None, name: str = "unknown"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can stream get a type hint?
| nvdaSourceDir = "../../source" | ||
| runtimeSourceDir = "../../source/_bridge/runtimes/synthDriverHost" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use os.path.join here?
| from py2exe.dllfinder import DllFinder # noqa: E402 | ||
|
|
||
| RT_MANIFEST = 24 | ||
| manifestTemplateFilePath = f"{nvdaSourceDir}/manifest.template.xml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use os.path.join here?
| "description": "NVDA Add-on Runtime", | ||
| "product_name": "NVDA ART", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still want to use these names for the 32bit shim?
| # #3368: bisect was implicitly included with Python 2.7.3, but isn't with 2.7.5. | ||
| "bisect", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need to include bisect? I think this was done for the benefit of add-on authors primarily for our main setup.py script
…it to the 32 bit runtime (#19436) ### Link to issue number: <!-- Use Closes/Fixes/Resolves #xxx to link this PR to the issue it is responding to. --> Reverts #19290 ### Summary of the issue: with the move to 64 bit, sapi4 became unusable as it only functions in a 32 bit process. PR #19290 was merged which removes sapi4. However, pr #19432 is adding the infrastructure to run sapi4 within a 32 bit runtime. Thus the original sapi4 should be tempoararily added back so that pr #19432 can move it into its 32 bit location. ### Description of user facing changes: None. ### Description of developer facing changes: ### Description of development approach: This is a clean revert of pr #19290 (with only a conflict in changes.md which was addressed). ### Testing strategy: None - clean revert. ### Known issues with pull request: None known. ### Code Review Checklist: <!-- This checklist is a reminder of things commonly forgotten in a new PR. Authors, please do a self-review of this pull-request. Check items to confirm you have thought about the relevance of the item. Where items are missing (eg unit / system tests), please explain in the PR. To check an item `- [ ]` becomes `- [x]`, note spacing. You can also check the checkboxes after the PR is created. A detailed explanation of this checklist is available here: https://github.com/nvaccess/nvda/blob/master/projectDocs/dev/githubPullRequestTemplateExplanationAndExamples.md#code-review-checklist --> - [x] Documentation: - Change log entry - User Documentation - Developer / Technical Documentation - Context sensitive help for GUI changes - [x] Testing: - Unit tests - System (end to end) tests - Manual testing - [x] UX of all users considered: - Speech - Braille - Low Vision - Different web browsers - Localization in other languages / culture than English - [x] API is compatible with existing add-ons. - [x] Security precautions taken. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…i4_32' which means it will continu to be used if sapi4 was in use in 2025 (32 bit NVDA). Similarly, standard sapi5 is now called sapi5_64, and its Description coearly notes 64 bit, and sapi5 (32 bit proxy) is now called 'sapi5', which means that if sapi5 was in use in 2025, 32 bit sapi5 will continue to be used, as most 3rd party sapi5 voices that were in use were only 32 bit and would stop working otherwise.
source/synthDrivers/sapi5.py
Outdated
| name = "sapi5" | ||
| description = "Microsoft Speech API version 5" | ||
| name = "sapi5_64" | ||
| description = "Microsoft Speech API version 5 (64 bit)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this string translatable?
source/synthDrivers/sapi5_32.py
Outdated
|
|
||
| class SynthDriver(SynthDriverProxy32): | ||
| name = "sapi5" | ||
| description = "Microsoft Speech API version 5 (32 bit)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this string translatable?
|
Tested with nvda_snapshot_pr19432-4091,1d096220.exe on Microsoft Windows [Version 10.0.26200.7462], NVDA failed to find or load the sapi4 and sapi5_64 speech synthesizer drivers. Below is the log: |
…driver names, as required by NvDA when loading from config.
Link to issue number:
Replaces pr #19412
Summary of the issue:
With the switch to 64 bit, NVDA no longer supports sapi4, or 32 bit specific sapi5 synthesizers. NVDA should somehow continue to support these synthesizers.
Description of user facing changes:
NvDA continues to support sapi4 and 32 bit specific Sapi5 synthesizers.
Description of developer facing changes:
Description of development approach:
Compile nvdaHelperLocal for all archetectures, rather than just for NVDA's core archetecture (x64). NVDA's 32 bit synthDriver host runtime needs nvdaHelperLocal for WASAPI.
Move the wasapi function definitions for nvdaHelperLocal.dll into their own 'wasapi' Python module, so that they can be imported separate to NvDAHelper. NVDA's 32 bit synthDriver host runtime uses WASAPI directly.
Compile sonic independently of eSpeak and for all archetectures (not just x64), as it is needed by sapi5 when running in NVDA's 32 bit synthDriver host runtime. The x86 sonic dll will be placed in source/synthDrivers32.
winBindings.kernel32: add definition for GetCurrentProcessId. Used in logging for NVDA's 32 bit synthDriver host runtime.
Add a jobObject module, which contains a Job class which wraps a win32 Job object, which allows automatically killing any process associated with it. This is sued to ensure that any process started for the 32 bit synthDriver host runtime is killed off if NvDA exits.
Add a _bridge package, which provides a client and runtime for the 32 bit synthDriver host. The client is code that runs in NVDA, and the runtime is code that runs in its own 32 bit child process. Communication between NvDA and the child process is via RPYC over the child processs's standard pipe handles. This _bridge package can be later extended with more clients and runtimes as we start to implement a full ART.
Add 32 bit sapi4 and sapi5 synthDrivers which use the synthDriverHost32 runtime.
Testing strategy:
Manually tested running NvDA with both 32 bit sapi4 (Truevoice and MS Mike Mary Sam) and 32 bit sapi5 (Windows built-in David, eSpeak bridge, Mikropuhe). Ran with NvDA launcher, installed normal user, and on secure desktop (UAC and logon screen).
Known issues with pull request:
Code Review Checklist: