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

TextToSpeech_unix: lazy initialize speech-dispatcher. #3071

Merged
merged 2 commits into from May 8, 2017

Conversation

@mkrautz
Copy link
Member

commented May 6, 2017

This PR includes two commits:

  • The first commit, to make TextToSpeech_unix lazily initialize speech-dispatcher.
  • Second commit, which fixes the behavior of setVolume(). Previously, our lazy-init wouldn't be effective, because TextToSpeech would call setVolume() on us very early on in Mumble's initialization.

@mkrautz mkrautz requested review from Kissaki, hacst and davidebeatrici May 6, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented May 6, 2017

One commit mentions that it's "TextToSpeech" that calls setVolume on its "engines". Well... It's actually just Log that does it.

@Kissaki
Kissaki approved these changes May 7, 2017
Copy link
Member

left a comment

Maybe this should be done in TextToSpeech instead of TextToSpeechPrivate?

That way every OS could make use of this? (And its more lifetime management logic than (OS) private TTS implementation.)

If you disagree, I'm fine with it.

@@ -21,6 +21,9 @@ class TextToSpeechPrivate {
#ifdef USE_SPEECHD
protected:
SPDConnection *spd;
int volume;

This comment has been minimized.

Copy link
@Kissaki

Kissaki May 7, 2017

Member

Comment: Store volume before initialization on use/say.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented May 7, 2017

Maybe this should be done in TextToSpeech instead of TextToSpeechPrivate?

That way every OS could make use of this? (And its more lifetime management logic than (OS) private TTS implementation.)

If you disagree, I'm fine with it.

I don't disagree. I actually like the idea.
But the problem is that TextToSpeech (non-private) is implemented per-OS, so it's really practical. (We'd duplicate the code for all implementations anyway.)
Also, I don't think it is necessary for Windows or macOS, so I'm willing to do this simpler version first.

@Kissaki

This comment has been minimized.

Copy link
Member

commented May 7, 2017

TextToSpeech (non-private) is implemented per-OS

Is that what you meant to say? TextToSpeech is one class, and the private classes are implemented per OS from what I saw earlier.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented May 7, 2017

Yes, that's what I wanted to say.

It's confusing. The file called TextToSpeech.cpp is the implementation that uses QtSpeech (from Qt 5.8+(?)). If you look in TextToSpeech_unix.cpp, you'll see it has its own TextToSpeech (non-private) class.

mkrautz added 2 commits May 6, 2017
TextToSpeech_unix: lazily initialize speech-dispatcher.
This commit should ensure that Mumble does not spawn
instances of the speech-dispatcher daemon for users that do not
use Text-to-Speech.
TextToSpeech_unix: make setVolume not initialize speech-dispatcher.
It turns out that TextToSpeech attempts to set the volume on its engines
near initialization time.

This made our lazy-initialization pretty much useless.

This commit tries to remedy that by storing the last value passed to setVolume.
It then uses that stored value the first time speech-dispatcher is initialized.

As a result, we only initialize speech-dispatcher in say(), not setVolume().

@mkrautz mkrautz force-pushed the mkrautz:lazy-init-spd branch from 1a3783c to 44687d5 May 7, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented May 7, 2017

Added a comment.

PTAL @Kissaki.

@Kissaki
Kissaki approved these changes May 7, 2017

@mkrautz mkrautz merged commit d3470c3 into mumble-voip:master May 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.