1331696 - Remove speech synth direct audio support

Status: RESOLVED FIXED

(Core :: Web Speech, defect)

Description

[Eitan Isaacson [:eeejay]]

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.

Comment 1

[Jean-Yves Avenard [:jya]]

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.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: fakesynth.patch

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1331696 - P1. Remove WebSpeech Pico service.

It's no longer used, and won't be able to work with the removal of speech synth direct audio support.

Review commit: https://reviewboard.mozilla.org/r/205062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205062/

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1331696 - P3. Remove direct audio support from speech synth.

Review commit: https://reviewboard.mozilla.org/r/205064/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205064/

Comment 5

[Jean-Yves Avenard [:jya]]

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 6

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: make the mochitests pass

You can incorporate this into your patch.

Comment 8

[Eitan Isaacson [:eeejay]]

Comment on attachment 8934119 [details]
Bug 1331696 - P1. Remove WebSpeech Pico service.

https://reviewboard.mozilla.org/r/205062/#review211508

Comment 9

[Jean-Yves Avenard [:jya]]

awesome thanks...

I'll make this into a P3

Comment 10

[Eitan Isaacson [:eeejay]]

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 11

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1331696 - P2. make the speech synth mochitests pass.

Review commit: https://reviewboard.mozilla.org/r/205934/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205934/

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8934119 [details]
Bug 1331696 - P1. Remove WebSpeech Pico service.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205062/diff/1-2/

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8934120 [details]
Bug 1331696 - P3. Remove direct audio support from speech synth.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205064/diff/1-2/

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8935077 [details]
Bug 1331696 - P2. make the speech synth mochitests pass.

https://reviewboard.mozilla.org/r/205934/#review211538

Comment 15

[Jean-Yves Avenard [:jya]]

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 16

[Code Review Bot [:reviewbot]]

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]

Comment 17

[Eitan Isaacson [:eeejay]]

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

[Eitan Isaacson [:eeejay]]

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/

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8934120 [details]
Bug 1331696 - P3. Remove direct audio support from speech synth.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205064/diff/2-3/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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 21

[Code Review Bot [:reviewbot]]

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;

Comment 22

[Pulsebot]

Backout by [e-mail]:
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

Comment 23

[Arthur Iakab [arthur_iakab]]

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

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8934120 [details]
Bug 1331696 - P3. Remove direct audio support from speech synth.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205064/diff/3-4/

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8934120 [details]
Bug 1331696 - P3. Remove direct audio support from speech synth.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205064/diff/4-5/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Cosmin Sabou [:CosminS]]

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