setValueCurveAtTime has implicit setValueAtTime at end

setValueCurveAtTime has implicit setValueAtTime at end

So that the value curve is always run even if there's a linearRamp or
exponentialRamp afterwards, call setValueAtTime to establish an event
at the end of the curve which will be used as the starting event for
any future event.  In this way, the value curve is always run.

This also exposed a bug in the implementation of SetValueCurveAtTime where
the current time would sometimes get reset to 0 instead of monotonically increasing.

WebAudio spec issue: https://github.com/WebAudio/web-audio-api/issues/821
Resolved by https://github.com/WebAudio/web-audio-api/pull/832

BUG=613590
TEST=audioparam-setValueCurve-end.html

Committed: https://crrev.com/de853bc3e87d40c16d82ddedc2688b95d50be435
Cr-Commit-Position: refs/heads/master@{#399788}

[Raymond Toy, 2016-06-01 21:26:13]

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

[Raymond Toy, 2016-06-01 21:26:14]

PTAL.

[Raymond Toy, 2016-06-03 15:20:51]

On 2016/06/01 21:26:14, Raymond Toy wrote:
> PTAL.

Ping.

[hongchan, 2016-06-03 17:15:47]

Sorry for the late response.

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right):

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:202:
insertEvent(ParamEvent::createSetValueEvent(curve->data()[curve->length() - 1],
time + duration), exceptionState);
I understand the logic and it does a reasonable thing, but does this behavior
align with the spec?

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:416: //
computed, and we move on to the next event.
What is the purpose of this comment? Is this a TODO or just a node? If this is
TODO, let us make it so.

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:855:
currentFrame += nextEventFillToFrame;
Why are we accumulating this?

[Raymond Toy, 2016-06-03 17:34:26]

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right):

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:202:
insertEvent(ParamEvent::createSetValueEvent(curve->data()[curve->length() - 1],
time + duration), exceptionState);
On 2016/06/03 17:15:47, hoch wrote:
> I understand the logic and it does a reasonable thing, but does this behavior
> align with the spec?

I think so.  The spec (now) says that setValueCurve behaves as if there is a
setValueAtTime at the end of the curve.  We just make it explicit instead of
implicit.

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:416: //
computed, and we move on to the next event.
On 2016/06/03 17:15:47, hoch wrote:
> What is the purpose of this comment? Is this a TODO or just a node? If this is
> TODO, let us make it so.

A note.  I'm not sure what a TODO should say. I think the code is doing the
correct thing in handling this (except for the fix for setValueCurve below).

This shows up in one of the tests and I was surprised that an event that we've
already processed would get handled again (producing no new output).

On reading the comment, I think I need to expand on it some more.

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:855:
currentFrame += nextEventFillToFrame;
On 2016/06/03 17:15:47, hoch wrote:
> Why are we accumulating this?

This is a bug in the original code.  If you look closely (it's hard!)
nextEventFillToFrame is always in the range [0, 128].  The previous code would
just reset currentFrame to be in the range [0,128], even if currentFrame was way
past that, which is definitely wrong.  The time should advance by the number of
frames we've processed.

This error shows up in one of the tests.

[hongchan, 2016-06-03 17:39:30]

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right):

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:202:
insertEvent(ParamEvent::createSetValueEvent(curve->data()[curve->length() - 1],
time + duration), exceptionState);
On 2016/06/03 17:34:25, Raymond Toy wrote:
> On 2016/06/03 17:15:47, hoch wrote:
> > I understand the logic and it does a reasonable thing, but does this
behavior
> > align with the spec?
> 
> I think so.  The spec (now) says that setValueCurve behaves as if there is a
> setValueAtTime at the end of the curve.  We just make it explicit instead of
> implicit.

