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

TTS: Fix crashing and freezing when compiled with msvc #1831

Merged
merged 4 commits into from Sep 5, 2019

Conversation

@vyzigold
Copy link
Contributor

commented Sep 5, 2019

This pull request resolves crashing when compiling with msvc and using text to speech. The crashes were actualy caused by Common::Encoding, that returned a converted string from SDL_iconv_string(), which couldn't be freed by free(), while strings returned by Common::Encoding should be freed by free().

This also fixes the freezing caused by waiting on a thread while trying to stop speech (the freeze could last up to multiple seconds). There still is a barely noticable lag when using TTS in the GUI and moving fast with the cursor between multiple GUI widgets and thus starting and stoping multiple speeches too quickly. This is caused by a call to ISpVoice->Speak() inside the stop() method. I haven't found a better solution to stopping the speech yet, but the lag shouldn't be noticable with TTS in games (like in Mortevielle) and people with reduced sight (which should be the people, that will be using the TTS in GUI) also shouldn't notice.

@criezy
Copy link
Member

left a comment

Nice. I had started to look at this as well and I came up with the same solution you did to compute the size in byte of a string.

However I don't agree with the commit aa5ab6f. We cannot do the copy everywhere where OSystem_SDL::convertEncoding() is called, because sometimes we don't even know that it has been called. And you cannot assume either that Windows is the only platform on which SDL_free() is not compatible with free(). So in my opinion the copy should not be done in OSystem_Win32::convertEncoding() after calling OSystem_SDL::convertEncoding(), but instead it should be done directly in OSystem_SDL::convertEncoding().

Also please add a else for the second if in

if (Common::String(to).hasPrefixIgnoreCase("utf-16"))
	zeroBytes = 2;
if (Common::String(to).hasPrefixIgnoreCase("utf-32"))
	zeroBytes = 4;

@vyzigold vyzigold force-pushed the vyzigold:tts2 branch from d89c91a to 1420b84 Sep 5, 2019

@criezy

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Thank you. Can you please squash commits aa5ab6f and 1420b84 together? Maybe also add a comment just before the new code that makes the copy to indicate why it is necessary.
After that I think it will be ready to be merged.

vyzigold added 2 commits Sep 5, 2019

@vyzigold vyzigold force-pushed the vyzigold:tts2 branch from 1420b84 to 3405cc7 Sep 5, 2019

@criezy
criezy approved these changes Sep 5, 2019

@vyzigold vyzigold force-pushed the vyzigold:tts2 branch from 3405cc7 to 5a5ebe5 Sep 5, 2019

@vyzigold

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

The commits are squashed and I added an explanation for the copy.

@criezy criezy merged commit 6886ae0 into scummvm:master Sep 5, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.