Closed
Bug 897092
Opened 11 years ago
Closed 11 years ago
Fix setTargetAtTime glitch
Categories
(Core :: Web Audio, defect)
Core
Web Audio
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: padenot, Assigned: padenot)
References
Details
Attachments
(2 files)
34.39 KB,
image/png
|
Details | |
13.39 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
(x is the number of frame process, y is the value of the param)
Assignee | ||
Comment 2•11 years ago
|
||
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)
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
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.
Comment 6•11 years ago
|
||
(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!
Comment 7•11 years ago
|
||
Based on the public-audio discussion, we should use the last intrinsic value, not the initial value.
Assignee | ||
Comment 8•11 years ago
|
||
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)
Comment 9•11 years ago
|
||
(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.
Assignee | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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 12•11 years ago
|
||
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?
Assignee | ||
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f74f1e59dc50
Updated•11 years ago
|
Attachment #785779 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Keywords: checkin-needed
Whiteboard: [checkin-needed-aurora]
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/88a8dfd19a67
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 17•11 years ago
|
||
Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/4d652113720b because something in https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=2d110609f4d3 caused test_mediaStreamAudioSourceNodeResampling.html to time out (once https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=918a847c0018 let things get that far).
You need to log in
before you can comment on or make changes to this bug.
Description
•