All the major platforms that we support (and will support) speech in writen to the audio device from the platform service. The exception is the b2g pico service which should probably be purged. If we remove MediaStreamGraph usage from speech we can greatly simplify our implementation.

I worked on it a bit this morning, and I get some mochitest failures like: dom/media/webspeech/synth/test/test_global_queue_pause.html | frame2: 'paused" does not match - got true, expected false @dom/media/webspeech/synth/test/file_global_queue_pause.html:79:7 Now, looking at the original code, it appears to me that this just expose problems there were always there By simply removing the FakeDirectAudioSynth, I can reproduce the error. It works in non-e10s mode, but with e10s it fails.. I can also get it to crash. I attach a simple patch to reproduce the problem.

So this is the code that removed direct speech synth. However, it only exposes that the code never really worked when e10s is on as the mochitests are testing. Only the FakeDirectAudioSynth code was ever exercised. With those changes, I manually tested on all platforms, and it works as far as I can tell. Just the mochitests are failing. I haven't looked at what is wrong in the IPC code.. Here is a try run with only FakeDirectAudioSynth disabled. It shows the failures and the crashes (the new code no longer crash however)

Comment on attachment 8934119 [details] Bug 1331696 - P1. Remove WebSpeech Pico service. https://reviewboard.mozilla.org/r/205062/#review210814 moz.build and old-configure.in changes lgtm.

You can incorporate this into your patch.

Comment on attachment 8934119 [details] Bug 1331696 - P1. Remove WebSpeech Pico service. https://reviewboard.mozilla.org/r/205062/#review211508

awesome thanks... I'll make this into a P3

Comment on attachment 8934120 [details] Bug 1331696 - P3. Remove direct audio support from speech synth. https://reviewboard.mozilla.org/r/205064/#review211512 This is good. Some more nits and places below that can be cleaned up. I think there is no more need for nsSpeechTask::Dispatch*Inner, so now we can have the less confusing pair of Dispatch*/Dispatch*Impl. ::: dom/media/webspeech/synth/nsISpeechService.idl:9 (Diff revision 1) > #include "nsISupports.idl" > > -typedef unsigned short SpeechServiceType; > - > /** > * A callback is implemented by the service. For direct audio services, it is Remove the comment about direct audio services (it was also not correct). ::: dom/media/webspeech/synth/nsISpeechService.idl:118 (Diff revision 1) > * The main interface of a speech synthesis service. > * > - * A service's speak method could be implemented in two ways: > + * A service's speak method could be implemented in one way: > * 1. Indirect audio - the service is responsible for outputting audio. > * The service calls the nsISpeechTask.dispatch* methods directly. Starting > * with dispatchStart() and ending with dispatchEnd or dispatchError(). How about removing this misleading list and just changing it to: A service is responsible for outputting audio. The service dispatches events, starting with dispatchStart() and ending with dispatchEnd or dispatchError(). A service must also respond with the currect actions and events in response to implemented callback methods. ::: dom/media/webspeech/synth/nsSpeechTask.h:50 (Diff revision 1) > - uint32_t GetCurrentCharOffset(); > - > void SetSpeechSynthesis(SpeechSynthesis* aSpeechSynthesis); > > - void InitDirectAudio(); > void InitIndirectAudio(); Change to Init(). ::: dom/media/webspeech/synth/test/nsFakeSynthServices.cpp:47 (Diff revision 1) > - {"urn:moz-tts:fake-direct:lenny", "Leonard Cohen", "en-CA", false, 0}, > - {"urn:moz-tts:fake-direct:celine", "Celine Dion", "fr-CA", false, 0}, > - {"urn:moz-tts:fake-direct:julie", "Julieta Venegas", "es-MX", false, }, > -}; > - > static const VoiceDetails sIndirectVoices[] = { rename this sVoices ::: dom/media/webspeech/synth/test/nsFakeSynthServices.cpp:48 (Diff revision 1) > - {"urn:moz-tts:fake-direct:celine", "Celine Dion", "fr-CA", false, 0}, > - {"urn:moz-tts:fake-direct:julie", "Julieta Venegas", "es-MX", false, }, > -}; > - > static const VoiceDetails sIndirectVoices[] = { > + {"urn:moz-tts:fake-indirect:bob", "Bob Marley", "en-JM", true, 0}, Rename this urn:moz-tts:fake:bob, etc. ::: dom/media/webspeech/synth/test/nsFakeSynthServices.cpp:118 (Diff revision 1) > - return NS_OK; > -} > - > -// FakeDirectAudioSynth > > class FakeIndirectAudioSynth : public nsISpeechService Rename this class FakeSpeechSynth ::: dom/media/webspeech/synth/test/nsFakeSynthServices.cpp:272 (Diff revision 1) > nsFakeSynthServices::Init() > { > - mDirectService = new FakeDirectAudioSynth(); > - AddVoices(mDirectService, sDirectVoices, ArrayLength(sDirectVoices)); > - > mIndirectService = new FakeIndirectAudioSynth(); mSynthService

Comment on attachment 8935077 [details] Bug 1331696 - P2. make the speech synth mochitests pass. https://reviewboard.mozilla.org/r/205934/#review211538

