1094764 - Implement AudioContext.suspend and friends

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Paul Adenot (:padenot)]

Spec: http://webaudio.github.io/web-audio-api/#widl-AudioContext-suspend-Promise (see also the `close` and `resume` members).

This will be the proper solution to our battery consumption issues on FxOS caused by the AudioContext writing silence and keeping an audio stream alive.

Because a bunch of AudioContext share the same audio output thread, we need to do some graph traversal to determine if we can really pause the thread (to make sure another AudioContext is not running, or a WebRTC call is not happening, etc.).

Comment 1

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be6f7a17effa

Comment 2

[Paul Adenot (:padenot)]

Attachment: Implement AudioContext.suspend and friends. r=

Uploading a first patch. It's 90% there, but somehow something like
https://paul.cx/public/suspend5.html does not work properly, it triggers this
[0] assert a when suspending the first context, and does not restore the sound
when resuming.

[0]: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaStreamGraph.cpp?from=MediaStreamGraph.cpp#1331

Comment 3

[Paul Adenot (:padenot)]

Attachment: Implement AudioContext.suspend and friends. r=

- Relevant spec text:
    - http://webaudio.github.io/web-audio-api/#widl-AudioContext-suspend-Promise
    - http://webaudio.github.io/web-audio-api/#widl-AudioContext-resume-Promise
    - http://webaudio.github.io/web-audio-api/#widl-AudioContext-close-Promise
    - http://webaudio.github.io/web-audio-api/#widl-AudioContext-state
    - http://webaudio.github.io/web-audio-api/#widl-AudioContext-onstatechange

- In a couple words, the behavior we want:
    - Closed context cannot have new nodes created, but can do decodeAudioData,
    and create buffers, and such.
    - OfflineAudioContexts don't support those methods, transitions happen at
    startRendering and at the end of processing. onstatechange is used to make
    this observable.
    - (regular) AudioContexts support those methods. The promises and
    onstatechange should be resolved/called when the operation has actually
    completed on the rendering thread.  Once a context has been closed, it
    cannot transition back to "running". An AudioContext switches to "running"
    when the audio callback start running, this allow authors to know how long
    the audio stack takes to start running.
    - MediaStreams that feed in/go out of a suspended graph should respectively
    not buffer at the graph input, and output silence
    - suspended context should not be doing much on the CPU, and we should try
    to pause audio streams if we can (this behaviour is the main reason we need
    this in the first place, for saving battery on mobile, and CPU on all
    platforms)

- Now, the implementation:
    - AudioNodeStreams are now tagged with a context id, to be able to operate
    on all the streams of a given AudioContext on the Graph thread without
    having to go and lock everytime to touch the AudioContext. This happens in
    the AudioNodeStream ctor.
    - When an AudioContext goes into suspended mode, streams for this
    AudioContext are moved out of the mStreams array to a second array,
    mSuspendedStream. Streams in mSuspendedStream are not ordered, and are not
    processed.
    - The MSG will automatically switch to a SystemClockDriver when it finds
    that there are no more AudioNodeStream/Stream with an audio track. This is
    how pausing the audio subsystem and saving battery works. Subsequently, when
    the MSG finds that there are only streams in mSuspendedStreams, it will go
    to sleep (block on a monitor), so we save CPU, but it does not shut itself
    down. This is mostly not a new behaviour (this is what the MSG does since
    the refactoring), but it is important to note.
    - Promises are gripped (addref-ed) on the main thread, and then shepherd
    down other threads and to the GraphDriver, if needed (sometimes we can
    resolve them right away). Then, the driver executes the operation, and when
    it's done (initializing and closing audio streams can take some time), we
    send the promise back to the main thread, and resolve it. This way, we can
    send them back on the main thread once an operation has complete (suspending
    an audio stream, starting it again on resume(), etc.), without having to do
    bookkeeping between suspend calls and their result. Promises are not thread
    safe, so we can't move them around AddRef-ed.
    - The stream destruction logic now takes into account that a stream can be
    destroyed while not being in mStreams.
    - A graph can now switch GraphDriver twice or more per iteration, for
    example if an author goes suspend()/resume()/suspend() in the same script.
    - Some operation have to be done on suspended stream, so we now use double
    for-loop around mSuspendedStreams and mStreams in some places in
    MediaStreamGraph.cpp.
    - A tricky part was making sure everything worked at AudioContext
    boundaries.  TrackUnionStream that have one of their input stream suspended
    append null ticks instead, that seem to work fine.
    - The graph ordering algorithm had to be altered to not include suspended
    streams.
    - There are some edge cases (adding a stream on a suspended graph, calling
    suspend/resume when a graph has just been close()d), I tried to comment.

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 8570567 [details] [diff] [review]
Implement AudioContext.suspend and friends. r=

