Closed
Bug 1003452
Opened 11 years ago
Closed 10 years ago
Text to Speech API on Firefox for Mac
Categories
(Core :: DOM: Core & HTML, defect)
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)
683 bytes,
text/html
|
Details |
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.
Blocks: TTS_for_firefox
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → m_kato
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8646941 -
Attachment is obsolete: true
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8648561 [details] [diff] [review]
Implement OSX backend for WebSpeech Synthesis
OSX backend for WebSpeech synth.
Attachment #8648561 -
Flags: review?(smichaud)
Comment 4•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
(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•10 years ago
|
||
eeejay should probably also look at the patch.
Comment 8•10 years ago
|
||
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•10 years ago
|
||
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)
Comment 10•10 years ago
|
||
Welcome back! Could you please address my comment #8, and resubmit for review?
Flags: needinfo?(m_kato)
Assignee | ||
Updated•10 years ago
|
Attachment #8648561 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?smichaud
Attachment #8667284 -
Flags: review?(smichaud)
Assignee | ||
Updated•10 years ago
|
Attachment #8667284 -
Attachment is obsolete: true
Attachment #8667284 -
Flags: review?(smichaud)
Assignee | ||
Comment 13•10 years ago
|
||
Bug 1003452 - Implement OSX backend for WebSpeech Synthesis. r?smichaud
Attachment #8667302 -
Flags: review?(smichaud)
Comment 14•10 years ago
|
||
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•10 years ago
|
||
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)
Comment 16•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
> You should call [delegate delete] after this line.
[delegate release]
Comment 19•10 years ago
|
||
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]
Assignee | ||
Updated•10 years ago
|
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)
Assignee | ||
Comment 20•10 years ago
|
||
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•10 years ago
|
||
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+
Assignee | ||
Comment 23•10 years ago
|
||
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•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(m_kato)
Comment 25•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment 26•10 years ago
|
||
Updated•10 years ago
|
Keywords: dev-doc-needed
Comment 27•9 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
Comment 28•9 years ago
|
||
Chris, Awesome! Thanks!
Assignee | ||
Updated•9 years ago
|
Attachment #8667302 -
Attachment is obsolete: true
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•