Skip to content
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

Support for Windows OneCore voices included in Windows 10. #7110

Merged
merged 13 commits into from Jun 13, 2017
Merged

Conversation

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Apr 28, 2017

This uses a C++/CX dll to access the UWP SpeechSynthesizer class. There are other UWP APIs we might like to access in future (e.g. OCR), so rather than making this dll specific to OneCore speech, it's called nvdaHelperLocalWin10. The build system for this dll makes it easy to add other components in future.
In addition, this required code to generate balanced XML from an NVDA speech sequence. Although we use SSML for eSpeak, eSpeak happily accepts unbalanced (malformed) XML. OneCore speech does not. This code is in the speechXml module. This might eventually be reused to replace the ugly balanced XML code in the SAPI5 driver.
Fixes #6159.

This uses a C++/CX dll to access the UWP SpeechSynthesizer class. There are other UWP APIs we might like to access in future (e.g. OCR), so rather than making this dll specific to OneCore speech, it's called nvdaHelperLocalWin10. The build system for this dll makes it easy to add other components in future.
In addition, this required code to generate balanced XML from an NVDA speech sequence. Although we use SSML for eSpeak, eSpeak happily accepts unbalanced (malformed) XML. OneCore speech does not. This code is in the speechXml module. This might eventually be reused to replace the ugly balanced XML code in the SAPI5 driver.
@jcsteh jcsteh requested a review from michaelDCurran Apr 28, 2017
Copy link
Member

@michaelDCurran michaelDCurran left a comment

Attribute values should be also escaped, which will require " in xml_escapes

