start/stop method for AudioBufferSourceNodes and OscillatorNodes can take no args.

start/stop method for AudioBufferSourceNodes and OscillatorNodes can take no args.

For AudioBufferSourceNodes, see
http://webaudio.github.io/web-audio-api/#methodsandparams-AudioBufferSourceNode

For OscillatorNodes, see https://github.com/WebAudio/web-audio-api/issues/249

BUG=305829, 306141
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160845

[Ken Russell (switch to Gerrit), 2013-10-11 20:19:06]

On the whole this looks OK but a couple of comments. Tell me if you want to
proceed with this CL verbatim.

https://codereview.chromium.org/26913005/diff/1/Source/modules/webaudio/Audio...
File Source/modules/webaudio/AudioBufferSourceNode.idl (right):

https://codereview.chromium.org/26913005/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioBufferSourceNode.idl:46: [MeasureAs=WebAudioStart]
void start(optional double when);
It's a little weird to use "optional" just for this argument, when according to
the IDL all of the arguments are optional. What about refactoring the C++
implementation so you don't need the ImplementedAs= clauses for the other two
overloads, and then just have one version using "optional" on all arguments?

https://codereview.chromium.org/26913005/diff/1/Source/modules/webaudio/Oscil...
File Source/modules/webaudio/OscillatorNode.idl (right):

https://codereview.chromium.org/26913005/diff/1/Source/modules/webaudio/Oscil...
Source/modules/webaudio/OscillatorNode.idl:51: void stop(optional double when);
It looks to me like the "optional" argument needs to be added for these methods
in the Web Audio spec as well.

[Raymond Toy (Google), 2013-10-11 20:51:29]

https://codereview.chromium.org/26913005/diff/1/Source/modules/webaudio/Audio...
File Source/modules/webaudio/AudioBufferSourceNode.idl (right):

https://codereview.chromium.org/26913005/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioBufferSourceNode.idl:46: [MeasureAs=WebAudioStart]
void start(optional double when);
On 2013/10/11 20:19:06, Ken Russell wrote:
> It's a little weird to use "optional" just for this argument, when according
to
> the IDL all of the arguments are optional. What about refactoring the C++
> implementation so you don't need the ImplementedAs= clauses for the other two
> overloads, and then just have one version using "optional" on all arguments?

Let me try this out.

https://codereview.chromium.org/26913005/diff/1/Source/modules/webaudio/Oscil...
File Source/modules/webaudio/OscillatorNode.idl (right):

https://codereview.chromium.org/26913005/diff/1/Source/modules/webaudio/Oscil...
Source/modules/webaudio/OscillatorNode.idl:51: void stop(optional double when);
On 2013/10/11 20:19:06, Ken Russell wrote:
> It looks to me like the "optional" argument needs to be added for these
methods
> in the Web Audio spec as well.

Yes. I've raised this issue.  See
https://github.com/WebAudio/web-audio-api/issues/249

I believe this is needed to be consistent and will become part of the spec.

[Raymond Toy (Google), 2013-10-14 19:45:57]

Refactored C++ code as suggested.  Please take a look.

[Ken Russell (switch to Gerrit), 2013-10-14 22:54:05]

https://codereview.chromium.org/26913005/diff/14001/Source/modules/webaudio/A...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/26913005/diff/14001/Source/modules/webaudio/A...
Source/modules/webaudio/AudioBufferSourceNode.cpp:377:
AudioScheduledSourceNode::start(when);
Are you sure it's correct to delegate to AudioScheduledSourceNode::start here?
The previous implementation used to delegate
AudioBufferSourceNode::startGrain(double, double, double) (which is now
::start(...)), and it looks like it does a *lot* more work for
AudioBufferSourceNodes than AudioScheduledSourceNode::start does.

I assume that there are already existing WebAudio layout tests that cover this
functionality and which work after this change?

[Raymond Toy (Google), 2013-10-28 17:34:56]

https://codereview.chromium.org/26913005/diff/14001/Source/modules/webaudio/A...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/26913005/diff/14001/Source/modules/webaudio/A...
Source/modules/webaudio/AudioBufferSourceNode.cpp:377:
AudioScheduledSourceNode::start(when);
On 2013/10/14 22:54:05, Ken Russell wrote:
> Are you sure it's correct to delegate to AudioScheduledSourceNode::start here?
> The previous implementation used to delegate
> AudioBufferSourceNode::startGrain(double, double, double) (which is now
> ::start(...)), and it looks like it does a *lot* more work for
> AudioBufferSourceNodes than AudioScheduledSourceNode::start does.
>

The diff is confusing.  A plain start(when) is the simple start method that
doesn't deal with the issues with startGrain.  We didn't really replace
startGrain with start.  startGrain with 2 args is converted to startGrain (now
called start) with 3 args by using a default value for the third arg.
 
> I assume that there are already existing WebAudio layout tests that cover this
> functionality and which work after this change?

Yes, there are tests for start with 2+ args in audiobuffersource-start.html. 
All webaudio layout tests pass.

[Ken Russell (switch to Gerrit), 2013-10-28 20:13:42]

https://codereview.chromium.org/26913005/diff/14001/Source/modules/webaudio/A...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/26913005/diff/14001/Source/modules/webaudio/A...
Source/modules/webaudio/AudioBufferSourceNode.cpp:377:
AudioScheduledSourceNode::start(when);
On 2013/10/28 17:34:57, rtoy wrote:
> On 2013/10/14 22:54:05, Ken Russell wrote:
> > Are you sure it's correct to delegate to AudioScheduledSourceNode::start
here?
> > The previous implementation used to delegate
> > AudioBufferSourceNode::startGrain(double, double, double) (which is now
> > ::start(...)), and it looks like it does a *lot* more work for
> > AudioBufferSourceNodes than AudioScheduledSourceNode::start does.
> >
> 
> The diff is confusing.  A plain start(when) is the simple start method that
> doesn't deal with the issues with startGrain.  We didn't really replace
> startGrain with start.  startGrain with 2 args is converted to startGrain (now
> called start) with 3 args by using a default value for the third arg.

I understand this, but the new code's behavior is significantly different when
calling the simplified start() compared to the version taking more arguments.
The simple start() method delegates up to the superclass, and I think that is a
bad idea, because it will cause the AudioBufferSourceNode to potentially get
into states that are unexpected, especially as the code base evolves.

Because AudioBufferSourceNode shadows some of the invocations of
AudioScheduledSourceNode::start it should shadow all of them. There should be
one start() declaration in the header, taking all three arguments as optional,
just like in the spec.

[Raymond Toy (Google), 2013-10-28 21:21:29]

On 2013/10/28 20:13:42, Ken Russell wrote:
>
https://codereview.chromium.org/26913005/diff/14001/Source/modules/webaudio/A...
> File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):
> 
>
https://codereview.chromium.org/26913005/diff/14001/Source/modules/webaudio/A...
> Source/modules/webaudio/AudioBufferSourceNode.cpp:377:
> AudioScheduledSourceNode::start(when);
> On 2013/10/28 17:34:57, rtoy wrote:
> > On 2013/10/14 22:54:05, Ken Russell wrote:
> > > Are you sure it's correct to delegate to AudioScheduledSourceNode::start
> here?
> > > The previous implementation used to delegate
> > > AudioBufferSourceNode::startGrain(double, double, double) (which is now
> > > ::start(...)), and it looks like it does a *lot* more work for
> > > AudioBufferSourceNodes than AudioScheduledSourceNode::start does.
> > >
> > 
> > The diff is confusing.  A plain start(when) is the simple start method that
> > doesn't deal with the issues with startGrain.  We didn't really replace
> > startGrain with start.  startGrain with 2 args is converted to startGrain
(now
> > called start) with 3 args by using a default value for the third arg.
> 
> I understand this, but the new code's behavior is significantly different when
> calling the simplified start() compared to the version taking more arguments.
> The simple start() method delegates up to the superclass, and I think that is
a
> bad idea, because it will cause the AudioBufferSourceNode to potentially get
> into states that are unexpected, especially as the code base evolves.

Done. start() no longer delegates to the superclass.
> 
> Because AudioBufferSourceNode shadows some of the invocations of
> AudioScheduledSourceNode::start it should shadow all of them. There should be
> one start() declaration in the header, taking all three arguments as optional,
> just like in the spec.

[Ken Russell (switch to Gerrit), 2013-10-28 21:32:30]

Thanks for making this change. The direction seems significantly better. One
remaining concern.

https://codereview.chromium.org/26913005/diff/99001/Source/modules/webaudio/A...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/26913005/diff/99001/Source/modules/webaudio/A...
Source/modules/webaudio/AudioBufferSourceNode.cpp:418: m_isGrain = m_grainOffset
|| m_grainDuration != bufferDuration;
The == comparison with bufferDuration seems like it will be flaky. Can this be
expressed differently? For example, by continuing to treat a grainDuration of
0.0 as playing the entire buffer?

[Raymond Toy (Google), 2013-10-28 21:39:10]

On 2013/10/28 21:32:30, Ken Russell wrote:
> Thanks for making this change. The direction seems significantly better. One
> remaining concern.
> 
>
https://codereview.chromium.org/26913005/diff/99001/Source/modules/webaudio/A...
> File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):
> 
>
https://codereview.chromium.org/26913005/diff/99001/Source/modules/webaudio/A...
> Source/modules/webaudio/AudioBufferSourceNode.cpp:418: m_isGrain =
m_grainOffset
> || m_grainDuration != bufferDuration;
> The == comparison with bufferDuration seems like it will be flaky. Can this be
> expressed differently? For example, by continuing to treat a grainDuration of
> 0.0 as playing the entire buffer?

Doing this will simplify the code to just one start method, which would be good.
 But the spec doesn't say that the duration of 0 has that special meaning. 
We've also inadvertently broken that feature for the deprecated noteGrainOn
method.

[Ken Russell (switch to Gerrit), 2013-10-28 21:55:40]

On 2013/10/28 21:39:10, rtoy wrote:
> Doing this will simplify the code to just one start method, which would be
good.
>  But the spec doesn't say that the duration of 0 has that special meaning. 
> We've also inadvertently broken that feature for the deprecated noteGrainOn
> method.

In that case would you consider adding a private startImpl method, having the
three start() variants delegate to that, and explicitly passing a flag from
start(double when) and start(double when, double offset) indicating to play the
rest of the buffer? start(double when, double offset, double duration) would
pass false for this flag.

[Raymond Toy (Google), 2013-10-28 22:12:50]

On 2013/10/28 21:55:40, Ken Russell wrote:
> On 2013/10/28 21:39:10, rtoy wrote:
> > Doing this will simplify the code to just one start method, which would be
> good.
> >  But the spec doesn't say that the duration of 0 has that special meaning. 
> > We've also inadvertently broken that feature for the deprecated noteGrainOn
> > method.
> 
> In that case would you consider adding a private startImpl method, having the
> three start() variants delegate to that, and explicitly passing a flag from
> start(double when) and start(double when, double offset) indicating to play
the
> rest of the buffer? start(double when, double offset, double duration) would
> pass false for this flag.

Heh. I was just implementing that right now.

[Raymond Toy (Google), 2013-10-28 22:26:05]

PTAL.

[Ken Russell (switch to Gerrit), 2013-10-28 23:06:31]

https://codereview.chromium.org/26913005/diff/179001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/26913005/diff/179001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.cpp:409: if (isGrain) {
It looks like an else clause is needed initializing m_grainDuration, etc.

If there isn't already a test catching that kind of error then please add one.

https://codereview.chromium.org/26913005/diff/179001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.cpp:424: m_isGrain = isGrain;
I don't think this should be inside the conditional.

[Raymond Toy (Google), 2013-10-28 23:20:38]

https://codereview.chromium.org/26913005/diff/179001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/26913005/diff/179001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.cpp:409: if (isGrain) {
On 2013/10/28 23:06:31, Ken Russell wrote:
> It looks like an else clause is needed initializing m_grainDuration, etc.
> 
> If there isn't already a test catching that kind of error then please add one.

m_grainOffset and m_grainDuration are initialized in the constructor.  start is
only supposed to be called once, but we have no checks for that currently.

I will add the else case for now, until the checks for start are implemented.

[Raymond Toy (Google), 2013-10-28 23:37:59]

https://codereview.chromium.org/26913005/diff/179001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/26913005/diff/179001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.cpp:409: if (isGrain) {
On 2013/10/28 23:20:38, rtoy wrote:
> On 2013/10/28 23:06:31, Ken Russell wrote:
> > It looks like an else clause is needed initializing m_grainDuration, etc.
> > 
> > If there isn't already a test catching that kind of error then please add
one.
> 
> m_grainOffset and m_grainDuration are initialized in the constructor.  start
is
> only supposed to be called once, but we have no checks for that currently.
> 
> I will add the else case for now, until the checks for start are implemented.

Done.

https://codereview.chromium.org/26913005/diff/179001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.cpp:424: m_isGrain = isGrain;
On 2013/10/28 23:06:31, Ken Russell wrote:
> I don't think this should be inside the conditional.

Done.

[Ken Russell (switch to Gerrit), 2013-10-28 23:55:48]

Thanks for persevering with this CL. LGTM

[Raymond Toy (Google), 2013-10-29 14:59:59]

On 2013/10/28 23:55:48, Ken Russell wrote:
> Thanks for persevering with this CL. LGTM

Likewise!

[commit-bot: I haz the power, 2013-10-29 15:01:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtoy@google.com/26913005/269002

[commit-bot: I haz the power, 2013-10-29 17:20:00]

Change committed as 160845