Closed Bug 1003452 Opened 10 years ago Closed 9 years ago

Text to Speech API on Firefox for Mac

Categories

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

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: yash.girdhar, Assigned: m_kato)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 4 obsolete files)

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.
OS: Linux → Mac OS X
Component: Disability Access APIs → DOM
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → m_kato
Attachment #8646941 - Attachment is obsolete: true
Comment on attachment 8648561 [details] [diff] [review]
Implement OSX backend for WebSpeech Synthesis

OSX backend for WebSpeech synth.
Attachment #8648561 - Flags: review?(smichaud)
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.
Attached file test case
(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.
eeejay should probably also look at the patch.
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 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.
Attachment #8648561 - Flags: review?(smichaud)
Welcome back! Could you please address my comment #8, and resubmit for review?
Flags: needinfo?(m_kato)
patch is updated soon
Flags: needinfo?(m_kato)
Attachment #8648561 - Attachment is obsolete: true
Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?smichaud
Attachment #8667284 - Flags: review?(smichaud)
Attachment #8667284 - Attachment is obsolete: true
Attachment #8667284 - Flags: review?(smichaud)
Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?smichaud
Attachment #8667302 - Flags: review?(smichaud)
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 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
Attachment #8667302 - Flags: review?(smichaud)
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.
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.
> You should call [delegate delete] after this line.

[delegate release]
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]
Attachment #8667302 - Attachment description: MozReview Request: Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?smichaud → MozReview Request: Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?eeejay
Attachment #8667302 - Flags: review?(eitan)
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 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.
Attachment #8667302 - Flags: review?(eitan) → review+
You know what your next steps are? :)
Flags: needinfo?(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.
Flags: needinfo?(m_kato)
https://hg.mozilla.org/mozilla-central/rev/26c7f2d75c75
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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.
Chris, Awesome! Thanks!
Attachment #8667302 - Attachment is obsolete: true
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: