Implement suspend/resume for AudioContext

Implement suspend/resume for AudioContext

WebAudio is adding a suspend()/resume() API:

https://github.com/WebAudio/web-audio-api/issues/361
https://github.com/WebAudio/web-audio-api/issues/317

In more detail, here is the proposed additions to the spec:

New attribute "state" with values:
paused
  Currently paused (time is not proceeding, audio hardware may be powered down/released).
running
  Audio is being processed.
released
  AudioContext has been released, and can no longer be used to process audio. All system resources should be released.

void suspend()
Suspends the progression of time in the audio context, allows any current buffer contents to be played to the destination and then allows the system to power down and/or release audio hardware. If the context has been released, an InvalidStateError MUST be thrown. This is generally useful when the application knows it will not need the AudioContext for some time, and wishes to let the audio hardware power down.

While the system is suspend, MediaStreams will have their output ignored; that is, data will be lost by the real time nature of media streams. HTMLMediaElements will similarly have their output ignored until the system is resumed. Audio Workers and ScriptProcessorNodes will simply not fire their onaudioprocess events while suspended, but will resume when resumed. For the purpose of AnalyserNode window functions, the data is considered as a continuous stream - i.e. the resume()/suspend() does not cause silence to appear in the AnalyserNode's stream of data.

Promise resume()
Resumes the progression of time in the audio context, which may involve re-priming the frame buffer contents. The promise resolves when the system has re-acquired (if necessary) access to audio hardware and has begun streaming to the destination, or immediately (with no other effect) if the context is already running. The promise is rejected if the context has been released.

BUG=420106
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183916

[Raymond Toy, 2014-10-07 17:09:41]

rtoy@chromium.org changed reviewers:
+ haraken@chromium.org

[Raymond Toy, 2014-10-07 17:09:42]

PTAL

[haraken, 2014-10-08 00:31:36]

haraken@chromium.org changed reviewers:
+ yhirano@chromium.org

[haraken, 2014-10-08 00:31:37]

hirano-san: Would you take a look at the Promise part of this change?

[yhirano, 2014-10-08 01:12:14]

https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:846: ASSERT(isMainThread);
Please acquire the lock.

https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:858: 
Please add a comment that the lock is already acquired.

https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.h:176: void
resolvePromisesForResumeOnMainThread();
These function can be private.

[yhirano, 2014-10-08 01:15:54]

https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:846: ASSERT(isMainThread);
On 2014/10/08 01:12:14, yhirano wrote:
> Please acquire the lock.

Oh, I found AudioContext::lock is not callable from the main thread. You need to
protect shared member variables though. Maybe manipulating these variables in
the audio thread and passing them to this function via parameters works?

[Raymond Toy, 2014-10-09 17:50:38]

On 2014/10/08 at 01:15:54, yhirano wrote:
>
https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
> File Source/modules/webaudio/AudioContext.cpp (right):
> 
>
https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
> Source/modules/webaudio/AudioContext.cpp:846: ASSERT(isMainThread);
> On 2014/10/08 01:12:14, yhirano wrote:
> > Please acquire the lock.
> 
> Oh, I found AudioContext::lock is not callable from the main thread. You need
to protect shared member variables though. Maybe manipulating these variables in
the audio thread and passing them to this function via parameters works?

The audio thread doesn't touch m_resumePromises vector. It only sets the flag
m_isResolvingResumePromises and checks the size of m_resumePromises.

I can add a Autolocker lock in resolvePromisesForResumeOnMainThread(). That
should prevent the audio thread from unexpectedly changing
m_isResolvingResumePromises, since the audio thread won't run if the main thread
as an Autolocker lock.

[yhirano, 2014-10-14 04:40:50]

On 2014/10/09 17:50:38, Raymond Toy wrote:
> On 2014/10/08 at 01:15:54, yhirano wrote:
> >
>
https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
> > File Source/modules/webaudio/AudioContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
> > Source/modules/webaudio/AudioContext.cpp:846: ASSERT(isMainThread);
> > On 2014/10/08 01:12:14, yhirano wrote:
> > > Please acquire the lock.
> > 
> > Oh, I found AudioContext::lock is not callable from the main thread. You
need
> to protect shared member variables though. Maybe manipulating these variables
in
> the audio thread and passing them to this function via parameters works?
> 
> The audio thread doesn't touch m_resumePromises vector. It only sets the flag
> m_isResolvingResumePromises and checks the size of m_resumePromises.
Even so, I would like a lock here, because accessing to shared variables without
locks confuses the threads and confuses readers.

> I can add a Autolocker lock in resolvePromisesForResumeOnMainThread(). That
> should prevent the audio thread from unexpectedly changing
> m_isResolvingResumePromises, since the audio thread won't run if the main
thread
> as an Autolocker lock.
That sounds reasonable.

