Convert NVDAHelper LocalWin10 to C++/WinRT by LeonarddeR · Pull Request #11768 · nvaccess/nvda

LeonarddeR · 2020-10-17T07:45:27Z

Link to issue number:

Summary of the issue:

Interaction with the Windows 10 UWP/OneCore APIs was performed using C++/CX code. This is an extension to the C++ language that which dialect is sometimes a bit different from standard C++.

C++/CX is superseded by C++/WinRT. which is a C++20 language projection. Using this results in code that is IMO much more readable to people who know C++ but are not accustomed to the CX dialect. It also uses C++ coroutines instead of the Parallel Patterns Library. Not to mention that CX is likely to be deprecated in the future.

Description of how this pull request fixes the issue:

This pr converts all C++/CX and WRL code to its equivalent in C++/WinRT. While at it, I converted the OcSpeech and UwpOcr structs to classes that hold the speech synthesis and ocr engines as private members, ensuring that the exported functions could only access the things they need.

Testing performed:

Tested both OCR and OneCore on a local system on Windows 10 19H1, 19H2 and 20H1. All seemed to work fine. See below for checklist

Known issues with pull request:

None known

Change log entry:

Changes for developers
- Moved the UWP/OneCore interaction layer of NVDAHelper from C++/CX to C++/Winrt. (NVDAHelper Windows 10, switch from WRL/CXX to C++/WinRT #10662)

Code Review Checklist:

josephsl · 2020-10-17T07:58:28Z

CC @jcsteh, may need to test minimum Windows 10 build supported (hopefully 17134 or later). Thanks.

LeonarddeR · 2020-10-17T08:07:30Z

17134 should be supported, but I'm pretty sure that even older builds should work. I will test it against Server 2016 next week.

Brian1Gaff · 2020-10-17T10:16:23Z

Would any of this upset any routines used in Windows 7 or8.1?

dpy013 · 2020-10-17T10:42:28Z

Tested with nvda_snapshot_pr11768-21228 on Microsoft Windows [version 10.0.19041.572] in Simplified Chinese and no error messages appear.

LeonarddeR · 2020-10-17T11:17:47Z

Would any of this upset any routines used in Windows 7 or8.1

No. Nothing is changed for these versions of Windows.

feerrenrut · 2020-10-19T10:16:48Z

Generally I like the look of this change, but I'd like to target the 2021.1 release (and merge early for maximum testing). I'd like to mark it as a draft while we make a decision about making onecore a separate dll.

jcsteh · 2020-10-19T11:23:11Z

I'd like to mark it as a draft while we make a decision about making onecore a separate dll.

Note that nvdaHelperLocalWin10 (which includes Win 10 OCR and OneCore speech) is already a separate dll. It previously had a separate SCons environment which allowed compiler flags to be tweaked, but this PR removes that. If you're worried about coroutine support being enabled everywhere, I'd suggest reinstating that separate SCons environment and moving the /await flag to that.

Note that given the highly asynchronous nature of WinRT in general, I think most things in nvdaHelperLocalWin10 are probably going to need await sooner or later, so I think it's reasonable to enable this for the whole of nvdaHelperLocalWin10 (but not necessarily other NVDA dlls).

All of that said, I can think of other places in nvdaHelper where coroutines could be useful, but it's certainly a riskier change that way.

AppVeyorBot · 2020-10-19T15:11:55Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit 3f58ed2f3b

feerrenrut · 2020-10-19T15:43:10Z

 env.Append(LINKFLAGS='/OPT:REF') #having symbols usually turns this off but we have no need for unused symbols

+win10env = env.Clone()
+# Add C++ co-routine support


Please add "for winrt usage" or something more specific. I haven't yet done a thorough review of this so I'm not sure I can make a precise suggestion yet. Generally, if I encountered this while trying to split up the code in this DLL or merge with another, or perhaps copy the approach for a new DLL, then I'd want to know which code / features depend on this flag and why it is necessary.

lukaszgo1 · 2020-12-05T00:15:49Z

Hi @LeonarddeR
I've tried a build from this PR on my Windows 10 1511 box, and unfortunately OneCore driver fails to load with the following, not very useful IMHO, log:

IO - inputCore.InputManager.executeGesture (01:00:22.585) - winInputHook (956):
Input: kb(desktop):enter
DEBUG - autoSettingsUtils.autoSettings.AutoSettings._registerConfigSaveAction (01:00:22.602) - MainThread (4764):
registering pre_configSave action: <class 'synthDrivers.oneCore.SynthDriver'>
DEBUGWARNING - NVDAHelperLocal (01:00:22.604) - MainThread (4764):
Thread 4764, build\x86\localWin10\oneCoreSpeech.cpp, OcSpeech::OcSpeech, 43:
AppendedSilence not supported

ERROR - synthDriverHandler.setSynth (01:00:22.604) - MainThread (4764):
setSynth
Traceback (most recent call last):
  File "synthDriverHandler.pyc", line 448, in setSynth
  File "synthDriverHandler.pyc", line 420, in getSynthInstance
  File "synthDriverHandler.pyc", line 303, in initSettings
  File "synthDrivers\oneCore.pyc", line 266, in _get_rate
OSError: exception: access violation reading 0x00000000

While W10 1511 is quite old, unsupported and therefore not very important the fact that this code fails there suggests to me that it would also fail on 1507 LTSB which is still supported until October 2025 so I believe we need to support it too.

In general I'd like to push back a little on this. Having more readable code is of course important but since it brings no user visible benefits merging this PR seems quite risky to me. We had similar discussion in the past on #10405 which quite similarly to this one just modernized syntax without user visible changes and all parties involved there decided that it should be closed.
IMHO it makes sense to delay reviewing/merging this until one of the following happens:

Microsoft announces that C++/CX is indeed going to be deprecated - for now when they intent to do this is anyone's guess.
Some sort of a bug is found in the localWin10 code which would require a significant rewrite to fix anyway.

BTW Were you able to test this on Server 2016?

LeonarddeR · 2020-12-08T18:08:48Z

In general I'd like to push back a little on this. Having more readable code is of course important but since it brings no user visible benefits merging this PR seems quite risky to me.

If properly tested, I'm not sure why this code introduces risk. We just will have to make sure that we test all of it in the full range of still supported Windows 10 builds.

We had similar discussion in the past on #10405 which quite similarly to this one just modernized syntax without user visible changes and all parties involved there decided that it should be closed.

There's a huge difference between Python and C++ in that C++ compiles all code before running it, which tackles most of the risk IMO. Changing Python code has a way bigger risk of experiencing regressions at run time.

IMHO it makes sense to delay reviewing/merging this until one of the following happens:
* Microsoft announces that C++/CX is indeed going to be deprecated - for now when they intent to do this is anyone's guess.

True. However, they already highly recomment C++/WinRT at this point.

I'd like to add that coroutine suppot in C++/WinRT is currently based on the experimental support in C++17. The current WinRT bindings in the Windows 10 SDK don't support C++20 style coroutines. We could consider this another reason to wait for this to be merged.

lukaszgo1 · 2020-12-08T21:16:12Z

Unfortunately with latest build from this PR Onecore driver still crashes on Windows 10 1511. Log is as follows:

DEBUG - autoSettingsUtils.autoSettings.AutoSettings._registerConfigSaveAction (20:56:04.880) - MainThread (1812):
registering pre_configSave action: <class 'synthDrivers.oneCore.SynthDriver'>
DEBUGWARNING - NVDAHelperLocal (20:56:04.892) - MainThread (1812):
Thread 1812, build\x86\localWin10\oneCoreSpeech.cpp, OcSpeech::OcSpeech, 46:
AppendedSilence not supported

ERROR - synthDriverHandler.setSynth (20:56:04.897) - MainThread (1812):
setSynth
Traceback (most recent call last):
File "synthDriverHandler.pyc", line 448, in setSynth
File "synthDriverHandler.pyc", line 420, in getSynthInstance
File "synthDriverHandler.pyc", line 303, in initSettings
File "synthDrivers\oneCore.pyc", line 268, in _get_rate
OSError: exception: access violation reading 0x00000000

lukaszgo1 · 2020-12-08T21:30:20Z

If properly tested, I'm not sure why this code introduces risk. We just will have to make sure that we test all of it in the full range of still supported Windows 10 builds.

Most of testing coverage we can get comes from Alpha users and Server versions of Windows / LTSB editions of Windows 10 are used mostly in corporate environments so I don't think many people are willing/able to use test builds in such circumstances.

There's a huge difference between Python and C++ in that C++ compiles all code before running it, which tackles most of the risk IMO. Changing Python code has a way bigger risk of experiencing regressions at run time.

Its obviously true when considering risks which comes from syntactic errors, but when it comes to regression testing our Python code has a lot of unit tests which simply does not exists for the C++ part.

In essence with changes like this it is more than likely that some kind of regression will slip through especiallly for older Win 10 versions so we're trading not so pretty but certainly working (it has been written back when old versions of Win 10 were a current one so it supports them well) code for a new one which is written in a modern way but may or ma not work.

LeonarddeR · 2020-12-09T06:58:27Z

Regardless what we do, I think I'd like to go to the bottom of this. I might be able to test on server 2016 later on.

Could you provide output of the following on 1511? NVDAHelper.getHelperLocalWin10Dll().ocSpeech_supportsProsodyOptions()

lukaszgo1 · 2020-12-09T12:29:50Z

Could you provide output of the following on 1511? NVDAHelper.getHelperLocalWin10Dll().ocSpeech_supportsProsodyOptions()

On the latest Alpha it returns 0, whereas with the build from this PR 1631488 is returned.

LeonarddeR · 2020-12-09T12:35:07Z

Wow, there's some real weirdness going on there indeed.

This reverts commit 7abe8f9.

LeonarddeR · 2020-12-09T13:25:21Z

It looks like IsApiContractPresent panics if you provide it a regular string that's implicitly converted to a hstring. Providing it a hstring seems to fix it. @lukaszgo1 could you verify?

lukaszgo1 · 2020-12-09T16:50:39Z

Yes, OneCore works correctly using latest build on 1511.

LeonarddeR · 2021-10-20T19:20:43Z

@feerrenrut I hope I have addressed all the review concerns.

Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>

feerrenrut

Let's leave the other more complicated design changes for a follow up. With these minor fixes I propose we merge.

Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>

…3072) 1. Visual Studio has support for C++20. 2. WinRT is C++20 (#11768). 3. Compiling with C++20 ensures code is more conformant. - Passing string literals to parameters expecting `wchar_t*` or `BSTR` is no longer supported. This ensures safe code, for a rationale see: https://docs.microsoft.com/en-us/cpp/build/reference/zc-strictstrings-disable-string-literal-type-conversion?view=msvc-170 - The `/permissive-` flag (note the minus symbol disabling permissive mode), ensuring conformance in more areas. See: https://docs.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-170 This change: - Updates build params so `nvdaHelper` is built with C++20 and disable permissive mode. - Fix errors (`wchar_t*` or `BSTR` from string literal)

Convert NVDAHelper LocalWin10 to C++/WinRT

7c89f2b

This comment has been minimized.

Sign in to view

Fix linting

83f3d25

feerrenrut reviewed Oct 19, 2020

View reviewed changes

Comment thread nvdaHelper/archBuild_sconscript Outdated

feerrenrut reviewed Oct 19, 2020

View reviewed changes

Comment thread nvdaHelper/localWin10/oneCoreSpeech.h

feerrenrut marked this pull request as draft October 19, 2020 10:17

Bring back win10 environment

3626314

feerrenrut reviewed Oct 19, 2020

View reviewed changes

Review action/linting

4ea6190

LeonarddeR added 2 commits December 8, 2020 19:20

Hopefully avoid crash

7abe8f9

Merge remote-tracking branch 'origin/master' into winrt

9847e66

LeonarddeR added 2 commits December 9, 2020 13:38

Revert "Hopefully avoid crash"

08bea9e

This reverts commit 7abe8f9.

Provide hstring to IsApiContractPresent

d6a75dd

Review actions

ceed023

LeonarddeR requested a review from feerrenrut October 20, 2021 19:20

LeonarddeR removed the blocked label Oct 21, 2021

LeonarddeR mentioned this pull request Oct 22, 2021

Swap minhook for Microsoft Detours #12964

Merged

10 tasks

lukaszgo1 reviewed Nov 2, 2021

View reviewed changes

Comment thread source/comInterfaces_sconscript Outdated

LeonarddeR added 3 commits November 3, 2021 07:37

Merge remote-tracking branch 'origin/master' into winrt

df608b1

Revert accidental change to cominterfaces_sconscript

808e7d1

Merge remote-tracking branch 'origin/master' into winrt

69ba8db

LeonarddeR mentioned this pull request Nov 19, 2021

Improve nvda helper by compiling with C++20 and disabling the /permissive flag #13072

Merged

5 tasks

Merge remote-tracking branch 'origin/master' into winrt

14e20e7

feerrenrut self-assigned this Jan 10, 2022

feerrenrut suggested changes Jan 13, 2022

View reviewed changes

LeonarddeR and others added 3 commits January 13, 2022 16:58

Apply suggestions from code review

20e1510

Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>

Merge remote-tracking branch 'origin/master' into winrt

7ccc3de

Update nvdaHelper/localWin10/oneCoreSpeech.cpp

b9bc37b

Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>

This comment has been minimized.

Sign in to view

LeonarddeR added 2 commits January 13, 2022 19:29

NO longer a shared pointer for markers

a722048

Add comments about exceptions

27fd086

feerrenrut suggested changes Jan 18, 2022

View reviewed changes

Comment thread nvdaHelper/localWin10/oneCoreSpeech.cpp Outdated

Comment thread nvdaHelper/localWin10/uwpOcr.cpp Outdated

feerrenrut mentioned this pull request Jan 18, 2022

Reconsider technical design decisions in onecore and ocr nvdaHelper #13253

Open

Apply suggestions from code review

bc10122

Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>

feerrenrut approved these changes Jan 19, 2022

View reviewed changes

feerrenrut added 2 commits January 19, 2022 18:01

Update changes doc for PR nvaccess#11768

f5f345a

Merge remote-tracking branch 'origin/master' into winrt

e996a08

feerrenrut approved these changes Jan 19, 2022

View reviewed changes

feerrenrut merged commit 4acef2b into nvaccess:master Jan 19, 2022

seanbudd mentioned this pull request Jun 2, 2022

Error with OneCore voice and automatic language switching in NVDA2022.1 #13732

Closed

LeonarddeR deleted the winrt branch August 23, 2025 06:28

Uh oh!

Conversation

LeonarddeR commented Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Code Review Checklist:

Uh oh!

This comment has been minimized.

josephsl commented Oct 17, 2020 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LeonarddeR commented Oct 17, 2020

Uh oh!

Brian1Gaff commented Oct 17, 2020 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dpy013 commented Oct 17, 2020

Uh oh!

LeonarddeR commented Oct 17, 2020

Uh oh!

Uh oh!

Uh oh!

feerrenrut commented Oct 19, 2020

Uh oh!

jcsteh commented Oct 19, 2020

Uh oh!

AppVeyorBot commented Oct 19, 2020

Uh oh!

feerrenrut Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 commented Dec 5, 2020

Uh oh!

LeonarddeR commented Dec 8, 2020

Uh oh!

lukaszgo1 commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszgo1 commented Dec 8, 2020

Uh oh!

LeonarddeR commented Dec 9, 2020

Uh oh!

lukaszgo1 commented Dec 9, 2020

Uh oh!

LeonarddeR commented Dec 9, 2020 via email

Uh oh!

LeonarddeR commented Dec 9, 2020

Uh oh!

lukaszgo1 commented Dec 9, 2020

Uh oh!

LeonarddeR commented Oct 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

LeonarddeR commented Oct 17, 2020 •

edited

Loading

josephsl commented Oct 17, 2020 via email •

edited by feerrenrut

Loading

Brian1Gaff commented Oct 17, 2020 via email •

edited by feerrenrut

Loading

lukaszgo1 commented Dec 8, 2020 •

edited

Loading