Remove speech synth direct audio support

RESOLVED FIXED in Firefox 59
(NeedInfo from)

Status

()

Core
Web Speech
RESOLVED FIXED
11 months ago
3 days ago

People

(Reporter: eeejay, Assigned: jya, NeedInfo)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected, firefox59 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Reporter)

Description

11 months ago
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.
(Assignee)

Updated

9 days ago
Assignee: nobody → jyavenard
(Assignee)

Comment 1

8 days ago
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.
Flags: needinfo?(eitan)
(Assignee)

Comment 2

8 days ago
Created attachment 8933978 [details] [diff] [review]
fakesynth.patch
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

7 days ago
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

6 days ago
mozreview-review
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.
Attachment #8934119 - Flags: review+
Attachment #8934119 - Flags: review?(core-build-config-reviews)
(Reporter)

Comment 7

4 days ago
Created attachment 8935042 [details] [diff] [review]
make the mochitests pass

You can incorporate this into your patch.
(Reporter)

Comment 8

4 days ago
mozreview-review
Comment on attachment 8934119 [details]
Bug 1331696 - P1. Remove WebSpeech Pico service.

https://reviewboard.mozilla.org/r/205062/#review211508
Attachment #8934119 - Flags: review?(eitan) → review+
(Assignee)

Comment 9

4 days ago
awesome thanks...

I'll make this into a P3
Flags: needinfo?(eitan)
(Reporter)

Comment 10

4 days ago
mozreview-review
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
Attachment #8934120 - Flags: review?(eitan)
(Assignee)

Updated

4 days ago
Blocks: 1404997
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

4 days ago
mozreview-review
Comment on attachment 8935077 [details]
Bug 1331696 - P2. make the speech synth mochitests pass.

https://reviewboard.mozilla.org/r/205934/#review211538
Attachment #8935077 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 15

4 days ago
mozreview-review
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

4 days ago
mozreview-review
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]
(Reporter)

Comment 17

4 days ago
(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.
(Reporter)

Comment 18

4 days ago
mozreview-review
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/
Attachment #8934120 - Flags: review?(eitan) → review+
Comment hidden (mozreview-request)

Comment 20

4 days ago
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 21

4 days ago
mozreview-review
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

4 days ago
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
Flags: needinfo?(jyavenard)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 26

4 days ago
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
(Assignee)

Updated

4 days ago
See Also: → bug 1423867
Attachment #8934119 - Flags: review?(core-build-config-reviews)

Comment 27

3 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e347f793d8af
https://hg.mozilla.org/mozilla-central/rev/8b191f1e28a8
https://hg.mozilla.org/mozilla-central/rev/d71f9e83d6ef
Status: NEW → RESOLVED
Last Resolved: 3 days ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.