roc, this is a a big patch, but I haven't found a good way to split it that is not a dumb split per file. It's really one feature, and mostly easy plumbing, though.

Maybe a good review order would be:
- AudioContext.cpp (DOM bindings sending message controls to the MSG)
- AudioDestinationNode.cpp (offline audio context handling)
- MediaStreamGraph.cpp
    - ApplyAudioContextOperationImpl and function it calls
    - The modifications to the (quasi)-topological sort
    - The preservation of stream invariants for suspended streams (everywhere there is a runningAndSuspendedPair array)
- GraphDriver.cpp

Basically, walking down from the DOM to the audio thread.

Comment 5

[Paul Adenot (:padenot)]

Comment on attachment 8570567 [details] [diff] [review]
Implement AudioContext.suspend and friends. r=

Ehsan, would you mind looking at the DOM bits in AudioContext.cpp ?

Comment 6

[Paul Adenot (:padenot)]

Comment on attachment 8570567 [details] [diff] [review]
Implement AudioContext.suspend and friends. r=

Karl, this is modifying you sort algorithm in the MSG (MediaStreamGraphImpl::UpdateStreamOrder) to avoid it walking to streams that are in suspended mode, does that look right to you?

This happens when two AudioContexts are connected together, using MediaStreamAudioDestinationNode and MediaStreamAudioSourceNode, and one of those AudioContexts is suspended, and its streams should not be ordered, because they are not going to be processed. Adjustments have been made to other parts of the code so that a ProcessedMediaStream that has only suspended streams as its inputs works fine.

A stream is in suspended mode when it is in mSuspendedStreams (instead of being in mStreams).

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8570567 [details] [diff] [review]
Implement AudioContext.suspend and friends. r=

