Closed Bug 1118071 Opened 5 years ago Closed 5 years ago

setTargetAtTime curve calculating incorrectly after first calculation

Categories

(DevTools Graveyard :: Web Audio Editor, defect)

37 Branch
x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 1 obsolete file)

Possibly due to implementation in bug 1108928, but suspect its in the calculation library[0], on the first calculation, this is done correctly, exponentially approaching target[1], but on subsequent calculations, the approach is lost[2]

[0] https://github.com/jsantell/web-audio-automation-timeline/issues/1
[1] http://i.imgur.com/KfVLENH.png
[2] http://i.imgur.com/WK70Sg8.png
Assignee: nobody → jsantell
Blocks: 1108928
Status: NEW → ASSIGNED
Landed fix in the automation library[0].

Victor, r? for you for review for devtools.

Paul -- f? for you on implementation of the patch. Due to Firefox's automation timeline implementation, there exists a stateful computed value as automation values are calculated real-time (and there's no going in time). Removing the statefulness of the JS implementation resulted in a function used to calculate the lastComputedValue[1] -- meaning get the 'value' for the previous event, or the set value (with value setter/setValue) if the only event in timeline. Wanted to run this by you, to see if this is inline with the spec's `setTargetAtTime` implementation.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d2eeb2953693

[0] https://github.com/jsantell/web-audio-automation-timeline/pull/2
[1] https://github.com/jsantell/web-audio-automation-timeline/blob/ac783cb80688bdfdd499a443a8cff55adc6c4e4c/lib/timeline.js#L249-L264
Attachment #8544375 - Flags: review?(vporof)
Attachment #8544375 - Flags: feedback?(padenot)
Comment on attachment 8544375 [details] [diff] [review]
1118071-automation-library-update.patch

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

::: toolkit/devtools/server/actors/utils/automation-timeline.js
@@ +67,5 @@
>  exports.fuzzyEqual = function (lhs, rhs) {
>    return Math.abs(lhs - rhs) < EPSILON;
>  };
>  
> +exports.EPSILON = EPSILON;

Why not Number.EPSILON?
Attachment #8544375 - Flags: review?(vporof) → review+
Comment on attachment 8544375 [details] [diff] [review]
1118071-automation-library-update.patch

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

::: toolkit/devtools/server/actors/utils/automation-timeline.js
@@ +67,5 @@
>  exports.fuzzyEqual = function (lhs, rhs) {
>    return Math.abs(lhs - rhs) < EPSILON;
>  };
>  
> +exports.EPSILON = EPSILON;

For less precision in some calculations, as well as browsers that do not have Number.EPSILON (Safari is a culprit here in overlap between web audio support and no EPSILON)
No longer blocks: 1108928
Comment on attachment 8544375 [details] [diff] [review]
1118071-automation-library-update.patch

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

::: browser/devtools/webaudioeditor/test/browser_audionode-actor-get-automation-data-02.js
@@ +24,2 @@
>  
> +  is(events.length, 4, "8 recorded events returned.");

s/8/4/, or there is maybe something non obvious?

::: toolkit/devtools/server/actors/utils/automation-timeline.js
@@ +324,5 @@
> +  // the `setTargetAtTime` node.
> +  var lastEvent = this._getPreviousEvent(event.time - F.EPSILON);
> +
> +  // If no event before the setTargetAtTime event, then return the
> +  // built in value.

not sure about the term built-in. In spec terms, we are not sure as well, but the "intrinsic" value is what I'd call it.

@@ +328,5 @@
> +  // built in value.
> +  if (!lastEvent) {
> +    return this._value;
> +  }
> +  // Otheriwse, return the value for the previous event, which should

nit: s/Otheriwse/Otherwise/

@@ +329,5 @@
> +  if (!lastEvent) {
> +    return this._value;
> +  }
> +  // Otheriwse, return the value for the previous event, which should
> +  // always be the last computed value (? I think?)

So, what if we did a setTargetAtTime while ramping up using a linearRampToValueAtTime ? i.e. before the end of the ramp:

audioparam.linearRampToValueAtTime(1.0, currentTime + 1.0);
audioparam.setTargetAtTime(0.0, currentTime + 0.5);

That was the issue I had, and the reason why I introduced a state.
Attachment #8544375 - Flags: feedback?(padenot)
Paul, in the case that you listed, I graphed this out with this library:
http://i.imgur.com/cIqItpc.png

With an intrinsic value of 0.5:
timeline.linearRampToValueAtTime(1.0, 1.0)
timeline.setTargetAtTime(0.0, 0.5, 0.1)

The computation seems to correctly display what the current implementation does -- it approaches the target at time, as `T1` in the exponentially approaching calc is `1` (the linear ramp's time), so then immediately "snaps" to the linear ramp's value -- the linear ramp doesn't seem to have any affect other than "snapping" to that value at that time. In fact, switching out the linear ramp with an exponential ramp, or set value at time, seems to have no different in an implementation (based off of what I'm hearing). So I think that the graph case IS, in fact, representing the curve correctly, if I'm understand this correctly!
Updated with your nits, Paul.

I'm going to checkin-needed this for now, and if you have comments on the previous example I've given, then can improve those in another bug for another case.
Attachment #8544375 - Attachment is obsolete: true
Attachment #8545456 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/b336e9272939
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b336e9272939
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.