Closed Bug 1191814 Opened 5 years ago Closed 5 years ago

Hook up webspeech synthesis API to audio channel service

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ehsan, Assigned: baku)

References

Details

Attachments

(1 file, 2 obsolete files)

The ultimate goal is to dispatch audio-playback events for speech synthesis start/stop.

Andrea, do you feel like taking this on when you get back?
Flags: needinfo?(amarchesini)
Attached patch speech1.patch (obsolete) — Splinter Review
Flags: needinfo?(amarchesini)
Attachment #8646391 - Flags: review?(eitan)
Assignee: nobody → amarchesini
Attached patch speech1.patch (obsolete) — Splinter Review
Attachment #8646391 - Attachment is obsolete: true
Attachment #8646391 - Flags: review?(eitan)
Attachment #8646394 - Flags: review?(eitan)
Blocks: 1191667
Comment on attachment 8646394 [details] [diff] [review]
speech1.patch

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

Sorry for the late review. Just got back from PTO.

I'm canceling the review because I am 90% sure this won't work in e10s. Maybe a new mochitest should be added as well to test in both environments.

::: dom/media/webspeech/synth/nsSpeechTask.cpp
@@ +689,5 @@
> +  if (!mStream) {
> +    return NS_OK;
> +  }
> +
> +  mStream->SetAudioOutputVolume(this, mVolume * aVolume * aMuted);

This will only work in non-e10s. When a task will have mStream AND mUtterance. Because you only have an agent in the content process, and the actual audio stream is on the top-level process. In other words, child process tasks have mUtterance != null, and mStream == null. Parent process tasks have no mUtterance, but do have an mStream. Non-e10s windows have one task with both.


Also, this will only work for direct audio services. Indirect services, like Windows that render to the sound device not through Firefox would not have their volume changed. This may be acceptable. If not, we would need to add a method to nsISpeechTaskCallback to allow changing the volume mid-utterance. This could be a followup patch...
Attachment #8646394 - Flags: review?(eitan) → review-
Attached patch speech1.patchSplinter Review
This patch should work for e10s. I'll write a mochitest in a separate patch.
Of course it doesn't the indirect audio, yet. Can it be a follow up?
Attachment #8646394 - Attachment is obsolete: true
Attachment #8649981 - Flags: review?(eitan)
Comment on attachment 8649981 [details] [diff] [review]
speech1.patch

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

Assuming you tested these changes, they look good with some small nits. Please add a mochitest patch before committing.

::: dom/media/webspeech/synth/nsSpeechTask.cpp
@@ +27,5 @@
>  
> +static bool UseAudioChannelService()
> +{
> +  return Preferences::GetBool("media.useAudioChannelService");
> +}

Is this really necessary as a function? You only use it in one conditional.

I don't feel super strong about this. It could stay.

@@ +688,5 @@
> +
> +NS_IMETHODIMP
> +nsSpeechTask::WindowAudioCaptureChanged()
> +{
> +  // This is not supported yet.

If I understand nsIAudioChannelAgentCallback correctly, this will never be implemented. Since this API is output only.

::: dom/media/webspeech/synth/nsSpeechTask.h
@@ +107,5 @@
>  
>    nsresult DispatchEndInner(float aElapsedTime, uint32_t aCharIndex);
>  
> +  void CreateAudioChannelAgent();
> +  void DestroyAudioChannelAgent();

Add newline between prototypes.

@@ +114,5 @@
>  
>    nsRefPtr<MediaInputPort> mPort;
>  
>    nsCOMPtr<nsISpeechTaskCallback> mCallback;
> +  nsCOMPtr<nsIAudioChannelAgent> mAudioChannelAgent;

Add an extra newline above mAudioChannelAgent.
Attachment #8649981 - Flags: review?(eitan) → review+
> @@ +688,5 @@
> > +
> > +NS_IMETHODIMP
> > +nsSpeechTask::WindowAudioCaptureChanged()
> > +{
> > +  // This is not supported yet.
> 
> If I understand nsIAudioChannelAgentCallback correctly, this will never be
> implemented. Since this API is output only.

Right, but this must be there because it's part of the AudioChannelCallback methods.

I'll upload a second patch for the mochitest.
(In reply to Andrea Marchesini (:baku) from comment #6)
> > @@ +688,5 @@
> > > +
> > > +NS_IMETHODIMP
> > > +nsSpeechTask::WindowAudioCaptureChanged()
> > > +{
> > > +  // This is not supported yet.
> > 
> > If I understand nsIAudioChannelAgentCallback correctly, this will never be
> > implemented. Since this API is output only.
> 
> Right, but this must be there because it's part of the AudioChannelCallback
> methods.

Yes. The comment is misleading. It is not supported, and it will never be :)
Actually, writing a mochitest is quite hard and it involves a lot of changes.
Can we land this patch without it?
Flags: needinfo?(eitan)
(In reply to Andrea Marchesini (:baku) from comment #8)
> Actually, writing a mochitest is quite hard and it involves a lot of changes.
> Can we land this patch without it?

Sure.
Flags: needinfo?(eitan)
Out of curiosity, why is it hard to test this?  Couldn't we at least add a test for the audio-playback notification similar to the other tests for that?
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f6317224a6

ehsan, the reason why is complex is because the audioChannel is managed by the task and this is not exposed and it can change in the utterance obj. An approach I tried was to dispatch events for testing but it's a lot of changes that too.

eeejay, the patch reintroduces mIndirectAudio because mStream is always null in e10s and my changes were breaking the logic.
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/c5f6317224a6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
I will follow by bug 1191667 for indirect service.
Blocks: 1197673
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.