Review of attachment 8570567 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/TrackUnionStream.cpp
@@ +85,5 @@
>      bool allHaveCurrentData = !mInputs.IsEmpty();
>      for (uint32_t i = 0; i < mInputs.Length(); ++i) {
>        MediaStream* stream = mInputs[i]->GetSource();
> +      if (GraphImpl()->StreamSuspended(stream)) {
> +        mBuffer.FindTrack(2)->GetSegment()->AppendNullData(aTo - aFrom);

FindTrack(2) looks bad :-(. Having to check for suspended streams here is bad too.

In fact, having these special suspended streams is unfortunate.

Are you sure that it's not possible to implement this by permanently blocking the suspended streams?

::: dom/media/webaudio/AudioContext.cpp
@@ +42,5 @@
>  namespace dom {
>  
> +// 0 is a special value that MediaStreams use to denote they are not part of a
> +// AudioContext.
> +static uint32_t gAudioContextId = 1;

I think we should use a 64-bit ID. It's not impossible for a script to cause wraparound otherwise.

Make a typedef for it.

@@ +208,5 @@
>  {
> +  if (mAudioContextState == AudioContextState::Closed) {
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return nullptr;
> +  }

Let's have a helper function so we can write
  if (CheckClosed(aRv)) {
    return nullptr;
  }

@@ +779,5 @@
> +             (mAudioContextState == AudioContextState::Suspended &&
> +              aNewState == AudioContextState::Closed)    ||
> +             (mAudioContextState == AudioContextState::Closed &&
> +              aNewState == AudioContextState::Closed),
> +             "Invalid AudioContextState transition");

We can simplify this by checking mAudioContextState == aNewState, which must obviously always be legal.

Should we be firing statechanged when the state hasn't actually changed?

@@ +809,5 @@
> +    return nullptr;
> +  }
> +  if (mIsOffline) {
> +    promise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return promise.forget();

The spec says "This method will cause an INVALID_STATE_ERR exception to be thrown if called on an OfflineAudioContext." which I think means we need to throw in aRv here instead of in the promise.

@@ +847,5 @@
> +  }
> +
> +  if (mIsOffline) {
> +    promise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return promise.forget();

Likewise, I think this needs to throw instead of rejecting the promise.

@@ +885,5 @@
> +  }
> +
> +  if (mIsOffline) {
> +    promise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return promise.forget();

Here too

@@ +890,5 @@
> +  }
> +
> +  if (mAudioContextState == AudioContextState::Closed) {
> +    promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return promise.forget();

Shouldn't the promise succeed in this case?

::: dom/media/webaudio/test/test_audioContextSuspendResumeClose.html
@@ +144,5 @@
> +
> +
> +// Test that there is no buffering between contexts when connecting a running
> +// AudioContext to a suspended AudioContext. Our ScriptProcessorNode does some
> +// buffering internaly, so we ensure this by using a very very low frequency

"internally"

@@ +145,5 @@
> +
> +// Test that there is no buffering between contexts when connecting a running
> +// AudioContext to a suspended AudioContext. Our ScriptProcessorNode does some
> +// buffering internaly, so we ensure this by using a very very low frequency
> +// on a sinus, and oberve that the phase has changed by a big enough margin.

"on a sine"

Comment 8

[Paul Adenot (:padenot)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Comment on attachment 8570567 [details] [diff] [review]
> Implement AudioContext.suspend and friends. r=
> 
> Review of attachment 8570567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/TrackUnionStream.cpp
> @@ +85,5 @@
> >      bool allHaveCurrentData = !mInputs.IsEmpty();
> >      for (uint32_t i = 0; i < mInputs.Length(); ++i) {
> >        MediaStream* stream = mInputs[i]->GetSource();
> > +      if (GraphImpl()->StreamSuspended(stream)) {
> > +        mBuffer.FindTrack(2)->GetSegment()->AppendNullData(aTo - aFrom);
> 
> FindTrack(2) looks bad :-(. Having to check for suspended streams here is
> bad too.

Hrm, I forgot to qref, this is using the proper track id, locally: https://dxr.mozilla.org/mozilla-central/source/dom/media/webaudio/MediaStreamAudioDestinationNode.cpp#26. Note that the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1081819 will change this.

> In fact, having these special suspended streams is unfortunate.
> Are you sure that it's not possible to implement this by permanently
> blocking the suspended streams?

This is what I tried first. The blocking mechanism, in its current form, is not quite what we want, here:
- We want "suspend" to affect all the nodes from a graph, not all nodes connected down to other nodes, and we don't want blocking propagation to cross AudioContext boundaries
- We want no buffering to occur at the AudioContext edges, data should be dropped
- We want minimal CPU cost associated with the suspended streams, it seems unnecessary to even order the suspended streams.

Also, blocking and unbounded buffering in general are not concepts that are very useful or desirable in a lot of use cases the MSG solves today (Web Audio and WebRTC deal with real-time media where we'd rather drop data than buffer), and are actually expensive (in CPU and memory), they show up in profiles. As I said last time in Portland, I'd like to maybe reconsider some architectural choices in the MSG (the notion of blocking being one of them), so I did not write this on top of the blocking mechanism.


> @@ +779,5 @@
> > +             (mAudioContextState == AudioContextState::Suspended &&
> > +              aNewState == AudioContextState::Closed)    ||
> > +             (mAudioContextState == AudioContextState::Closed &&
> > +              aNewState == AudioContextState::Closed),
> > +             "Invalid AudioContextState transition");
> 
> We can simplify this by checking mAudioContextState == aNewState, which must
> obviously always be legal.
> 
> Should we be firing statechanged when the state hasn't actually changed?

The spec says to fire statechanged "when the corresponding promise would have resolved", so yes.

> @@ +809,5 @@
> > +    return nullptr;
> > +  }
> > +  if (mIsOffline) {
> > +    promise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> > +    return promise.forget();
> 
> The spec says "This method will cause an INVALID_STATE_ERR exception to be
> thrown if called on an OfflineAudioContext." which I think means we need to
> throw in aRv here instead of in the promise.
> 
> @@ +847,5 @@
> > +  }
> > +
> > +  if (mIsOffline) {
> > +    promise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> > +    return promise.forget();
> 
> Likewise, I think this needs to throw instead of rejecting the promise.
> 
> @@ +885,5 @@
> > +  }
> > +
> > +  if (mIsOffline) {
> > +    promise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> > +    return promise.forget();
> 
> Here too

Those three are errors in the current spec, there is an issue about it. I completely forgot to mention that, sorry:
https://github.com/WebAudio/web-audio-api/issues/441

I'll write and merge the spec patch soon, so I figured I'd just write the version with promises.

> 
> @@ +890,5 @@
> > +  }
> > +
> > +  if (mAudioContextState == AudioContextState::Closed) {
> > +    promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);
> > +    return promise.forget();
> 
> Shouldn't the promise succeed in this case?

Ha yes, close is idempotent, fixed.

Comment 9

[Paul Adenot (:padenot)]

Attachment: interdiff addressing roc's comments

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Paul Adenot (:padenot) from comment #8)
> > In fact, having these special suspended streams is unfortunate.
> > Are you sure that it's not possible to implement this by permanently
> > blocking the suspended streams?
> 
> This is what I tried first. The blocking mechanism, in its current form, is
> not quite what we want, here:
> - We want "suspend" to affect all the nodes from a graph, not all nodes
> connected down to other nodes, and we don't want blocking propagation to
> cross AudioContext boundaries

I don't think these would be problems. We would individually block all the streams for a given AudioContext, and you can only cross AudioContext boundaries via a MediaStreamAudioDestinationNode, which can be configured so its output MediaStream does not block when the AudioNode's stream blocks.

> - We want no buffering to occur at the AudioContext edges, data should be
> dropped

This shouldn't be a problem either. Data feeding from an unblocked to blocked stream is dropped, not buffered.

> - We want minimal CPU cost associated with the suspended streams, it seems
> unnecessary to even order the suspended streams.

True, but I'm not sure that that's significant.

> Also, blocking and unbounded buffering in general are not concepts that are
> very useful or desirable in a lot of use cases the MSG solves today (Web
> Audio and WebRTC deal with real-time media where we'd rather drop data than
> buffer), and are actually expensive (in CPU and memory), they show up in
> profiles.

Blocking doesn't cause unbounded buffering currently. In fact, we don't do unbounded buffering anywhere (unless something generating a SourceMediaStream fails to throttle correctly).

> As I said last time in Portland, I'd like to maybe reconsider some
> architectural choices in the MSG (the notion of blocking being one of them),
> so I did not write this on top of the blocking mechanism.

I think we should figure out now whether we're going to get rid of blocking or not. If we are, then the approach here makes sense, but if not, then I suspect we should use blocking here.

I'll post to dev-media.

Comment 11

[(no longer active)]

Comment on attachment 8570567 [details] [diff] [review]
Implement AudioContext.suspend and friends. r=

Review of attachment 8570567 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +13015,5 @@
>      // Suspend all of the AudioContexts for this window
>      for (uint32_t i = 0; i < mAudioContexts.Length(); ++i) {
> +      ErrorResult dummy;
> +      nsRefPtr<Promise> p = mAudioContexts[i]->Suspend(dummy);
> +      p = nullptr;

This is unneeded.

@@ +13077,5 @@
>      // Resume all of the AudioContexts for this window
>      for (uint32_t i = 0; i < mAudioContexts.Length(); ++i) {
> +      ErrorResult dummy;
> +      nsRefPtr<Promise> p = mAudioContexts[i]->Resume(dummy);
> +      p = nullptr;

Ditto.

::: dom/media/webaudio/AudioContext.cpp
@@ +737,5 @@
> +  }
> +
> +  mAudioContext->OnStateChanged(mPromise, mNewState);
> +
> +  mAudioContext = nullptr;

Why do you need to do this?  If this is really needed, it could use a comment explaining why.

@@ +779,5 @@
> +             (mAudioContextState == AudioContextState::Suspended &&
> +              aNewState == AudioContextState::Closed)    ||
> +             (mAudioContextState == AudioContextState::Closed &&
> +              aNewState == AudioContextState::Closed),
> +             "Invalid AudioContextState transition");

According to the spec, we should only fire this event when the state has actually changed.

@@ +809,5 @@
> +    return nullptr;
> +  }
> +  if (mIsOffline) {
> +    promise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return promise.forget();

roc is right.  In fact, you should create the promise after performing these checks, and return nullptr here.

@@ +815,5 @@
> +
> +  if (mAudioContextState == AudioContextState::Closed ||
> +      mCloseCalled) {
> +    promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return promise.forget();

Ditto.

@@ +820,5 @@
> +  }
> +
> +  if (mAudioContextState == AudioContextState::Suspended) {
> +    promise->MaybeResolve(JS::UndefinedHandleValue);
> +    mPromiseGripArray.RemoveElement(promise);

You just created the promise!  How can it exist in mPromiseGripArray?

@@ +830,4 @@
>    }
> +
> +  mPromiseGripArray.AppendElement(promise);
> +  Graph()->ApplyAudioContextOperation(DestinationStream()->AsAudioNodeStream(),

ApplyAudioContextOperation() should take its promise as a void* (see my comment in AudioContext.h)

@@ +853,5 @@
> +
> +  if (mAudioContextState == AudioContextState::Closed ||
> +      mCloseCalled) {
> +    promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return promise.forget();

Same comment as before.

@@ +858,5 @@
> +  }
> +
> +  if (mAudioContextState == AudioContextState::Running) {
> +    promise->MaybeResolve(JS::UndefinedHandleValue);
> +    mPromiseGripArray.RemoveElement(promise);

This cannot happen either.

@@ +862,5 @@
> +    mPromiseGripArray.RemoveElement(promise);
> +  }
> +
> +  MediaStream* ds = DestinationStream();
> +  if (ds) {

Can ds be null here?

@@ +890,5 @@
> +  }
> +
> +  if (mAudioContextState == AudioContextState::Closed) {
> +    promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return promise.forget();

The spec seems to be silent on what to do here.

@@ +900,5 @@
> +  Graph()->ApplyAudioContextOperation(DestinationStream()->AsAudioNodeStream(),
> +                                      AudioContextOperation::Close, promise);
> +
> +  MediaStream* ds = DestinationStream();
> +  if (ds) {

Can ds be null here?

::: dom/media/webaudio/AudioContext.h
@@ +19,5 @@
>  #include "nsHashKeys.h"
>  #include "nsTHashtable.h"
>  #include "js/TypeDecls.h"
>  #include "nsIMemoryReporter.h"
> +#include "mozilla/dom/AudioContextBinding.h"

Why do you need to include this header here?  It would be nice if you could avoid that.

@@ +71,5 @@
> + * flowing */
> +class StateChangeTask MOZ_FINAL : public nsRunnable
> +{
> +public:
> +  /* This constructor should be used when this even is sent from the main

Nit: event

@@ +79,5 @@
> +  /* This constructor should be used when this even is sent from the audio
> +   * thread. */
> +  StateChangeTask(AudioNodeStream* aStream, Promise* aPromise, AudioContextState aNewState);
> +
> +  NS_IMETHOD Run();

Nit: MOZ_OVERRIDE.

@@ +83,5 @@
> +  NS_IMETHOD Run();
> +
> +private:
> +  nsRefPtr<AudioContext> mAudioContext;
> +  Promise* mPromise;

This is very unsafe.  I think you are doing this in order to avoid addrefing the promise object on the MSG thread.  In that case, the only way this could be relatively safe is if you did something such as make this a void* and add thread checks everywhere that you need to cast it back to a Promise*.

@@ +89,5 @@
> +  AudioContextState mNewState;
> +};
> +
> +/* This runnable allows to fire the "statechange" event */
> +class OnStateChangeTask MOZ_FINAL : public nsRunnable

I think this class can be moved to AudioContext.cpp.

@@ +92,5 @@
> +/* This runnable allows to fire the "statechange" event */
> +class OnStateChangeTask MOZ_FINAL : public nsRunnable
> +{
> +public:
> +  OnStateChangeTask(AudioContext* aAudioContext, AudioContextState aNewState)

What's the point of passing in aNewState?

@@ +96,5 @@
> +  OnStateChangeTask(AudioContext* aAudioContext, AudioContextState aNewState)
> +    : mAudioContext(aAudioContext)
> +  {}
> +
> +  NS_IMETHOD Run();

Nit: MOZ_OVERRIDE.

@@ +176,5 @@
> +  already_AddRefed<Promise> Suspend(ErrorResult& aRv);
> +  already_AddRefed<Promise> Resume(ErrorResult& aRv);
> +  already_AddRefed<Promise> Close(ErrorResult& aRv);
> +  IMPL_EVENT_HANDLER(statechange)
> +  void OnStateChanged(Promise* aPromise, AudioContextState aNewState);

Please separate OnStateChanged from the WebIDL methods here.

::: dom/webidl/AudioContext.webidl
@@ +28,5 @@
>      readonly attribute double currentTime;
>      readonly attribute AudioListener listener;
> +    readonly attribute AudioContextState state;
> +    [Throws]
> +    Promise<void> suspend();

Can you please make sure the spec is updated with the correct promise syntax?

Comment 12

[Paul Adenot (:padenot)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11)
> Comment on attachment 8570567 [details] [diff] [review]
> Implement AudioContext.suspend and friends. r=
>
> Review of attachment 8570567 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +809,5 @@
> > +    return nullptr;
> > +  }
> > +  if (mIsOffline) {
> > +    promise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> > +    return promise.forget();
> 
> roc is right.  In fact, you should create the promise after performing these
> checks, and return nullptr here.

Quoting comment 8

> Those three are errors in the current spec, there is an issue about it. I completely forgot to 
> mention that, sorry:
> https://github.com/WebAudio/web-audio-api/issues/441
>
> I'll write and merge the spec patch soon, so I figured I'd just write the version with promises.

> @@ +858,5 @@
> > +  }
> > +
> > +  if (mAudioContextState == AudioContextState::Running) {
> > +    promise->MaybeResolve(JS::UndefinedHandleValue);
> > +    mPromiseGripArray.RemoveElement(promise);
> 
> This cannot happen either.
> 
> @@ +862,5 @@
> > +    mPromiseGripArray.RemoveElement(promise);
> > +  }
> > +
> > +  MediaStream* ds = DestinationStream();
> > +  if (ds) {
> 
> Can ds be null here?

I don't think it can, I kind of copied this from the existing code.

> @@ +890,5 @@
> > +  }
> > +
> > +  if (mAudioContextState == AudioContextState::Closed) {
> > +    promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);
> > +    return promise.forget();
> 
> The spec seems to be silent on what to do here.

It does not say to do anything particular, as roc mentioned, so I just resolved
the promise (in the interdiff). I can write a patch to the spec stating
explicitly that close is idempotent.

> ::: dom/media/webaudio/AudioContext.h
> @@ +19,5 @@
> >  #include "nsHashKeys.h"
> >  #include "nsTHashtable.h"
> >  #include "js/TypeDecls.h"
> >  #include "nsIMemoryReporter.h"
> > +#include "mozilla/dom/AudioContextBinding.h"
> 
> Why do you need to include this header here?  It would be nice if you could avoid that.

We need it for the enum. This seem to be what is done for other headers that
need a WebIDL enum.

> @@ +83,5 @@
> > +  NS_IMETHOD Run();
> > +
> > +private:
> > +  nsRefPtr<AudioContext> mAudioContext;
> > +  Promise* mPromise;
> 
> This is very unsafe.  I think you are doing this in order to avoid addrefing
> the promise object on the MSG thread.  In that case, the only way this could
> be relatively safe is if you did something such as make this a void* and add
> thread checks everywhere that you need to cast it back to a Promise*.

I did what you mention, moving only a void* around, and reinterpret_cast-ing it
when it's back on the main thread, right below a MOZ_ASSERT(NS_IsMainThread()),
with a debug-only check that it was found in the promise array for extra safety.
I agree that it was not particularly smart to pass the right interface around.

> ::: dom/webidl/AudioContext.webidl
> @@ +28,5 @@
> >      readonly attribute double currentTime;
> >      readonly attribute AudioListener listener;
> > +    readonly attribute AudioContextState state;
> > +    [Throws]
> > +    Promise<void> suspend();
> 
> Can you please make sure the spec is updated with the correct promise syntax?

Yes, filed https://github.com/WebAudio/web-audio-api/issues/488

Comment 13

[Paul Adenot (:padenot)]

Attachment: Implement AudioContext.suspend and friends. r=

This patch has all comments addressed. It seems to me that we will ending up
ripping blocking out of the MSG, so we can maybe go forward here?

Comment 14

[Paul Adenot (:padenot)]

Attachment: interdiff addressing ehsan's comments

Comment 15

[(no longer active)]

Comment on attachment 8578657 [details] [diff] [review]
Implement AudioContext.suspend and friends. r=

Review of attachment 8578657 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webaudio/AudioContext.h
@@ +6,5 @@
>  
>  #ifndef AudioContext_h_
>  #define AudioContext_h_
>  
> +#include "mozilla/dom/AudioContextBinding.h"

Can you avoid this by forward declaring AudioContextState?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8578657 [details] [diff] [review]
Implement AudioContext.suspend and friends. r=

Review of attachment 8578657 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. This basically looks great. I have a few questions but if you address them one way or another, r+

The way we pipe promises through the MSG thread is not very nice. Can you write a comment in the code somewhere that explains how that works, how we guarantee that the promises are always cleaned up even during shutdown, and why it's best to do it that way? I'd like to think of a better way to do it, but I can't :-).

::: dom/media/MediaStreamGraph.cpp
@@ +556,5 @@
> +MediaStreamGraphImpl::StreamSuspended(MediaStream* aStream)
> +{
> +  // Only AudioNodeStreams can be suspended, so we can shortcut here.
> +  return aStream->AsAudioNodeStream() &&
> +         mSuspendedStreams.IndexOf(aStream) != mSuspendedStreams.NoIndex;

We should really have a flag on MediaStream so this isn't O(N).

@@ +1498,5 @@
>    UpdateCurrentTimeForStreams(aFrom, aTo);
>  
>    UpdateGraph(aStateEnd);
>  
> +  printf("mStreams: %ld, mSuspendedStreams: %ld\n", mStreams.Length(), mSuspendedStreams.Length());

delete

@@ +3244,5 @@
> +                                  mozilla::LinkedList<MediaStream>& aStreamSet)
> +{
> +   nsTArray<MediaStream*>* runningAndSuspendedPair[2];
> +   runningAndSuspendedPair[0] = &mStreams;
> +   runningAndSuspendedPair[1] = &mSuspendedStreams;

Fix indent

@@ +3329,5 @@
> +
> +void
> +MediaStreamGraphImpl::ApplyAudioContextOperationImpl(AudioNodeStream* aStream,
> +                                               AudioContextOperation aOperation,
> +                                               void* aPromise)

fix indent

@@ +3351,5 @@
> +  // track. When resuming, force switching to an AudioCallbackDriver. It would
> +  // have happened at the next iteration anyways, but doing this now save
> +  // some time.
> +  if (aOperation == AudioContextOperation::Resume) {
> +    if(!CurrentDriver()->AsAudioCallbackDriver()) {

Space after "if"

::: dom/media/webaudio/AudioContext.cpp
@@ +796,5 @@
> +
> +  if (mAudioContextState != aNewState) {
> +    nsRefPtr<OnStateChangeTask> onStateChangeTask =
> +      new OnStateChangeTask(this);
> +    NS_DispatchToMainThread(onStateChangeTask);

Currently the spec says to fire the event "when the corresponding promise would have resolved". Here you're queuing a task to fire the event, which technically is different --- though maybe the intent of the spec is to queue a task, and the spec just needs to be fixed?

I think we could fire the event here. We'd just need to ensure that AudioContext::OnStateChanged is called when it's safe to fire the event, which I think is already true. But that would meaning firing the event during a startRendering() call, which while safe is probably not good style.

::: dom/media/webaudio/MediaStreamAudioDestinationNode.h
@@ +11,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +static const int MEDIA_STREAM_DEST_TRACK_ID = 2;

This is not used.

Comment 17

[Paul Adenot (:padenot)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> ::: dom/media/MediaStreamGraph.cpp
> @@ +556,5 @@
> > +MediaStreamGraphImpl::StreamSuspended(MediaStream* aStream)
> > +{
> > +  // Only AudioNodeStreams can be suspended, so we can shortcut here.
> > +  return aStream->AsAudioNodeStream() &&
> > +         mSuspendedStreams.IndexOf(aStream) != mSuspendedStreams.NoIndex;
> 
> We should really have a flag on MediaStream so this isn't O(N).

Ha yes. I wrote when it was only used in one case (cross-AudioContext connections, that I expect to be rare), and I thought the CPU/memory tradeoff was worth it, but now it's in the ordering algorithm so it's a bit different.

> ::: dom/media/webaudio/AudioContext.cpp
> @@ +796,5 @@
> > +
> > +  if (mAudioContextState != aNewState) {
> > +    nsRefPtr<OnStateChangeTask> onStateChangeTask =
> > +      new OnStateChangeTask(this);
> > +    NS_DispatchToMainThread(onStateChangeTask);
> 
> Currently the spec says to fire the event "when the corresponding promise
> would have resolved". Here you're queuing a task to fire the event, which
> technically is different --- though maybe the intent of the spec is to queue
> a task, and the spec just needs to be fixed?

Yes, that's one of the comments I'm making on the current text indeed.

Comment 18

[Paul Adenot (:padenot)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #15)
> Comment on attachment 8578657 [details] [diff] [review]
> Implement AudioContext.suspend and friends. r=
> 
> Review of attachment 8578657 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/webaudio/AudioContext.h
> @@ +6,5 @@
> >  
> >  #ifndef AudioContext_h_
> >  #define AudioContext_h_
> >  
> > +#include "mozilla/dom/AudioContextBinding.h"
> 
> Can you avoid this by forward declaring AudioContextState?

Ah yes indeed.

Comment 19

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf22c9e5c5a3

Comment 20

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8228425&repo=mozilla-inbound

Comment 21

[Ryan VanderMeulen [:RyanVM]]

This was leaking too:
https://treeherder.mozilla.org/logviewer.html#?job_id=8232150&repo=mozilla-inbound

Comment 22

[Paul Adenot (:padenot)]

Yeah something keeps the AudioContext alive I think, for both type of failure, not sure what.

Comment 23

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc52c48e6e3

Comment 24

[Carsten Book [:Tomcat]]

sorry had to back this out again for mulet m3 perma failure like https://treeherder.mozilla.org/logviewer.html#?job_id=8647881&repo=mozilla-inbound

Comment 25

[Paul Adenot (:padenot)]

A bit of an update on this.

This fails (segfaults) only on mulet/linux during random webrtc tests. Since the task cluster switch, it seems that the new environment is not configured properly (PulseAudio is not started, for example), and there are no stacks after a process crashes, because of other unrelated task cluster issues.

Obviously it passes locally on mulet (since it's kind of just a normal linux build), and I've talked to the mulet people, they agreed that there are too much problems with real-time media tests on mulet, and that I should disable broken tests and land this. Once they have all their infra bugs fixed, we'll be able to re-enable (it's likely to not be very difficult, since the same test and code passes reliably on very similar platforms).

Comment 26

[(no longer active)]

Sounds good to me!

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82bbdffc624b

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/82bbdffc624b

Comment 29

[Chris Mills [:cmills]]

These new features have now been documented:

https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/state
https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/onstatechange
https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/close
https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/resume
https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/suspend

https://developer.mozilla.org/en-US/Firefox/Releases/40#Web_Audio_API

A tech review would be great; thanks!