Open Bug 1178738 Opened 5 years ago Updated Last year

SpeechSynthesisErrorEvent is not integrated into the speech synthesis machine

Categories

(Core :: Web Speech, defect)

defect
Not set

Tracking

()

People

(Reporter: kdavis, Assigned: eeejay)

Details

Attachments

(6 files)

As a result of  Bug 1167542 SpeechSynthesisErrorEvent is implemented but not 
integrated into the speech synthesis machine. In other words it exists but is 
never used.

Hence, SpeechSynthesisErrorEvent must be used in accord with the WebSpeech API

https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html
Assignee: nobody → kdavis
Assignee: kdavis → nobody
Assignee: nobody → eitan
Comment on attachment 8754118 [details]
Bug 1178738 - Update narrator to use error events when narration is stopped.

https://reviewboard.mozilla.org/r/53754/#review50584

LGTM. It does seem like this will intermittently cause tests to fail if people run them locally and their audio device is busy or something, but I don't see what we can do about that.
Attachment #8754118 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8754118 [details]
> MozReview Request: Bug 1178738 - Update narrator to use error events when
> narration is stopped. r?Gijs
> 
> https://reviewboard.mozilla.org/r/53754/#review50584
> 
> LGTM. It does seem like this will intermittently cause tests to fail if
> people run them locally and their audio device is busy or something, but I
> don't see what we can do about that.

Luckily we don't use an audio device or the system speech service in the narrate tests.
Comment on attachment 8754114 [details]
MozReview Request: Bug 1178738 - Update nsISpeechTask.DispatchError to provide error code. r?smaug

https://reviewboard.mozilla.org/r/53746/#review50980
Attachment #8754114 - Flags: review?(bugs) → review+
Comment on attachment 8754115 [details]
MozReview Request: Bug 1178738 - Implement dispatch of SpeechSynthesisErrorEvent. r?smaug

https://reviewboard.mozilla.org/r/53748/#review50982
Attachment #8754115 - Flags: review?(bugs) → review+
Comment on attachment 8754116 [details]
MozReview Request: Bug 1178738 - Have cancel() dispatch an "interrupted" error on spoken utterance. r?smaug

https://reviewboard.mozilla.org/r/53750/#review50988

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm:166
(Diff revision 1)
>    // XXX: Provide more specific error messages
>    mTask->DispatchError(GetTimeDurationFromStart(), aIndex,
>      uint32_t(dom::SpeechSynthesisErrorCode::Synthesis_failed));
>  }
>  
>  void

This method is called on mainthread only, right?