[Raymond Toy, 2014-10-14 17:16:59]

https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:846: ASSERT(isMainThread);
On 2014/10/08 01:12:14, yhirano wrote:
> Please acquire the lock.

Done.

https://codereview.chromium.org/625363004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:858: 
On 2014/10/08 01:12:14, yhirano wrote:
> Please add a comment that the lock is already acquired.

Done.

[Raymond Toy, 2014-10-14 17:19:20]

yhirano: PTAL and the locking and ScriptPromise items again.
haraken: PTAL at the overall CL. I think I got the oilpan stuff implemented
correctly.

[yhirano, 2014-10-15 09:02:27]

lgtm

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:898: m_resumePromises[k]->reject();
[opt] In general, it would be better to reject a promise with an Exception. That
being said, it seems that the spec doesn't state that so it's OK now.

[haraken, 2014-10-15 15:36:26]

The approach looks good.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:173:
m_destinationNode->startRendering();

You can call startRendering() instead of m_destinationNode->startRendering().

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:174: setState(Running);

Then you can remove this. It's important to reduce the call sites of setState to
avoid mistakes :)

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:551: return "running";

Nit: I'd return "".

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:565: break;

Shall we add ASSERT(m_state == Released) ?

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:579: "cannot suspend an AudioContext
that has been released");

Nit: You can immediately return here. Then you don't need the following 'else'.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:583: setState(Paused);

Probably you want to implement a helper method for AudioContext::stopRendering
for consistency with AudioContext::startRendering.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:606:
m_destinationNode->startRendering();

You can call startRendering() instead of m_destinationNode->startRendering().

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:607: setState(Running);

Then you can remove this. We want to reduce the call sites of setState in order
to avoid mistakes.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:909: // This is run inside the
AudioContext's lock when handling pre-render tasks.

Nit: is run => runs

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:910: ASSERT(isAudioThread());

Shall we add ASSERT(isGraphOwner()) ?

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:294:
GC_PLUGIN_IGNORE("http://crbug.com/404527")

ScriptPromiseResolver is not yet moved to Oilpan, so you don't need this macro.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
File Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioDestinationNode.h:59: virtual void stop() = 0;

stop => stopRendering (for consistency with startRendering) ?

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
File Source/modules/webaudio/DefaultAudioDestinationNode.cpp (right):

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/DefaultAudioDestinationNode.cpp:102: }

Can we add an ASSERT to verify that start() and stop() are balanced? (i.e.,
start() is not called twice in a row etc)

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
File Source/modules/webaudio/OfflineAudioDestinationNode.h (right):

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/OfflineAudioDestinationNode.h:55: virtual void stop()
override { };

Nit: ';' is not needed.

[Raymond Toy, 2014-10-15 16:31:56]

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:565: break;
On 2014/10/15 15:36:26, haraken wrote:
> 
> Shall we add ASSERT(m_state == Released) ?

When the close() method is implemented, it will be possible to transition to
Released from the Paused or Running state. But it's probably good to check that
m_state != Released. We shouldn't be releasing twice.

[Raymond Toy, 2014-10-15 19:50:50]

PTAL. I think I've addressed all of your comments. I've also added some comments
on the meaning of AudioContextState and the valid transitions of the state.  And
renamed m_state to m_contextState since the former seems overly generic.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:173:
m_destinationNode->startRendering();
On 2014/10/15 15:36:26, haraken wrote:
> 
> You can call startRendering() instead of m_destinationNode->startRendering().

Done.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:174: setState(Running);
On 2014/10/15 15:36:26, haraken wrote:
> 
> Then you can remove this. It's important to reduce the call sites of setState
to
> avoid mistakes :)

Done.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:551: return "running";
On 2014/10/15 15:36:26, haraken wrote:
> 
> Nit: I'd return "".

Done.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:579: "cannot suspend an AudioContext
that has been released");
On 2014/10/15 15:36:26, haraken wrote:
> 
> Nit: You can immediately return here. Then you don't need the following
'else'.

Done.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:583: setState(Paused);
On 2014/10/15 15:36:26, haraken wrote:
> 
> Probably you want to implement a helper method for AudioContext::stopRendering
> for consistency with AudioContext::startRendering.

Done.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:606:
m_destinationNode->startRendering();
On 2014/10/15 15:36:26, haraken wrote:
> 
> You can call startRendering() instead of m_destinationNode->startRendering().
> 

Done.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:607: setState(Running);
On 2014/10/15 15:36:26, haraken wrote:
> 
> Then you can remove this. We want to reduce the call sites of setState in
order
> to avoid mistakes.

