Hook up webspeech synthesis API to audio channel service

RESOLVED FIXED in Firefox 43

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: baku)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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)
(Assignee)

Comment 1

3 years ago
Created attachment 8646391 [details] [diff] [review]
speech1.patch
Flags: needinfo?(amarchesini)
Attachment #8646391 - Flags: review?(eitan)
(Assignee)

Updated

3 years ago
Assignee: nobody → amarchesini
(Assignee)

Comment 2

3 years ago
Created attachment 8646394 [details] [diff] [review]
speech1.patch
Attachment #8646391 - Attachment is obsolete: true
Attachment #8646391 - Flags: review?(eitan)
Attachment #8646394 - Flags: review?(eitan)

Updated

3 years ago
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-
(Assignee)

Comment 4

3 years ago
Created attachment 8649981 [details] [diff] [review]
speech1.patch

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+
(Assignee)

Comment 6

3 years ago
> @@ +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 :)
(Assignee)

Comment 8

3 years ago
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)
(Reporter)

Comment 11

3 years ago
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)
(Assignee)

Comment 14

3 years ago
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
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
I will follow by bug 1191667 for indirect service.

Updated

3 years ago
Blocks: 1197673
You need to log in before you can comment on or make changes to this bug.