Closed Bug 897092 Opened 6 years ago Closed 6 years ago

Fix setTargetAtTime glitch

Categories

(Core :: Web Audio, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox25 --- affected
firefox26 --- fixed

People

(Reporter: padenot, Assigned: padenot)

References

Details

Attachments

(2 files)

Attached image glitch.png
Attached is a graph showing the value over time of the gain member of a gain node on which setTargetAtTime has been called applied. This is from the noteflight testcase. We can clearly see the glitch.
(x is the number of frame process, y is the value of the param)
So, this is probably caused by [1]: we use mValue all the time when starting to compute a setValueAtTime event in a timeline. mValue being 1.0, we do an exponential decrease from 1.0 to 0.0 to implement the release portion of the ADSR, instead of doing an exponential decrease from the previous value to 0.0.

The spec could be understood in two ways:

> Start exponentially approaching the target value at the given time with a rate having 
> the given time constant. Among other uses, this is useful for implementing the "decay" 
> and "release" portions of an ADSR envelope.

To me, to implement a release or decay, you take the current value, and you move down with a specific curve to the specified value, during a specified time. That's how I implemented it in the past in other projects.

Then, the spec goes:

> Where V0 is the initial value (the .value attribute) at T0 (the startTime parameter) 
> and V1 is equal to the target parameter. 

The .value attribute could be read as "the value computed the last time", but we read it as "whatever the author specified on the AudioParam", it's actually the same problem as in bug 893020:

> An intrinsic parameter value will be calculated at each time, which is either the 
> value set directly to the .value attribute, or, if there are any scheduled parameter 
> changes (automation events) with times before or at this time, the value as
> calculated from these events.

Then,

> When read, the .value attribute always returns the intrinsic value for the current 
> time.

From our reading of the spec, our code is correct, but from a practical standpoint, this method is useless (an author would have to call setValueAtTime, with the same time as the setTargetAtTime, and recompute the last value the UA would have computed considering the past events to implement the decay and release portion of an ADSR curve).

[1]: http://mxr.mozilla.org/mozilla-central/source/content/media/AudioEventTimeline.h#266
Flags: needinfo?(ehsan)
I _think_ our reading of the spec is correct, but that's not entirely clear.  Do you mind raising this on public-audio please?  Note that I think that changing the spec to use the current value as opposed to .value makes sense, but I still don't like the idea of making .value return a time based on the currentTime.
Flags: needinfo?(ehsan)
Yes, I'll needinfo myself here to do that on monday.
Flags: needinfo?(paul)
I feel that for the reasons Paul pointed out, the "intrinsic parameter value" (possibly computed based on active value automation events) is the way to go, rather than using the originally assigned (or defaulted) .value of the AudioParam.  The current intrinsic value is the obvious intended starting point of any value curve. The other interpretation requires multiple, redundant assignments of the .value attribute, possibly along with tricky calculations that must emulate the automation behavior.

I think this is probably what the spec intended anyway. I'm happy to back any clarification requests on public-audio that lead to the above result.
(In reply to joe from comment #5)
> I feel that for the reasons Paul pointed out, the "intrinsic parameter
> value" (possibly computed based on active value automation events) is the
> way to go, rather than using the originally assigned (or defaulted) .value
> of the AudioParam.  The current intrinsic value is the obvious intended
> starting point of any value curve. The other interpretation requires
> multiple, redundant assignments of the .value attribute, possibly along with
> tricky calculations that must emulate the automation behavior.

Yes, I agree.

> I think this is probably what the spec intended anyway. I'm happy to back
> any clarification requests on public-audio that lead to the above result.

Great, thanks!
Based on the public-audio discussion, we should use the last intrinsic value, not the initial value.
There are two ways of solving this:

(1) We can keep GetValueAtTime const (and a pure function), but that's not great, because we would have to find the last value of the last event. Now, that's not a problem when the events are not overlapping  (we can simply get the value), but because SetTargetAtTime is defined in terms of end time, and the other automation function in terms of start time, it can happen. The solution is to recurse and get the value at the time right before the event we are considering. This can be a bit complicated if multiple events are overlapping (or if multiple SetTargetAtTime events are in succession, because they don't really have an end time). This also brings the problems of a-rate and k-rate parameter. For an a-rate, this is fine, we just substract one to the requested time (note that this does not work well for our unit tests because they use floats). For a k-rate, we'd have to round to the last audio block.

(2) We can make AudioTimelineEvent stateful by keeping an |float mIntrinsicValue;| (this is the approach taken by webkit/blink). This has the benefit of speeding up the calculation, and making the edge cases rather straightforward (we just would have to remember the last computed value on events boundaries on the timeline, and interpolate from there). We would have to rewrite our unit tests, because they assume statelessness). We could also get an array of computed values very efficiently for a-rate parameters this way (webkit/blink does this, and it seems smart).

For now, I have an hybrid solution (The class is stateful, but the state is only used for the computation of SetTargetAtTime).
Flags: needinfo?(paul)
(In reply to comment #8)
> There are two ways of solving this:
> 
> (1) We can keep GetValueAtTime const (and a pure function), but that's not
> great, because we would have to find the last value of the last event. Now,
> that's not a problem when the events are not overlapping  (we can simply get
> the value), but because SetTargetAtTime is defined in terms of end time, and
> the other automation function in terms of start time, it can happen. The
> solution is to recurse and get the value at the time right before the event we
> are considering. This can be a bit complicated if multiple events are
> overlapping (or if multiple SetTargetAtTime events are in succession, because
> they don't really have an end time). This also brings the problems of a-rate
> and k-rate parameter. For an a-rate, this is fine, we just substract one to the
> requested time (note that this does not work well for our unit tests because
> they use floats). For a k-rate, we'd have to round to the last audio block.
> 
> (2) We can make AudioTimelineEvent stateful by keeping an |float
> mIntrinsicValue;| (this is the approach taken by webkit/blink). This has the
> benefit of speeding up the calculation, and making the edge cases rather
> straightforward (we just would have to remember the last computed value on
> events boundaries on the timeline, and interpolate from there). We would have
> to rewrite our unit tests, because they assume statelessness). We could also
> get an array of computed values very efficiently for a-rate parameters this way
> (webkit/blink does this, and it seems smart).
> 
> For now, I have an hybrid solution (The class is stateful, but the state is
> only used for the computation of SetTargetAtTime).

I like the second approach better, but it's hard to say for sure without a concrete patch.  :-)  FWIW if you have a patch which fixes this problem, we can just take that.  We don't need to switch to array calculation at the same time.  That will be more complicated.
This is what I had in mind. The glitch goes away with this, but since we are now
stateful when using SetTargetAtTime, I had to alter the test a bit, and fuzz the
equality check when GetValueAtTime checks if we reached a new event.
Attachment #785779 - Flags: review?(ehsan)
Comment on attachment 785779 [details] [diff] [review]
Fix setTargetAtTime glitch. r=

Review of attachment 785779 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AudioEventTimeline.h
@@ +14,5 @@
>  
>  #include "nsTArray.h"
>  #include "math.h"
> +#include <iostream>
> +#include <cstdlib>

You should not need any of these headers.

@@ +21,5 @@
>  
>  namespace dom {
>  
> +template<class TimeType>
> +bool CompareTime(TimeType aLhs, TimeType aRhs);

Call this TimesEqual to make the return value clearer.

@@ +251,5 @@
>    }
>  
>    // This method computes the AudioParam value at a given time based on the event timeline
>    template<class TimeType>
> +  float GetValueAtTime(TimeType aTime)

I think your patch would be a lot simpler if you converted this function to a private helper which would return float and assign mComputedValue as a signal that its work was done, and then redefined GetValueAtTime() to call GetValueAtTimeHelper() or whatever, and then return mComputedValue.

Something like:

template<class TimeType>
float GetValueAtTime(TimeType aTime)
{
  mComputedTime = GetValueAtTimeHelper(aTime);
  return mComputedTime;
}

I'm also fairly worried that we may forget to assign to mComputedValue in some code paths.  This way, the compiler will catch us if we ever do that.  It also makes your patch a lot smaller.

@@ +286,5 @@
>            if (mEvents[i - 1].mType == AudioTimelineEvent::SetValueCurve) {
> +            return mComputedValue = ExtractValueFromCurve(mEvents[i - 1].template Time<TimeType>(),
> +                                                          mEvents[i - 1].mCurve,
> +                                                          mEvents[i - 1].mCurveLength,
> +                                                          mEvents[i - 1].mDuration, aTime);

Please assign and return in two statements here and elsewhere in the patch, this is not very readable.  (Although this will probably not be an issue with the above comment addressed.

@@ +555,5 @@
>    // being a bottleneck.
>    nsTArray<AudioTimelineEvent> mEvents;
>    float mValue;
> +  float mComputedValue;
> +  float mLastComputedValue;

Please document these new values.

::: content/media/webaudio/compiledtest/TestAudioEventTimeline.cpp
@@ +8,5 @@
>  #include "TestHarness.h"
>  #include <sstream>
>  #include <limits>
> +#include <unistd.h>
> +#include <stdio.h>

This won't compile on windows (unistd.h doesn't exist there.)  But you shouldn't need any of these headers.

@@ +333,5 @@
>    timeline.SetTargetAtTime(20.0f, 1.0, 5.0, rv);
> +
> +  for (double i = 0.0; i < 9.99; i+=0.01) {
> +    timeline.GetValueAtTime(i);
> +  }

Please add a comment to explain what this loop is trying to achieve.
Attachment #785779 - Flags: review?(ehsan) → review+
Comment on attachment 785779 [details] [diff] [review]
Fix setTargetAtTime glitch. r=

This is needed for Web Audio in Firefox 25.
Attachment #785779 - Flags: approval-mozilla-aurora?
checkin-needed for Aurora, a=webaudio.
Keywords: checkin-needed
Attachment #785779 - Flags: approval-mozilla-aurora?
Keywords: checkin-needed
Whiteboard: [checkin-needed-aurora]
https://hg.mozilla.org/mozilla-central/rev/88a8dfd19a67
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Blocks: 907758
You need to log in before you can comment on or make changes to this bug.