1003452 - Text to Speech API on Firefox for Mac

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Yash]

As a part of Web Speech API (https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html), implement Text to Speech API on firefox for OS X.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Implement OSX backend for WebSpeech Synthesis (WIP)

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Implement OSX backend for WebSpeech Synthesis

Comment 3

[Makoto Kato [:m_kato]]

Comment on attachment 8648561 [details] [diff] [review]
Implement OSX backend for WebSpeech Synthesis

OSX backend for WebSpeech synth.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

I'll need to test this manually -- please provide some way to do so.

It may be a while before I can get to this.

Comment 5

[Makoto Kato [:m_kato]]

Attachment: test case

Comment 6

[Makoto Kato [:m_kato]]

(In reply to Steven Michaud [:smichaud] from comment #4)
> I'll need to test this manually -- please provide some way to do so.
> 
> It may be a while before I can get to this.

I attached test sample.  To test, you need media.webspeech.synth.enabled=true on about:config.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

eeejay should probably also look at the patch.

Comment 8

[Eitan Isaacson [:eeejay]]

Comment on attachment 8648561 [details] [diff] [review]
Implement OSX backend for WebSpeech Synthesis

Review of attachment 8648561 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm
@@ +215,5 @@
> +
> +    nsAutoString uri;
> +    uri.AssignLiteral("urn:moz-tts:osx:");
> +    uri.Append(identifier);
> +    rv = registry->AddVoice(mSpeechService, uri, name, locale, true, true);

Pretty sure you want global queue to be false, since NSSpeechSynthesizer does not queue utterances, and you use a separate instance for each Speak() call.

The desired effect, after setting global queue to false is that more than one window could synthesize speech at the same time!

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8648561 [details] [diff] [review]
Implement OSX backend for WebSpeech Synthesis

Sorry, but I'm going to be lazy and wait until eeejay's comments are addressed.

Comment 10

[Eitan Isaacson [:eeejay]]

Welcome back! Could you please address my comment #8, and resubmit for review?

Comment 11

[Makoto Kato [:m_kato]]

patch is updated soon

Comment 12

[Makoto Kato [:m_kato]]

Attachment: MozReview Request: Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?smichaud

Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?smichaud

Comment 13

[Makoto Kato [:m_kato]]

Attachment: MozReview Request: Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?eeejay

Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?smichaud

Comment 14

[Steven Michaud [:smichaud] (Retired)]

I'm not at all sure I'll have time for this -- I'm retiring, and tomorrow's my last day!

But I will try to find time.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8667302 [details]
MozReview Request:  Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?eeejay

https://reviewboard.mozilla.org/r/20705/#review18601

I found some problems.  Please update your patch quickly and resubmit for review.  I will lose my mozilla.com account after tomorrow, and will no longer be able to use mozreview!

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm:136
(Diff revision 1)
> +}

I *think* you currently leak mSpeechSynthesizer. Maybe you should call [mSpeechSynthesizer release] here.

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm:235
(Diff revision 1)
> +    if (![voice isEqualToString:defaultVoice]) {

Shouldn't this be:

if ([voice isEqualToString:defaultVoice]) {
...

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm:300
(Diff revision 1)
> +  // defualt rate is 180-220

defualt -> default

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm:306
(Diff revision 1)
> +  // Use default pitch value to calcurate this

calcurate -> calculate

Comment 16

[Steven Michaud [:smichaud] (Retired)]

I know very little about NSSpeechSynthesizer (or speech synthesis in general), and won't have time to learn.  So once I'm done reviewing someone else who knows more about it will also need to review.  Maybe eeejay?

I've just been evaluating whether or not your code is correct and seems plausible.

I'll test with your updated patch.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

https://reviewboard.mozilla.org/r/20705/#review18603

Another leak.

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm:320
(Diff revision 1)
> +  [synth setDelegate:delegate];

Another leak:

You should call [delegate delete] after this line.  The call to setDelegate: adds a reference to it which will keep it alive for the lifetime of the synth object.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

> You should call [delegate delete] after this line.

[delegate release]

Comment 19

[Steven Michaud [:smichaud] (Retired)]

https://reviewboard.mozilla.org/r/20705/#review18603

> Another leak:
> 
> You should call [delegate delete] after this line.  The call to setDelegate: adds a reference to it which will keep it alive for the lifetime of the synth object.

[delegate release], not [delegate delete]

Comment 20

[Makoto Kato [:m_kato]]

Comment on attachment 8667302 [details]
MozReview Request:  Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?eeejay

Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?eeejay

Comment 21

[Eitan Isaacson [:eeejay]]

Comment on attachment 8667302 [details]
MozReview Request:  Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?eeejay

https://reviewboard.mozilla.org/r/20705/#review19017

Mostly clarification questions. This patch looks pretty good besides that.

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm:29
(Diff revision 2)
> +    mStartingTime = TimeStamp::Now();

Does speech always start instantly? Is there an event to listen to for when the speech actually starts?

*update* I checked, and it looks like it does. That is very cool. All platforms should work that way.

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm:44
(Diff revision 2)
> +    [mSpeechSynthesizer release];

I know smichaud said you need release. But do you really? It looks like a raw pointer. I don't know objective c well, so just a question.

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm:70
(Diff revision 2)
> +  [mSpeechSynthesizer stopSpeaking];

Does OnDidFinishSpeaking get called after this? If not, you need to dispatch end.

::: dom/media/webspeech/synth/moz.build:47
(Diff revision 2)
> -        DIRS += ['windows']
> +        DIRS += [CONFIG['MOZ_WIDGET_TOOLKIT']]

Please make two separate if blocks for readability and consistency.

Comment 22

[Eitan Isaacson [:eeejay]]

You know what your next steps are? :)

Comment 23

[Makoto Kato [:m_kato]]

https://reviewboard.mozilla.org/r/20705/#review19017

> I know smichaud said you need release. But do you really? It looks like a raw pointer. I don't know objective c well, so just a question.

NSSpeechSynthsizer object is created by alloc method.  So we need to call release to decrement refcount.

> Does OnDidFinishSpeaking get called after this? If not, you need to dispatch end.

Yes.  stopSpeaking calls delegate interface.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26c7f2d75c75

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26c7f2d75c75

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26c7f2d75c75

Comment 27

[Chris Mills [:cmills]]

This has now been documented — see all the goodness at https://developer.mozilla.org/en-US/docs/Web/API/Web_Speech_API! A technical review would be really nice, although I appreciate that it is a fairly big API, so we might want to share the work between the different engineers involved.

Comment 28

[kdavis]

Chris, Awesome! Thanks!