Acknowledged.

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:855:
currentFrame += nextEventFillToFrame;
On 2016/06/03 17:34:25, Raymond Toy wrote:
> On 2016/06/03 17:15:47, hoch wrote:
> > Why are we accumulating this?
> 
> This is a bug in the original code.  If you look closely (it's hard!)
> nextEventFillToFrame is always in the range [0, 128].  The previous code would
> just reset currentFrame to be in the range [0,128], even if currentFrame was
way
> past that, which is definitely wrong.  The time should advance by the number
of
> frames we've processed.
> 
> This error shows up in one of the tests.

Wow, it is surprising that this hasn't come up so far. But is this related to
this CL? The CL description does not say about this bug being fixed.

[Raymond Toy, 2016-06-03 18:04:15]

On 2016/06/03 17:39:30, hoch wrote:
>
https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp
(right):
> 
>
https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:202:
> insertEvent(ParamEvent::createSetValueEvent(curve->data()[curve->length() -
1],
> time + duration), exceptionState);
> On 2016/06/03 17:34:25, Raymond Toy wrote:
> > On 2016/06/03 17:15:47, hoch wrote:
> > > I understand the logic and it does a reasonable thing, but does this
> behavior
> > > align with the spec?
> > 
> > I think so.  The spec (now) says that setValueCurve behaves as if there is a
> > setValueAtTime at the end of the curve.  We just make it explicit instead of
> > implicit.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:855:
> currentFrame += nextEventFillToFrame;
> On 2016/06/03 17:34:25, Raymond Toy wrote:
> > On 2016/06/03 17:15:47, hoch wrote:
> > > Why are we accumulating this?
> > 
> > This is a bug in the original code.  If you look closely (it's hard!)
> > nextEventFillToFrame is always in the range [0, 128].  The previous code
would
> > just reset currentFrame to be in the range [0,128], even if currentFrame was
> way
> > past that, which is definitely wrong.  The time should advance by the number
> of
> > frames we've processed.
> > 
> > This error shows up in one of the tests.
> 
> Wow, it is surprising that this hasn't come up so far. But is this related to
> this CL? The CL description does not say about this bug being fixed.

I think this didn't happen before because if you scheduled a linear ramp or
exponential ramp after the setValueCurve, the setValueCurve never gets run (per
spec) because the ramp effectively replaces the setValueCurve.

If this isn't fixed, some tests will fail.  And one of the cases where the bug
shows up is what the Warning comment is for because nextEventFillToFrame is 0 in
that case (because fillToFrame is 0).

[hongchan, 2016-06-03 18:20:08]

On 2016/06/03 18:04:15, Raymond Toy wrote:
> On 2016/06/03 17:39:30, hoch wrote:
> >
>
https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:202:
> > insertEvent(ParamEvent::createSetValueEvent(curve->data()[curve->length() -
> 1],
> > time + duration), exceptionState);
> > On 2016/06/03 17:34:25, Raymond Toy wrote:
> > > On 2016/06/03 17:15:47, hoch wrote:
> > > > I understand the logic and it does a reasonable thing, but does this
> > behavior
> > > > align with the spec?
> > > 
> > > I think so.  The spec (now) says that setValueCurve behaves as if there is
a
> > > setValueAtTime at the end of the curve.  We just make it explicit instead
of
> > > implicit.
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:855:
> > currentFrame += nextEventFillToFrame;
> > On 2016/06/03 17:34:25, Raymond Toy wrote:
> > > On 2016/06/03 17:15:47, hoch wrote:
> > > > Why are we accumulating this?
> > > 
> > > This is a bug in the original code.  If you look closely (it's hard!)
> > > nextEventFillToFrame is always in the range [0, 128].  The previous code
> would
> > > just reset currentFrame to be in the range [0,128], even if currentFrame
was
> > way
> > > past that, which is definitely wrong.  The time should advance by the
number
> > of
> > > frames we've processed.
> > > 
> > > This error shows up in one of the tests.
> > 
> > Wow, it is surprising that this hasn't come up so far. But is this related
to
> > this CL? The CL description does not say about this bug being fixed.
> 
> I think this didn't happen before because if you scheduled a linear ramp or
> exponential ramp after the setValueCurve, the setValueCurve never gets run
(per
> spec) because the ramp effectively replaces the setValueCurve.
> 
> If this isn't fixed, some tests will fail.  And one of the cases where the bug
> shows up is what the Warning comment is for because nextEventFillToFrame is 0
in
> that case (because fillToFrame is 0).

Acknowledged.

[Raymond Toy, 2016-06-09 16:23:27]

Anything else I need to do?

[hongchan, 2016-06-09 16:26:09]

lgtm with nits.

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right):

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:416: //
computed, and we move on to the next event.
On 2016/06/03 17:34:25, Raymond Toy wrote:
> On 2016/06/03 17:15:47, hoch wrote:
> > What is the purpose of this comment? Is this a TODO or just a node? If this
is
> > TODO, let us make it so.
> 
> A note.  I'm not sure what a TODO should say. I think the code is doing the
> correct thing in handling this (except for the fix for setValueCurve below).
> 
> This shows up in one of the tests and I was surprised that an event that we've
> already processed would get handled again (producing no new output).
> 
> On reading the comment, I think I need to expand on it some more.

Do you still think this needs to be expanded somehow?

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:855:
currentFrame += nextEventFillToFrame;
On 2016/06/03 17:39:30, hoch wrote:
> On 2016/06/03 17:34:25, Raymond Toy wrote:
> > On 2016/06/03 17:15:47, hoch wrote:
> > > Why are we accumulating this?
> > 
> > This is a bug in the original code.  If you look closely (it's hard!)
> > nextEventFillToFrame is always in the range [0, 128].  The previous code
would
> > just reset currentFrame to be in the range [0,128], even if currentFrame was
> way
> > past that, which is definitely wrong.  The time should advance by the number
> of
> > frames we've processed.
> > 
> > This error shows up in one of the tests.
> 
> Wow, it is surprising that this hasn't come up so far. But is this related to
> this CL? The CL description does not say about this bug being fixed.

Can we edit the description of this CL to reflect this fix?

[Raymond Toy, 2016-06-09 17:30:16]

Description was changed from

==========
setValueCurveAtTime has implicit setValueAtTime at end

So that the value curve is always run even if there's a linearRamp or
exponentialRamp afterwards, call setValueAtTime to establish an event
at the end of the curve which will be used as the starting event for
any future event.  In this way, the value curve is always run.

WebAudio spec issue: https://github.com/WebAudio/web-audio-api/issues/821
Resolved by https://github.com/WebAudio/web-audio-api/pull/832

BUG=613590
TEST=audioparam-setValueCurve-end.html
==========

to

==========
setValueCurveAtTime has implicit setValueAtTime at end

So that the value curve is always run even if there's a linearRamp or
exponentialRamp afterwards, call setValueAtTime to establish an event
at the end of the curve which will be used as the starting event for
any future event.  In this way, the value curve is always run.

This also exposed a bug in the implementation of SetValueCurveAtTime where the
current time would sometimes get reset to 0 instead of monotically increasing.

WebAudio spec issue: https://github.com/WebAudio/web-audio-api/issues/821
Resolved by https://github.com/WebAudio/web-audio-api/pull/832

BUG=613590
TEST=audioparam-setValueCurve-end.html
==========

[Raymond Toy, 2016-06-13 16:36:49]

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right):

https://codereview.chromium.org/2028773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:416: //
computed, and we move on to the next event.
On 2016/06/09 16:26:09, hoch wrote:
> On 2016/06/03 17:34:25, Raymond Toy wrote:
> > On 2016/06/03 17:15:47, hoch wrote:
> > > What is the purpose of this comment? Is this a TODO or just a node? If
this
> is
> > > TODO, let us make it so.
> > 
> > A note.  I'm not sure what a TODO should say. I think the code is doing the
> > correct thing in handling this (except for the fix for setValueCurve below).
> > 
> > This shows up in one of the tests and I was surprised that an event that
we've
> > already processed would get handled again (producing no new output).
> > 
> > On reading the comment, I think I need to expand on it some more.
> 
> Do you still think this needs to be expanded somehow?

Done. PTAL.

[hongchan, 2016-06-13 16:48:18]

lgtm

[Raymond Toy, 2016-06-13 16:56:13]

Description was changed from

==========
setValueCurveAtTime has implicit setValueAtTime at end

So that the value curve is always run even if there's a linearRamp or
exponentialRamp afterwards, call setValueAtTime to establish an event
at the end of the curve which will be used as the starting event for
any future event.  In this way, the value curve is always run.

This also exposed a bug in the implementation of SetValueCurveAtTime where the
current time would sometimes get reset to 0 instead of monotically increasing.

WebAudio spec issue: https://github.com/WebAudio/web-audio-api/issues/821
Resolved by https://github.com/WebAudio/web-audio-api/pull/832

BUG=613590
TEST=audioparam-setValueCurve-end.html
==========

to

==========
setValueCurveAtTime has implicit setValueAtTime at end

So that the value curve is always run even if there's a linearRamp or
exponentialRamp afterwards, call setValueAtTime to establish an event
at the end of the curve which will be used as the starting event for
any future event.  In this way, the value curve is always run.

This also exposed a bug in the implementation of SetValueCurveAtTime where
the current time would sometimes get reset to 0 instead of monotonically
increasing.

WebAudio spec issue: https://github.com/WebAudio/web-audio-api/issues/821
Resolved by https://github.com/WebAudio/web-audio-api/pull/832

BUG=613590
TEST=audioparam-setValueCurve-end.html
==========

[Raymond Toy, 2016-06-13 16:56:41]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2016-06-13 16:57:05]

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

[commit-bot: I haz the power, 2016-06-13 22:34:05]

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

[commit-bot: I haz the power, 2016-06-13 22:34:07]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Raymond Toy, 2016-06-14 16:12:22]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2016-06-14 16:12:42]

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