"""
# comtypes.BSTR.from_address seems to cause a crash for some reason. Not sure why.
# Just access the string ourselves.
val = ctypes.wstring_at(address)

This comment has been minimized.

@michaelDCurran

michaelDCurran May 2, 2017
Member

Would there ever be the case where a NULL char might be in that string? probably not, but you could use SysStringLen to get the length and pass it to wstring_at.

super(SynthDriver, self).terminate()
# Drop the ctypes function instance for the callback,
# as it is holding a reference to an instance method, which causes a reference cycle.
self._dll.ocSpeech_terminate(self._handle)

This comment has been minimized.

@michaelDCurran

michaelDCurran May 2, 2017
Member

Perhaps move this line above the comment as the comment is only about the callback.

ComPtr<IBufferByteAccess> bufferByteAccess;
insp.As(&bufferByteAccess);
byte* bytes = nullptr;
bufferByteAccess->Buffer(&bytes);

This comment has been minimized.

@michaelDCurran

michaelDCurran May 2, 2017
Member

Can inst.as fail? I.e. that interface is not supported? and perhaps then the pointer would be NULL?

@jcsteh jcsteh requested a review from michaelDCurran May 3, 2017
@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented May 15, 2017

@feerrenrut, would you mind reviewing 3b43394 and c02a788? These fix a reported issue where invalid characters break the OneCore Speech driver. @michaelDCurran originally reviewed this, but he's away for a week or so now and it'd be good to get this in ASAP.

The first commit ensures that errors get logged and don't break the driver for all subsequent speech. Previously, there was no logging and speech just went silent. The second fixes the actual problem with invalid characters.

@jcsteh jcsteh requested a review from feerrenrut May 15, 2017
Copy link
Member

@feerrenrut feerrenrut left a comment

As discussed, the changes look good.

jcsteh added a commit that referenced this pull request May 15, 2017
…etc. Incubates #7110 (issue #6159).
…eeded for OneCore Speech.
@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented May 17, 2017

@feerrenrut, would you mind reviewing ba62552? It updates the build system to bundle some required redistributable VC2015 components that aren't installed on all systems.

Two users confirmed that this fixes the problem for them; see this nvda-devel thread.

@jcsteh jcsteh requested a review from feerrenrut May 17, 2017
jcsteh added a commit that referenced this pull request May 18, 2017
…led. Incubates #7110 (issue #6159).
@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented May 24, 2017

@feerrenrut, would you mind reviewing three more commits? Sorry. :(

  1. f1a34ce deals with #6159 (comment): a document which specifies an invalid language code (or one unknown to Windows) causes an exception in the driver. We detect such language codes and ignore them. We don't do this more generally (in core speech code) because we can't assume that some other synth won't know about a language that Windows doesn't; e.g. Windows doesn't know about the Aragonese language, but NVDA and eSpeak do.
  2. 1f9f90a adds a User Guide section which I neglected to add earlier.
  3. 032e65e excludes this driver from available synths for binary copies if not Windows 10. The driver only works on Windows 10. We don't exclude for source copies because source copies can't detect Windows 10 due to Python's manifest. That's okay, though, because developers can be expected to understand such things and only developers would run from source. Note that the synth will still fail gracefully if the user attempts to use it in this case.
@jcsteh jcsteh requested a review from feerrenrut May 24, 2017
@@ -81,6 +84,13 @@ def convertCharacterModeCommand(self, command):
# Therefore, we don't use it.
return None

def convertLangChangeCommand(self, command):
lcid = languageHandler.localeNameToWindowsLCID(command.lang)
if lcid in (0, languageHandler.LOCALE_CUSTOM_UNSPECIFIED):

This comment has been minimized.

@feerrenrut

feerrenrut May 25, 2017
Member

How do you get a value of zero and what does it represent? Is there a test for this case?

This comment has been minimized.

@jcsteh

jcsteh May 25, 2017
Author Contributor

Honestly, I'm not sure. I did this because the other place we use that function checks for 0. It seems we can return 0 on Windows XP (where we implement all of that function ourselves), but that doesn't apply here, since this is always Windows 10. On Vista and later, we use the Windows LocaleNameToLCID function. That function says it can return 0 for invalid parameter values, but I don't know how to trigger this.

What do you think? On one hand, it might be better to remove the 0 so users notice potential unknown problems and report them. On the other hand, if we do get 0, there's probably nothing we can do and we'll end up just ignoring the language anyway.

This comment has been minimized.

@feerrenrut

feerrenrut May 25, 2017
Member

In that case I think its best to keep checking for it and logging. It might be worth adding the return value to the log message. I guess we could name the zero something like INVALID_PARAM_VALUES or include some of what you said here as a comment to save the next person from having to look up what zero means.

@jcsteh jcsteh requested a review from feerrenrut May 26, 2017
@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented May 26, 2017

@feerrenrut, as discussed, I've simplified localeNameToWindowsLCID so it returns 0 in all failure cases (with a constant of LCID_NONE). I'd prefer to just use Python's None, but I'm erring on the side of backwards compat caution. I also got rid of the oneCore synth driver tests and instead added tests for localeNameToWindowsLCID, since the method I was testing in oneCore is really just a thin wrapper around the latter.

Copy link
Member

@feerrenrut feerrenrut left a comment

I think these comments really help those who aren't familiar with this area.

@@ -1,34 +0,0 @@
#tests/unit/synthDrivers/test_oneCore.py

This comment has been minimized.

@feerrenrut

feerrenrut May 29, 2017
Member

I guess this was removed by accident?

This comment has been minimized.

@jcsteh

jcsteh May 29, 2017
Author Contributor

No; it was intentional. I got rid of the oneCore synth driver tests and instead added tests for localeNameToWindowsLCID , since the method I was testing in oneCore is really just a thin wrapper around the latter.

# #6259: LOCALE_CUSTOM_UNSPECIFIED denotes custom locale in Windows 10, thus returns "unknown language" or an odd description (observed for Aragonese).
# See https://msdn.microsoft.com/en-us/library/system.globalization.cultureinfo.lcid(v=vs.110).aspx.
if LCID not in (0, LOCALE_CUSTOM_UNSPECIFIED):
if LCID is not LCID_NONE:

This comment has been minimized.

@feerrenrut

feerrenrut May 29, 2017
Member

I think this is much clearer

@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented May 30, 2017

As we discussed yesterday, don't forget to update the readme. I had to ensure that I installed Tools 1.4.1 and Windows 10 SDK 10.0.14393 with visual studio community edition to get this to build.

@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented May 30, 2017

Readme updated.

@jcsteh jcsteh requested a review from feerrenrut May 30, 2017
jcsteh added a commit that referenced this pull request May 30, 2017
@jcsteh jcsteh merged commit 2ee954f into master Jun 13, 2017
@nvaccessAuto nvaccessAuto added this to the 2017.3 milestone Jun 13, 2017
@jcsteh jcsteh deleted the oneCoreSpeech branch Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants