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?

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...

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?

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.

> @@ +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?

(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.

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?

Backed out for m-e10s(3) failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a1489f33c93 https://treeherder.mozilla.org/logviewer.html#?job_id=13095164&repo=mozilla-inbound

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.

I will follow by bug 1191667 for indirect service.