[commit-bot: I haz the power, 2016-06-14 21:45:39]

Description was changed from

==========
setValueCurveAtTime has implicit setValueAtTime at end

So that the value curve is always run even if there's a linearRamp or
exponentialRamp afterwards, call setValueAtTime to establish an event
at the end of the curve which will be used as the starting event for
any future event.  In this way, the value curve is always run.

This also exposed a bug in the implementation of SetValueCurveAtTime where
the current time would sometimes get reset to 0 instead of monotonically
increasing.

WebAudio spec issue: https://github.com/WebAudio/web-audio-api/issues/821
Resolved by https://github.com/WebAudio/web-audio-api/pull/832

BUG=613590
TEST=audioparam-setValueCurve-end.html
==========

to

==========
setValueCurveAtTime has implicit setValueAtTime at end

So that the value curve is always run even if there's a linearRamp or
exponentialRamp afterwards, call setValueAtTime to establish an event
at the end of the curve which will be used as the starting event for
any future event.  In this way, the value curve is always run.

This also exposed a bug in the implementation of SetValueCurveAtTime where
the current time would sometimes get reset to 0 instead of monotonically
increasing.

WebAudio spec issue: https://github.com/WebAudio/web-audio-api/issues/821
Resolved by https://github.com/WebAudio/web-audio-api/pull/832

