Split "Online" and "Offline" AudioContext processing

Split "Online" and "Offline" AudioContext processing

Web Audio has two kinds of contexts: One that drives real sounds out
of something (called AudioContext) and one that renders into a buffer
for JavaScript (called OfflineAudioContext.) Blink's implementation is
kind of tangled into AudioContext, with isOffline() checks here and
there.

The two kinds of AudioContexts actually live pretty different lives,
though. For example, OfflineAudioContexts are manually started and closed
by a DOM event; online AudioContexts are automatically started and can be
closed by script and the ExecutionContext.

This starts to separate the implementation of these two AudioContexts
This will enable things like treating these as separate kinds of
ActiveDOMObject which will mean they can be reclaimed sooner and
not do unusual things like multiple self-inflicted calls to
ActiveDOMObject::stop.

BUG=279044, 503845
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198828

[dominicc (has gone to gerrit), 2015-06-26 08:21:50]

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

[dominicc (has gone to gerrit), 2015-06-26 08:22:03]

PTAL

[Ken Russell (switch to Gerrit), 2015-06-26 15:45:08]

kbr@chromium.org changed reviewers:
+ hongchan@chromium.org, kbr@chromium.org

[Ken Russell (switch to Gerrit), 2015-06-26 15:45:08]

+hongchan

Hongchan, can you review this? I'll be glad to review or rubber-stamp afterward.

[hongchan, 2015-06-26 15:49:12]

On 2015/06/26 at 15:45:08, kbr wrote:
> +hongchan
> 
> Hongchan, can you review this? I'll be glad to review or rubber-stamp
afterward.

dominicc@

I really like the idea of this CL, but actually I wish you can land this CL
after the one that I have been working on for a while.

https://codereview.chromium.org/1140723003

This CL implements suspend() and resume() for OAC.

[Raymond Toy, 2015-06-27 15:38:11]

Just a few drive-by comments. I won't be able review this until I get back.

https://codereview.chromium.org/1214463003/diff/1/Source/modules/webaudio/Onl...
File Source/modules/webaudio/OnlineAudioContext.h (right):

https://codereview.chromium.org/1214463003/diff/1/Source/modules/webaudio/Onl...
Source/modules/webaudio/OnlineAudioContext.h:21: class OnlineAudioContext :
public AudioContext {
Would it be possible in this CL to rename AudioContext (orig) to
AbstractAudioContext and use AudioContext instead of OnlineAudioContext?

https://codereview.chromium.org/1214463003/diff/1/Source/platform/audio/Reverb.h
File Source/platform/audio/Reverb.h (right):

https://codereview.chromium.org/1214463003/diff/1/Source/platform/audio/Rever...
Source/platform/audio/Reverb.h:48: Reverb(AudioBus* impulseResponseBuffer,
size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels, bool
hasRealtimeConstraint, bool normalize);
DBC:  I think useBackgroundThreads is clearer than hasRealtimeConstraints. 
While we don't allow it today, it seems possible that we might want multiple
threads for the reverb, even for offline contexts.

Same comment for ReverbConvolver.h

[dominicc (has gone to gerrit), 2015-07-09 02:11:01]

rtoy: Thanks for the comments; I will make it so.

hongchan: Thank you for the link to your patch. I don't think we should block on
each other for large patches; more frequent smaller patches are better. I'm back
from vacation now, it will take me a while to update this patch with rtoy's
comments, if you land the patch with suspend/resume first that's great I'll be
happy to resolve conflicts. Otherwise you can.

[hongchan, 2015-07-09 02:26:09]

On 2015/07/09 02:11:01, dominicc wrote:
> rtoy: Thanks for the comments; I will make it so.
> 
> hongchan: Thank you for the link to your patch. I don't think we should block
on
> each other for large patches; more frequent smaller patches are better. I'm
back
> from vacation now, it will take me a while to update this patch with rtoy's
> comments, if you land the patch with suspend/resume first that's great I'll be
> happy to resolve conflicts. Otherwise you can.

Thanks for your understanding. The CL I mentioned above is at the last stage of
code review, so hopefully it will be landed soon enough.

[dominicc (has gone to gerrit), 2015-07-09 04:03:30]

I've implemented rtoy's comments, PTAL.

hongchan: For context, here's my end goal:

- AudioContexts don't call ActiveDOMObject::stop on themselves. The
infrastructure in core/dom should do that.
- AudioContexts don't report hasPendingActivity after they're stopped.
- AudioContexts created in a suspended (or even stopped) context don't report
hasPendingActivity.

To do this we need to simplify how AudioContexts shut down. Because "online" and
OfflineAudioContexts shut down differently, I want to pull them apart to make
progress on the above problems. I don't want to add more isOffline() calls
everywhere, I found it hard to understand how this worked with the
implementation switching on isOffline and calling methods like
ActiveDOMObject::stop at different times.

[Raymond Toy, 2015-07-13 21:50:29]

lgtm.

In retrospect, I think it would have been better not to change
AbstractAudioContext at this point, but do it in a follow up CL that just did
that mechanical change.  So many files changed now that reviewing is more
difficult. (I didn't check all the files, only the ones that were changed in the
previous patch set.)

[dominicc (has gone to gerrit), 2015-07-14 01:18:42]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2015-07-14 01:18:43]

The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org
Link to the patchset: https://codereview.chromium.org/1214463003/#ps40001
(title: "Bring to ToT")

[commit-bot: I haz the power, 2015-07-14 01:18:55]

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

[commit-bot: I haz the power, 2015-07-14 02:41:00]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198828