Done.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:898: m_resumePromises[k]->reject();
On 2014/10/15 09:02:27, yhirano wrote:
> [opt] In general, it would be better to reject a promise with an Exception.
That
> being said, it seems that the spec doesn't state that so it's OK now.

Good idea. I've added an Exception.  The spec isn't quite finalized yet, and
we'll probably propose this addition.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:909: // This is run inside the
AudioContext's lock when handling pre-render tasks.
On 2014/10/15 15:36:26, haraken wrote:
> 
> Nit: is run => runs

Done.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:910: ASSERT(isAudioThread());
On 2014/10/15 15:36:26, haraken wrote:
> 
> Shall we add ASSERT(isGraphOwner()) ?

Done.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
File Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/AudioDestinationNode.h:59: virtual void stop() = 0;
On 2014/10/15 15:36:26, haraken wrote:
> 
> stop => stopRendering (for consistency with startRendering) ?

Done.

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
File Source/modules/webaudio/DefaultAudioDestinationNode.cpp (right):

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/DefaultAudioDestinationNode.cpp:102: }
On 2014/10/15 15:36:26, haraken wrote:
> 
> Can we add an ASSERT to verify that start() and stop() are balanced? (i.e.,
> start() is not called twice in a row etc)

Added a check that destination is playing when calling stop() and destination is
not playing when calling start().

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
File Source/modules/webaudio/OfflineAudioDestinationNode.h (right):

https://codereview.chromium.org/625363004/diff/170001/Source/modules/webaudio...
Source/modules/webaudio/OfflineAudioDestinationNode.h:55: virtual void stop()
override { };
On 2014/10/15 15:36:26, haraken wrote:
> 
> Nit: ';' is not needed.

Done.

[haraken, 2014-10-16 01:13:44]

LGTM

This needs API owner review.

https://codereview.chromium.org/625363004/diff/220011/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/625363004/diff/220011/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:143: AudioContext::~AudioContext()

Can we add ASSERT(!m_resumePromises.size())?

https://codereview.chromium.org/625363004/diff/220011/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:938: ASSERT(isMainThread());

Shall we add ASSERT(m_destinationNode)?

https://codereview.chromium.org/625363004/diff/220011/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:938: ASSERT(isMainThread());

Shall we add ASSERT(!isOfflineContext())?

[Raymond Toy, 2014-10-16 17:56:31]

https://codereview.chromium.org/625363004/diff/220011/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/625363004/diff/220011/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:143: AudioContext::~AudioContext()
On 2014/10/16 01:13:43, haraken wrote:
> 
> Can we add ASSERT(!m_resumePromises.size())?

Done.

https://codereview.chromium.org/625363004/diff/220011/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:938: ASSERT(isMainThread());
On 2014/10/16 01:13:43, haraken wrote:
> 
> Shall we add ASSERT(!isOfflineContext())?

Actually no.  startRendering is called for both online and offline contexts from
V8.

https://codereview.chromium.org/625363004/diff/220011/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:938: ASSERT(isMainThread());
On 2014/10/16 01:13:43, haraken wrote:
> 
> Shall we add ASSERT(m_destinationNode)?

Done.

[Raymond Toy, 2014-10-16 17:59:43]

rtoy@chromium.org changed reviewers:
+ tkent@chromium.org

[Raymond Toy, 2014-10-16 17:59:45]

tkent@: can you review this as an API owner?

[tkent, 2014-10-17 00:37:18]

lgtm.

I don't think we need "Intent to ship" for this feature. However, I recommend to
add an entry to www.chromestatus.com, and inform that the feature will be
shipped with M40 in the entry.

[Raymond Toy, 2014-10-17 15:45:29]

On 2014/10/17 at 00:37:18, tkent wrote:
> lgtm.
> 
> I don't think we need "Intent to ship" for this feature. However, I recommend
to add an entry to www.chromestatus.com, and inform that the feature will be
shipped with M40 in the entry.

www.chromestatus.com updated.

[Raymond Toy, 2014-10-17 15:45:58]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2014-10-17 15:45:59]

Thanks, everyone, for your reviews!

[commit-bot: I haz the power, 2014-10-17 15:47:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625363004/260001

[commit-bot: I haz the power, 2014-10-17 17:56:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-17 17:56:18]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/32332)

[Raymond Toy, 2014-10-17 18:02:12]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2014-10-17 18:02:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625363004/260001

[commit-bot: I haz the power, 2014-10-17 19:25:04]

Committed patchset #13 (id:260001) as 183916

[dstockwell, 2014-10-19 23:34:14]

A revert of this CL (patchset #13 id:260001) has been created in
https://codereview.chromium.org/663003002/ by dstockwell@chromium.org.

The reason for reverting is: Breaks webaudio/oscillator-sine.html on Windows.