setValueCurveAtTime has implicit setValueAtTime at end

third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp

Index: third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp
diff --git a/third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp b/third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp
index 54068c6a3b81c2422a1e44129231095271c94822..c97213f385764257493fb18091dee142e11b2cfb 100644
--- a/third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp
+++ b/third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp
@@ -195,6 +195,11 @@ void AudioParamTimeline::setValueCurveAtTime(DOMFloat32Array* curve, double time
         return;
 
     insertEvent(ParamEvent::createSetValueCurveEvent(curve, time, duration), exceptionState);
+
+    // Insert a setValueAtTime event too to establish an event so that all
+    // following events will process from the end of the curve instead of the
+    // beginning.
+    insertEvent(ParamEvent::createSetValueEvent(curve->data()[curve->length() - 1], time + duration), exceptionState);

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

I understand the logic and it does a reasonable thing, but does this behavior
align with the spec?

[Raymond Toy, 2016/06/03 17:34:25]

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.

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

Acknowledged.

 }
 
 void AudioParamTimeline::insertEvent(const ParamEvent& event, ExceptionState& exceptionState)
@@ -402,6 +407,13 @@ float AudioParamTimeline::valuesForFrameRangeImpl(
         ParamEvent* nextEvent = i < n - 1 ? &(m_events[i + 1]) : 0;
 
         // Wait until we get a more recent event.
+        //
+        // WARNING: due to round-off it might happen that nextEvent->time() is
+        // just less than currentFrame/sampleRate.  This means that we will end
+        // up running the event again.  The code below had better be prepared
+        // for this case!  What should happen is the fillToFrame should be 0 so
+        // that while the event is actually run again, nothing actually gets
+        // computed, and we move on to the next event.

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

What is the purpose of this comment? Is this a TODO or just a node? If this is
TODO, let us make it so.

[Raymond Toy, 2016/06/03 17:34:25]

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.

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

Do you still think this needs to be expanded somehow?

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

Done. PTAL.

         if (nextEvent && nextEvent->time() < currentFrame / sampleRate) {
             // But if the current event is a SetValue event and the event time is between
             // currentFrame - 1 and curentFrame (in time). we don't want to skip it.  If we do skip
@@ -840,7 +852,7 @@ float AudioParamTimeline::valuesForFrameRangeImpl(
                         values[writeIndex] = value;
 
                     // Re-adjust current time
-                    currentFrame = nextEventFillToFrame;
+                    currentFrame += nextEventFillToFrame;

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

Why are we accumulating this?

[Raymond Toy, 2016/06/03 17:34:25]

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]

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.

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

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

 
                     break;
                 }