Initial support for native TTS on linux through tts extension API.

Initial support for native TTS on linux through tts extension API.




BUG=none
TEST=Manually exercise the API through tools; try varying rate and pitch; try using a variety of native tts engines (espeak, ibmtts).


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155886

[David Tseng, 2012-09-09 18:47:14]

May not be ready for checkin.

Looking for initial feedback. This patch though not relying on the presence of
the libspeechd shared object, still requires the libspeechd.h header. Any
suggestions in regard to the proper way to handle this would be appreciated.

I wanted to get basic support for rate, pitch, and events on the initial patch.
Volume, index marks, and voice_name selection can come in another cl.

[dmazzoni, 2012-09-09 21:32:46]

+erg

Suggesting Elliot to review the dlopen part of the code, since he wrote
unity_service.cc.

I think that rather than including the header file, you just copy the function
prototypes and constants you need into the source file - indicating where they
came from. Elliot, please correct me if I'm wrong.

[dmazzoni, 2012-09-09 21:40:18]

https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/spee...
File chrome/browser/speech/extension_api/tts_extension_api_linux.cc (right):

https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/spee...
chrome/browser/speech/extension_api/tts_extension_api_linux.cc:17: typedef  
SPDConnection* (*spd_open_func)(const char* client_name,
Extra spaces

https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/spee...
chrome/browser/speech/extension_api/tts_extension_api_linux.cc:27:
SPDNotification notification);
Fix indentation

