setValueCurveAtTime has implicit setValueAtTime at end

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

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 177 matching lines @@
 void AudioParamTimeline::setValueCurveAtTime(DOMFloat32Array* curve, double time, double duration, ExceptionState& exceptionState)
 {
     ASSERT(isMainThread());
     ASSERT(curve);
 
     if (!isNonNegativeAudioParamTime(time, exceptionState)
         || !isPositiveAudioParamTime(duration, exceptionState, "Duration"))
         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)
 {
     ASSERT(isMainThread());
 
     // Sanity check the event. Be super careful we're not getting infected with NaN or Inf. These
     // should have been handled by the caller.
     bool isValid = event.getType() < ParamEvent::LastType
         && std::isfinite(event.value())
@@ skipping 187 matching lines @@
     // Go through each event and render the value buffer where the times overlap,
     // stopping when we've rendered all the requested values.
     // FIXME: could try to optimize by avoiding having to iterate starting from the very first event
     // and keeping track of a "current" event index.
     int n = m_events.size();
     for (int i = 0; i < n && writeIndex < numberOfValues; ++i) {
         ParamEvent& event = m_events[i];
         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
             // it, the SetValue event is completely skipped and not applied, which is wrong.  Other
             // events don't have this problem.  (Because currentFrame is unsigned, we do the time
             // check in this funny, but equivalent way.)
             double eventFrame = event.time() * sampleRate;
 
             // Condition is currentFrame - 1 < eventFrame <= currentFrame, but currentFrame is
             // unsigned and could be 0, so use currentFrame < eventFrame + 1 instead.
@@ skipping 418 matching lines @@
                     }
 
                     // If there's any time left after the duration of this event and the start
                     // of the next, then just propagate the last value of the curveData.
                     if (writeIndex < nextEventFillToFrame)
                         value = curveData[numberOfCurvePoints - 1];
                     for (; writeIndex < nextEventFillToFrame; ++writeIndex)
                         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;
                 }
             case ParamEvent::LastType:
                 ASSERT_NOT_REACHED();
                 break;
             }
         }
     }
 
     // If there's any time left after processing the last event then just propagate the last value
     // to the end of the values buffer.
     for (; writeIndex < numberOfValues; ++writeIndex)
         values[writeIndex] = value;
 
     // This value is used to set the .value attribute of the AudioParam.  it should be the last
     // computed value.
     return values[numberOfValues - 1];
 }
 
 } // namespace blink