BUG=613590
TEST=audioparam-setValueCurve-end.html
==========

[commit-bot: I haz the power, 2016-06-14 21:45:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-14 21:48:10]

Description was changed from

==========
setValueCurveAtTime has implicit setValueAtTime at end

So that the value curve is always run even if there's a linearRamp or
exponentialRamp afterwards, call setValueAtTime to establish an event
at the end of the curve which will be used as the starting event for
any future event.  In this way, the value curve is always run.

This also exposed a bug in the implementation of SetValueCurveAtTime where
the current time would sometimes get reset to 0 instead of monotonically
increasing.

WebAudio spec issue: https://github.com/WebAudio/web-audio-api/issues/821
Resolved by https://github.com/WebAudio/web-audio-api/pull/832

BUG=613590
TEST=audioparam-setValueCurve-end.html
==========

to

==========
setValueCurveAtTime has implicit setValueAtTime at end

So that the value curve is always run even if there's a linearRamp or
exponentialRamp afterwards, call setValueAtTime to establish an event
at the end of the curve which will be used as the starting event for
any future event.  In this way, the value curve is always run.

This also exposed a bug in the implementation of SetValueCurveAtTime where
the current time would sometimes get reset to 0 instead of monotonically
increasing.

WebAudio spec issue: https://github.com/WebAudio/web-audio-api/issues/821
Resolved by https://github.com/WebAudio/web-audio-api/pull/832

BUG=613590
TEST=audioparam-setValueCurve-end.html

Committed: https://crrev.com/de853bc3e87d40c16d82ddedc2688b95d50be435
Cr-Commit-Position: refs/heads/master@{#399788}
==========

[commit-bot: I haz the power, 2016-06-14 21:48:11]

Patchset 5 (id:??) landed as
https://crrev.com/de853bc3e87d40c16d82ddedc2688b95d50be435
Cr-Commit-Position: refs/heads/master@{#399788}