ScriptProcessor buffer size should be consistent with callback size

ScriptProcessor buffer size should be consistent with callback size

When selecting a buffer size for a ScriptProcessorNode (when
constructed using a requested size of 0), the selected size needs to
be consistent with the actuall callback buffer size of the
AudioContext.  Previously, the value was based on the audio hardware
size, which could be very small (128 or 256) on some Android devices,
but the actual callback size is much larger (1024 or 2048).  This
causes glitching because we will fire the process event back-to-back
because of the multiple calls ot process() to satisfy the callback
buffer size.

Thus, add appropriate interfaces so that an AudioContext (and
BaseAudioContext) can get the callback buffer size from the
AudioDestination.

Also, for an offline context, we can just always return a value of 256
(the smallest) for the buffer size.  There is no constraint here
because the offline context waits for the ScriptProcessor to finish
before continuing.

BUG=673854
TEST=

Committed: https://crrev.com/94c37e9181e0619415e8f4d2be3f631b44fe2e64
Cr-Commit-Position: refs/heads/master@{#439927}

[Raymond Toy, 2016-12-15 23:53:20]

The CQ bit was checked by rtoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 23:54:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 01:53:44]

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

[commit-bot: I haz the power, 2016-12-16 01:53:44]

Dry run: This issue passed the CQ dry run.

[Raymond Toy, 2016-12-19 18:52:58]

PTAL.  This is a first step at optimizing this.  At the least, we need to be
consistent to start with.

[Raymond Toy, 2016-12-20 16:32:36]

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

[Raymond Toy, 2016-12-20 16:32:37]

PTAL.

[hongchan, 2016-12-20 20:35:13]

Refactor the change if possible, but I will leave it up to you.

lgtm

https://codereview.chromium.org/2582443004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/2582443004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:68: virtual
size_t callbackBufferSize() const { return 0; }
The comment is needed here. We have many 'callbacks' and 'buffers' in the code,
so it is good to specify what it is for.

https://codereview.chromium.org/2582443004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:116: size_t
callbackBufferSize() const { return handler().callbackBufferSize(); }
Just curious: does this change dynamically? if not, can we just populate it at
the instantiation?

[hongchan, 2016-12-20 20:35:39]

You can refactor/simplify changes if possible, but I'll leave it up to you.

lgtm

[Raymond Toy, 2016-12-20 21:55:32]

https://codereview.chromium.org/2582443004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/2582443004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:68: virtual
size_t callbackBufferSize() const { return 0; }
On 2016/12/20 20:35:13, hongchan wrote:
> The comment is needed here. We have many 'callbacks' and 'buffers' in the
code,
> so it is good to specify what it is for.

Done.

And also made it pure virtual as we discussed.

https://codereview.chromium.org/2582443004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:116: size_t
callbackBufferSize() const { return handler().callbackBufferSize(); }
On 2016/12/20 20:35:12, hongchan wrote:
> Just curious: does this change dynamically? if not, can we just populate it at
> the instantiation?

Don't think this can change dynamically.  Might be able to populate it at
instantiation, but the derived classes are the only ones who know what the value
should be, so they need to be queried anyway.

[Raymond Toy, 2016-12-20 21:56:25]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2016-12-20 21:56:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/2582443004/#ps20001
(title: "Make callbackBufferSize pure virtual")

[commit-bot: I haz the power, 2016-12-20 21:57:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 23:37:13]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482270985127770,
"parent_rev": "c3eb1d200831ac469a8c043e09397eea6793d102", "commit_rev":
"8f2eab8d0c9929cb8754a24239baf01ef0d7cdeb"}

[commit-bot: I haz the power, 2016-12-20 23:38:07]

Description was changed from

==========
ScriptProcessor buffer size should be consistent with callback size