Comment on attachment 8935077 [details] Bug 1331696 - P2. make the speech synth mochitests pass. https://reviewboard.mozilla.org/r/205934/#review211540 ::: dom/media/webspeech/synth/test/file_global_queue_pause.html:65 (Diff revision 1) > testSynthState(win1, { speaking: true, pending: false, paused: false}); > - // 1188099: currently, paused state is not gaurenteed to be immediate. > + testSynthState(win2, { speaking: true, pending: true, paused: true }); > - testSynthState(win2, { speaking: true, pending: true }); > > // We now make the utterance end. > SpecialPowers.wrap(win1.speechSynthesis).forceEnd(); do we still need this force end?

Comment on attachment 8934120 [details] Bug 1331696 - P3. Remove direct audio support from speech synth. https://reviewboard.mozilla.org/r/205064/#review211548 C/C++ static analysis found 3 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/media/webspeech/synth/test/nsFakeSynthServices.cpp:122 (Diff revision 2) > > -class FakeDirectAudioSynth : public nsISpeechService > +class FakeSpeechSynth : public nsISpeechService > { > > public: > - FakeDirectAudioSynth() { } > + FakeSpeechSynth() {} Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] FakeSpeechSynth() {} ^ = default; ::: dom/media/webspeech/synth/test/nsFakeSynthServices.cpp:128 (Diff revision 2) > > NS_DECL_ISUPPORTS > NS_DECL_NSISPEECHSERVICE > > private: > - virtual ~FakeDirectAudioSynth() { } > + virtual ~FakeSpeechSynth() { } Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default] virtual ~FakeSpeechSynth() { } ^ = default; ::: dom/media/webspeech/synth/test/nsFakeSynthServices.cpp:203 (Diff revision 2) > nsCOMPtr<nsISpeechTask> mTask; > nsString mText; > }; > > uint32_t flags = 0; > - for (uint32_t i = 0; i < ArrayLength(sIndirectVoices); i++) { > + for (uint32_t i = 0; i < ArrayLength(sVoices); i++) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

(In reply to Jean-Yves Avenard [:jya] from comment #15) > Comment on attachment 8935077 [details] > Bug 1331696 - P2. make the speech synth mochitests pass. > > https://reviewboard.mozilla.org/r/205934/#review211540 > > ::: dom/media/webspeech/synth/test/file_global_queue_pause.html:65 > (Diff revision 1) > > testSynthState(win1, { speaking: true, pending: false, paused: false}); > > - // 1188099: currently, paused state is not gaurenteed to be immediate. > > + testSynthState(win2, { speaking: true, pending: true, paused: true }); > > - testSynthState(win2, { speaking: true, pending: true }); > > > > // We now make the utterance end. > > SpecialPowers.wrap(win1.speechSynthesis).forceEnd(); > > do we still need this force end? Yes. The utterance is set to a noend voice that will not end by itself.

Comment on attachment 8934120 [details] Bug 1331696 - P3. Remove direct audio support from speech synth. https://reviewboard.mozilla.org/r/205064/#review211554 This looks good. I checked on Linux, but please do some minimal testing on Mac and Windows with this patch. You can use this page: http://eeejay.github.io/webspeechdemos/

Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e2c5f5afb6fc P1. Remove WebSpeech Pico service. r=chmanchester,eeejay https://hg.mozilla.org/integration/autoland/rev/ce87e2d2f1db P2. make the speech synth mochitests pass. r=jya https://hg.mozilla.org/integration/autoland/rev/4f5d0c5f191b P3. Remove direct audio support from speech synth. r=eeejay

Comment on attachment 8934120 [details] Bug 1331696 - P3. Remove direct audio support from speech synth. https://reviewboard.mozilla.org/r/205064/#review211562 C/C++ static analysis found 2 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/media/webspeech/synth/test/nsFakeSynthServices.cpp:122 (Diff revision 3) > > -class FakeDirectAudioSynth : public nsISpeechService > +class FakeSpeechSynth : public nsISpeechService > { > > public: > - FakeDirectAudioSynth() { } > + FakeSpeechSynth() {} Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] FakeSpeechSynth() {} ^ = default; ::: dom/media/webspeech/synth/test/nsFakeSynthServices.cpp:128 (Diff revision 3) > > NS_DECL_ISUPPORTS > NS_DECL_NSISPEECHSERVICE > > private: > - virtual ~FakeDirectAudioSynth() { } > + virtual ~FakeSpeechSynth() { } Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default] virtual ~FakeSpeechSynth() { } ^ = default;

Backout by aiakab@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/efde0448d7bb Backed out 3 changesets for failing browser chrome on toolkit/components/narrate/test/browser_narrate.js r=backout on a CLOSED TREE

Backed out 3 changesets (bug 1331696) for failing browser chrome on toolkit/components/narrate/test/browser_narrate.js r=backout on a CLOSED TREE Link to a log: https://treeherder.mozilla.org/logviewer.html#?job_id=150298269&repo=autoland Backed out revision: https://hg.mozilla.org/integration/autoland/rev/efde0448d7bb7e5a7bef4ac6a9c734f87f2981a3 The failed push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4f5d0c5f191b55fbc33bb3b41762cd60b8ed73d0

Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e347f793d8af P1. Remove WebSpeech Pico service. r=chmanchester,eeejay https://hg.mozilla.org/integration/autoland/rev/8b191f1e28a8 P2. make the speech synth mochitests pass. r=jya https://hg.mozilla.org/integration/autoland/rev/d71f9e83d6ef P3. Remove direct audio support from speech synth. r=eeejay

https://hg.mozilla.org/mozilla-central/rev/e347f793d8af https://hg.mozilla.org/mozilla-central/rev/8b191f1e28a8 https://hg.mozilla.org/mozilla-central/rev/d71f9e83d6ef