https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/spee...
chrome/browser/speech/extension_api/tts_extension_api_linux.cc:36:
SpeechDispatcherWrapper() : loaded_(false) {
This is long, so move the implementation below rather than inlined in the class
definition.

https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/spee...
chrome/browser/speech/extension_api/tts_extension_api_linux.cc:156:
SPDNotificationType SpeechDispatcherWrapper::current_notification =
Add a newline before this, and add a comment that says "static" just before it.
Also, it should end in an underscore just like other member variables.

https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/spee...
chrome/browser/speech/extension_api/tts_extension_api_linux.cc:176:
spd_.SetRate(100 * log10(params.rate));
Documentation for how you came up with this mapping?

It looks symmetrical, so that's good - but is there a reason why a rate of 2.0x
gets mapped to +30 and 0.5 gets mapped to -30? Do those roughly correspond to
twice and half the speaking rate of normal?

Try installing the TTS Debug app from the web store, it does some timing
measurements to make sure that your speech engine is calibrated in the right
ballpark.

https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/spee...
chrome/browser/speech/extension_api/tts_extension_api_linux.cc:215: //
TODO(dtseng): Unimplemented.
Can you implement these as part of this CL? They shouldn't be too bad, and then
it'd be complete...

[David Tseng, 2012-09-10 16:49:48]

On Sun, Sep 9, 2012 at 2:40 PM, <dmazzoni@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10915164/diff/**
>
4001/chrome/browser/speech/**extension_api/tts_extension_**api_linux.cc<https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/speech/extension_api/tts_extension_api_linux.cc>
> File chrome/browser/speech/**extension_api/tts_extension_**api_linux.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/10915164/diff/**
> 4001/chrome/browser/speech/**extension_api/tts_extension_**
>
api_linux.cc#newcode17<https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/speech/extension_api/tts_extension_api_linux.cc#newcode17>
> chrome/browser/speech/**extension_api/tts_extension_**api_linux.cc:17:
> typedef   SPDConnection* (*spd_open_func)(const char* client_name,
> Extra spaces

Done.
>
> https://chromiumcodereview.**appspot.com/10915164/diff/**
> 4001/chrome/browser/speech/**extension_api/tts_extension_**
>
api_linux.cc#newcode27<https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/speech/extension_api/tts_extension_api_linux.cc#newcode27>
> chrome/browser/speech/**extension_api/tts_extension_**api_linux.cc:27:
> SPDNotification notification);
> Fix indentation

Done.
>
> https://chromiumcodereview.**appspot.com/10915164/diff/**
> 4001/chrome/browser/speech/**extension_api/tts_extension_**
>
api_linux.cc#newcode36<https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/speech/extension_api/tts_extension_api_linux.cc#newcode36>
> chrome/browser/speech/**extension_api/tts_extension_**api_linux.cc:36:
> SpeechDispatcherWrapper() : loaded_(false) {
> This is long, so move the implementation below rather than inlined in
> the class definition.

Done.
>
> https://chromiumcodereview.**appspot.com/10915164/diff/**
> 4001/chrome/browser/speech/**extension_api/tts_extension_**
>
api_linux.cc#newcode156<https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/speech/extension_api/tts_extension_api_linux.cc#newcode156>
> chrome/browser/speech/**extension_api/tts_extension_**api_linux.cc:156:
> SPDNotificationType SpeechDispatcherWrapper::**current_notification =
> Add a newline before this, and add a comment that says "static" just
> before it. Also, it should end in an underscore just like other member
> variables.

Done.
>
> https://chromiumcodereview.**appspot.com/10915164/diff/**
> 4001/chrome/browser/speech/**extension_api/tts_extension_**
>
api_linux.cc#newcode176<https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/speech/extension_api/tts_extension_api_linux.cc#newcode176>
> chrome/browser/speech/**extension_api/tts_extension_**api_linux.cc:176:
> spd_.SetRate(100 * log10(params.rate));
> Documentation for how you came up with this mapping?
>
> It looks symmetrical, so that's good - but is there a reason why a rate
> of 2.0x gets mapped to +30 and 0.5 gets mapped to -30? Do those roughly
> correspond to twice and half the speaking rate of normal?
>
> Try installing the TTS Debug app from the web store, it does some timing
> measurements to make sure that your speech engine is calibrated in the
> right ballpark.
>

Added comments.
> https://chromiumcodereview.**appspot.com/10915164/diff/**
> 4001/chrome/browser/speech/**extension_api/tts_extension_**
>
api_linux.cc#newcode215<https://chromiumcodereview.appspot.com/10915164/diff/4001/chrome/browser/speech/extension_api/tts_extension_api_linux.cc#newcode215>
> chrome/browser/speech/**extension_api/tts_extension_**api_linux.cc:215: //
> TODO(dtseng): Unimplemented.
> Can you implement these as part of this CL? They shouldn't be too bad,
> and then it'd be complete...

Done. Pause/resume don't really have good mappings (and we don't ever
expose them through the wrapper or through the extension API), so I
paired them with a close match.
>
>
https://chromiumcodereview.**appspot.com/10915164/<https://chromiumcodereview...
>

[Elliot Glaysher, 2012-09-10 16:56:23]

On 2012/09/09 21:32:46, Dominic Mazzoni wrote:
> +erg
> 
> Suggesting Elliot to review the dlopen part of the code, since he wrote
> unity_service.cc.
> 
> I think that rather than including the header file, you just copy the function
> prototypes and constants you need into the source file - indicating where they
> came from. Elliot, please correct me if I'm wrong.

Correct. I hate that that's what needs to be done because it is _fragile_, but
it's how we integrate optional components that we can run without, without
adding build dependencies.

[Elliot Glaysher, 2012-09-10 16:59:02]

btw, the dlopen code lgtm.

[dmazzoni, 2012-09-10 18:59:50]

Should be fine from my end once you inline the definitions you need fro the
header file.

[David Tseng, 2012-09-10 21:09:14]

Ok. Went ahead and in-lined everything needed.


On Mon, Sep 10, 2012 at 11:59 AM, <dmazzoni@chromium.org> wrote:

> Should be fine from my end once you inline the definitions you need fro the
> header file.
>
>
https://chromiumcodereview.**appspot.com/10915164/<https://chromiumcodereview...
>

[dmazzoni, 2012-09-10 21:42:05]

lgtm

https://chromiumcodereview.appspot.com/10915164/diff/9002/chrome/browser/spee...
File chrome/browser/speech/extension_api/tts_extension_api_linux.cc (right):

https://chromiumcodereview.appspot.com/10915164/diff/9002/chrome/browser/spee...
chrome/browser/speech/extension_api/tts_extension_api_linux.cc:20: typedef enum{
Fix the spacing and indentation to match Chromium style, even though these came
from another file.

https://chromiumcodereview.appspot.com/10915164/diff/9002/chrome/browser/spee...
chrome/browser/speech/extension_api/tts_extension_api_linux.cc:221:
ExtensionTtsPlatformImplLinux* ExtensionTtsPlatformImplLinux::GetInstance() {
Please re-organize the method implementations so the methods for each class are
grouped together.

https://chromiumcodereview.appspot.com/10915164/diff/9002/chrome/browser/spee...
chrome/browser/speech/extension_api/tts_extension_api_linux.cc:296: if
(!loaded())
Indentation

[David Tseng, 2012-09-10 22:33:20]

On Mon, Sep 10, 2012 at 2:42 PM, <dmazzoni@chromium.org> wrote:
lgtm




https://chromiumcodereview.appspot.com/10915164/diff/9002/chrome/browser/spee...
File chrome/browser/speech/extension_api/tts_extension_api_linux.cc
(right):

https://chromiumcodereview.appspot.com/10915164/diff/9002/chrome/browser/spee...
chrome/browser/speech/extension_api/tts_extension_api_linux.cc:20:
typedef enum{
Fix the spacing and indentation to match Chromium style, even though
these came from another file.

Done.
https://chromiumcodereview.appspot.com/10915164/diff/9002/chrome/browser/spee...
chrome/browser/speech/extension_api/tts_extension_api_linux.cc:221:
ExtensionTtsPlatformImplLinux*
ExtensionTtsPlatformImplLinux::GetInstance() {
Please re-organize the method implementations so the methods for each
class are grouped together.

Done.
https://chromiumcodereview.appspot.com/10915164/diff/9002/chrome/browser/spee...
chrome/browser/speech/extension_api/tts_extension_api_linux.cc:296: if
(!loaded())
Indentation

Done.
https://chromiumcodereview.appspot.com/10915164/
https://chromiumcodereview.appspot.com/10915164

[commit-bot: I haz the power, 2012-09-10 22:47:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/10915164/10002

[commit-bot: I haz the power, 2012-09-11 00:46:02]

Change committed as 155886