When selecting a buffer size for a ScriptProcessorNode (when
constructed using a requested size of 0), the selected size needs to
be consistent with the actuall callback buffer size of the
AudioContext.  Previously, the value was based on the audio hardware
size, which could be very small (128 or 256) on some Android devices,
but the actual callback size is much larger (1024 or 2048).  This
causes glitching because we will fire the process event back-to-back
because of the multiple calls ot process() to satisfy the callback
buffer size.

Thus, add appropriate interfaces so that an AudioContext (and
BaseAudioContext) can get the callback buffer size from the
AudioDestination.

Also, for an offline context, we can just always return a value of 256
(the smallest) for the buffer size.  There is no constraint here
because the offline context waits for the ScriptProcessor to finish
before continuing.

BUG=673854
TEST=
==========

to

==========
ScriptProcessor buffer size should be consistent with callback size

When selecting a buffer size for a ScriptProcessorNode (when
constructed using a requested size of 0), the selected size needs to
be consistent with the actuall callback buffer size of the
AudioContext.  Previously, the value was based on the audio hardware
size, which could be very small (128 or 256) on some Android devices,
but the actual callback size is much larger (1024 or 2048).  This
causes glitching because we will fire the process event back-to-back
because of the multiple calls ot process() to satisfy the callback
buffer size.

Thus, add appropriate interfaces so that an AudioContext (and
BaseAudioContext) can get the callback buffer size from the
AudioDestination.

Also, for an offline context, we can just always return a value of 256
(the smallest) for the buffer size.  There is no constraint here
because the offline context waits for the ScriptProcessor to finish
before continuing.

BUG=673854
TEST=

Review-Url: https://codereview.chromium.org/2582443004
==========

[commit-bot: I haz the power, 2016-12-20 23:38:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-20 23:40:13]

Description was changed from

==========
ScriptProcessor buffer size should be consistent with callback size

When selecting a buffer size for a ScriptProcessorNode (when
constructed using a requested size of 0), the selected size needs to
be consistent with the actuall callback buffer size of the
AudioContext.  Previously, the value was based on the audio hardware
size, which could be very small (128 or 256) on some Android devices,
but the actual callback size is much larger (1024 or 2048).  This
causes glitching because we will fire the process event back-to-back
because of the multiple calls ot process() to satisfy the callback
buffer size.

Thus, add appropriate interfaces so that an AudioContext (and
BaseAudioContext) can get the callback buffer size from the
AudioDestination.

Also, for an offline context, we can just always return a value of 256
(the smallest) for the buffer size.  There is no constraint here
because the offline context waits for the ScriptProcessor to finish
before continuing.

BUG=673854
TEST=

Review-Url: https://codereview.chromium.org/2582443004
==========

to

==========
ScriptProcessor buffer size should be consistent with callback size

When selecting a buffer size for a ScriptProcessorNode (when
constructed using a requested size of 0), the selected size needs to
be consistent with the actuall callback buffer size of the
AudioContext.  Previously, the value was based on the audio hardware
size, which could be very small (128 or 256) on some Android devices,
but the actual callback size is much larger (1024 or 2048).  This
causes glitching because we will fire the process event back-to-back
because of the multiple calls ot process() to satisfy the callback
buffer size.

Thus, add appropriate interfaces so that an AudioContext (and
BaseAudioContext) can get the callback buffer size from the
AudioDestination.

Also, for an offline context, we can just always return a value of 256
(the smallest) for the buffer size.  There is no constraint here
because the offline context waits for the ScriptProcessor to finish
before continuing.

BUG=673854
TEST=

Committed: https://crrev.com/94c37e9181e0619415e8f4d2be3f631b44fe2e64
Cr-Commit-Position: refs/heads/master@{#439927}
==========

[commit-bot: I haz the power, 2016-12-20 23:40:14]

Patchset 2 (id:??) landed as
https://crrev.com/94c37e9181e0619415e8f4d2be3f631b44fe2e64
Cr-Commit-Position: refs/heads/master@{#439927}