::: dom/media/webspeech/synth/speechd/SpeechDispatcherService.cpp:255
(Diff revision 1)
>      case SPD_EVENT_RESUME:
>        mTask->DispatchResume((TimeStamp::Now() - mStartTime).ToSeconds(), 0);
>        break;
>  
>      case SPD_EVENT_CANCEL:
> +      mTask->DispatchError((TimeStamp::Now() - mStartTime).ToSeconds(), 0,

ditto for this method

::: dom/media/webspeech/synth/windows/SapiService.cpp:135
(Diff revision 1)
>    case SPEI_START_INPUT_STREAM:
>      mTask->DispatchStart();
>      break;
>    case SPEI_END_INPUT_STREAM:
>      if (mSpeakTextLen) {
> +      // mSpeakTextLen will be > 0 on any utterance except a cancel utterance.

tiny bit odd that the nSpeakTextLen has such double meaning, but fine.
Attachment #8754116 - Flags: review?(bugs) → review+
Comment on attachment 8754117 [details]
Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

https://reviewboard.mozilla.org/r/53752/#review50990

Because of that, r-

::: dom/media/webspeech/synth/SpeechSynthesis.cpp:199
(Diff revision 1)
> -    // Remove all queued utterances except for current one, we will remove it
> -    // in OnEnd
> -    mSpeechQueue.RemoveElementsAt(1, mSpeechQueue.Length() - 1);
> -  } else {
> -    mSpeechQueue.Clear();
> +
> +  while (mSpeechQueue.Length() > startIndex) {
> +    RefPtr<SpeechSynthesisUtterance> utterance = mSpeechQueue.ElementAt(startIndex);
> +    mSpeechQueue.RemoveElementAt(startIndex);
> +
> +    utterance->DispatchSpeechSynthesisErrorEvent(

So what if some event listener adds more stuff to the queue? Dispatching events using the existing queue looks error prone.
Couldn't you swap the existing queue to some local variable and operate on that?

What happens if cancel() is called again while cancel() is being handled?
Attachment #8754117 - Flags: review?(bugs)
Attachment #8754117 - Flags: review-
https://reviewboard.mozilla.org/r/53752/#review50990

> So what if some event listener adds more stuff to the queue? Dispatching events using the existing queue looks error prone.
> Couldn't you swap the existing queue to some local variable and operate on that?
> 
> What happens if cancel() is called again while cancel() is being handled?

This is a good point!
Comment on attachment 8754114 [details]
MozReview Request: Bug 1178738 - Update nsISpeechTask.DispatchError to provide error code. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53746/diff/1-2/
Comment on attachment 8754115 [details]
MozReview Request: Bug 1178738 - Implement dispatch of SpeechSynthesisErrorEvent. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53748/diff/1-2/
Comment on attachment 8754116 [details]
MozReview Request: Bug 1178738 - Have cancel() dispatch an "interrupted" error on spoken utterance. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53750/diff/1-2/
Comment on attachment 8754117 [details]
Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53752/diff/1-2/
Comment on attachment 8754118 [details]
Bug 1178738 - Update narrator to use error events when narration is stopped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53754/diff/1-2/
(In reply to Eitan Isaacson [:eeejay] from comment #14)
> Created attachment 8757030 [details]
> MozReview Request: Bug 1178738 - Have SpeechSynthesis::mSpeechQueue not
> contain spoken utterance. r?smaug
> 
> Review commit: https://reviewboard.mozilla.org/r/55548/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/55548/

Here is an additional patch..

This simplifies some of the queue manipulations while slightly sacrificing the encapsulation of nsSpeechTask.
Comment on attachment 8754117 [details]
Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

https://reviewboard.mozilla.org/r/53752/#review52708

::: dom/media/webspeech/synth/SpeechSynthesis.cpp:213
(Diff revision 2)
> +    utterance->DispatchSpeechSynthesisErrorEvent(
> +      0, 0, SpeechSynthesisErrorCode::Canceled);
> +  }
>  
>    if (mCurrentTask) {
>      mCurrentTask->Cancel();

So some event listener may have initiated a new task, which you end up cancelling here. That is somewhat unexpected.
Shouldn't you take a strong ref to the current task and call Cancel using that or something?
Attachment #8754117 - Flags: review?(bugs)
Comment on attachment 8757030 [details]
MozReview Request: Bug 1178738 - Have SpeechSynthesis::mSpeechQueue not contain spoken utterance. r?smaug

https://reviewboard.mozilla.org/r/55548/#review52710

::: dom/media/webspeech/synth/SpeechSynthesis.cpp:125
(Diff revision 1)
>    return mHoldQueue || (mCurrentTask && mCurrentTask->IsPrePaused()) ||
> -         (!mSpeechQueue.IsEmpty() && mSpeechQueue.ElementAt(0)->IsPaused());
> +         (utterance && utterance->IsPaused());
>  }
>  
>  bool
>  SpeechSynthesis::HasEmptyQueue() const

ok, so empty queue is about current task and speech queue... please document that somewhere.
Attachment #8757030 - Flags: review?(bugs) → review+
https://reviewboard.mozilla.org/r/55548/#review52710

> ok, so empty queue is about current task and speech queue... please document that somewhere.

I know I made that function, but TBH I am not sure why it is necessary. It looks like you could simply do !Pending() && !Speaking(). I'm going to propose that in a followup bug. In the meantime, I'll document HasEmptyQueue in SpeechSynthesis.h.
https://reviewboard.mozilla.org/r/53752/#review52708

> So some event listener may have initiated a new task, which you end up cancelling here. That is somewhat unexpected.
> Shouldn't you take a strong ref to the current task and call Cancel using that or something?

Are you concerned mCurrentTask might be replaced by a new task from one of the listeners above, and then we call cancel() on the wrong one?

I don't think that is possible, mCurrentTask won't get replaced until it has "ended". If one of the listeners calls speak() it will wait and not advance the queue if mCurrentTask != null.
https://reviewboard.mozilla.org/r/53752/#review52708

> Are you concerned mCurrentTask might be replaced by a new task from one of the listeners above, and then we call cancel() on the wrong one?
> 
> I don't think that is possible, mCurrentTask won't get replaced until it has "ended". If one of the listeners calls speak() it will wait and not advance the queue if mCurrentTask != null.

.. and if that were the case we would get in trouble even if we did what you suggest, because OnEnd would set the wrong task to null.
(In reply to Eitan Isaacson [:eeejay] from comment #24)

> I don't think that is possible, mCurrentTask won't get replaced until it has
> "ended". If one of the listeners calls speak() it will wait and not advance
> the queue if mCurrentTask != null.

What if there are nested cancel()/speak() calls?
(In reply to Olli Pettay [:smaug] from comment #26)
> (In reply to Eitan Isaacson [:eeejay] from comment #24)
> 
> > I don't think that is possible, mCurrentTask won't get replaced until it has
> > "ended". If one of the listeners calls speak() it will wait and not advance
> > the queue if mCurrentTask != null.
> 
> What if there are nested cancel()/speak() calls?

I'm adding some mochitests and finding interesting things.. so good catch :P
Comment on attachment 8754114 [details]
MozReview Request: Bug 1178738 - Update nsISpeechTask.DispatchError to provide error code. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53746/diff/2-3/
Attachment #8754117 - Flags: review?(bugs)
Comment on attachment 8754115 [details]
MozReview Request: Bug 1178738 - Implement dispatch of SpeechSynthesisErrorEvent. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53748/diff/2-3/
Comment on attachment 8754116 [details]
MozReview Request: Bug 1178738 - Have cancel() dispatch an "interrupted" error on spoken utterance. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53750/diff/2-3/
Comment on attachment 8757030 [details]
MozReview Request: Bug 1178738 - Have SpeechSynthesis::mSpeechQueue not contain spoken utterance. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55548/diff/1-2/
Comment on attachment 8754117 [details]
Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53752/diff/2-3/
Comment on attachment 8754118 [details]
Bug 1178738 - Update narrator to use error events when narration is stopped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53754/diff/2-3/
https://reviewboard.mozilla.org/r/53752/#review52708

> .. and if that were the case we would get in trouble even if we did what you suggest, because OnEnd would set the wrong task to null.

Changed it to have a stored list of canceled utterances to dispatch "canceled" to after current task ended, or immediately if there is no task.
Comment on attachment 8754117 [details]
Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

https://reviewboard.mozilla.org/r/53752/#review53724

::: dom/media/webspeech/synth/SpeechSynthesis.cpp:198
(Diff revision 3)
>  
>    return;
>  }
>  
>  void
> +SpeechSynthesis::DispatchToCanceledQueue() {

{ goes to its own line.
Attachment #8754117 - Flags: review?(bugs) → review+
Comment on attachment 8754117 [details]
Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53752/diff/3-4/
Attachment #8754117 - Attachment description: MozReview Request: Bug 1178738 - Have utterances dispatch "canceled" error when they never start. r?smaug → Bug 1178738 - Have utterances dispatch "canceled" error when they never start.
Attachment #8754118 - Attachment description: MozReview Request: Bug 1178738 - Update narrator to use error events when narration is stopped. r?Gijs → Bug 1178738 - Update narrator to use error events when narration is stopped.
Comment on attachment 8754118 [details]
Bug 1178738 - Update narrator to use error events when narration is stopped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53754/diff/3-4/
Pushed by eisaacson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21944f25fbf0
Update nsISpeechTask.DispatchError to provide error code. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe5dd5c3f33
Implement dispatch of SpeechSynthesisErrorEvent. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c66c0ddf562
Have cancel() dispatch an "interrupted" error on spoken utterance. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab50796d2616
Have SpeechSynthesis::mSpeechQueue not contain spoken utterance. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c995bbdfa3a
Have utterances dispatch "canceled" error when they never start. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/54d1e8caf69b
Update narrator to use error events when narration is stopped. r=Gijs
I'm having trouble pinpointing the issue. I'm going to remove this as a blocker for bug 1268633. No other browser supports SpeechSynthesisError events this yet.
Flags: needinfo?(eitan)
No longer blocks: 1268633
You need to log in before you can comment on or make changes to this bug.