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: Improve Windows audio, remove sphelper-scummvm.h #1917

Merged
merged 1 commit into from Nov 6, 2019

Conversation

@sluicebox
Copy link
Member

sluicebox commented Nov 6, 2019

This removes sphelper-scummvm.h, a large patched Microsoft SDK header whose inclusion in the codebase raises the typical open source concerns: https://bugs.scummvm.org/ticket/11143.

Personally, I'm just offended that Microsoft ships a header that infects us with IsBadWritePtr().

In the process I found that we're setting the audio quality to SPSF_11kHz8BitMono, which is noticeably poor. I thought it had been my imagination that Windows TTS sounded bad on ScummVM but not my other programs. I don't see why we're setting a format at all. That's an OS setting so I've removed it, which conveniently removes another sphelper dependency, and makes TTS sound good.

@sluicebox sluicebox requested a review from vyzigold Nov 6, 2019
@sluicebox sluicebox force-pushed the sluicebox:gabbitygabgab branch from bc61808 to 0ee34c5 Nov 6, 2019
- Remove sphelper-scummvm.h
- Use default audio quality instead of lowest
- Add HRESULT tests
- Fix new[]/free() mismatches
- Fix voice description memory leak
@sluicebox sluicebox force-pushed the sluicebox:gabbitygabgab branch from 0ee34c5 to 18d6ffc Nov 6, 2019
@criezy

This comment has been minimized.

Copy link
Member

criezy commented Nov 6, 2019

The idea sounds good.

I don't develop on Windows and I don't have an opinion on the details of the changes, but I noticed you replaced some free with delete [] to free the result of Win32::unicodeToAnsi(). So I checked it and indeed that function uses new char[] and thus we should be using delete []. This change at least should be committed (and we have the same issue in OSystem_Win32::convertEncoding).

Copy link
Contributor

vyzigold left a comment

Good job, I'm glad the scummvm-sphelper.h is gone. I think, this even helped with the "lags" when using TTS in GUI on Windows.

@lotharsm

This comment has been minimized.

Copy link
Member

lotharsm commented Nov 6, 2019

Looks good to me too!

@sluicebox sluicebox merged commit fc97862 into scummvm:master Nov 6, 2019
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sluicebox

This comment has been minimized.

Copy link
Member Author

sluicebox commented Nov 6, 2019

Thanks TTS and Windows brain trusts! =) I'm a big fan of the TTS feature. Mergin'

@lotharsm

This comment has been minimized.

Copy link
Member

lotharsm commented Nov 6, 2019

I just re-checked this again with my latest build: The TTS quality is much better now without any audible lags. Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.