1118071 - setTargetAtTime curve calculating incorrectly after first calculation

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1118071-automation-library-update.patch

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

Comment 2

[Victor Porof [:vporof][:vp]]

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?

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

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)

Comment 4

[Paul Adenot (:padenot)]

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.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

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!

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1118071-automation-library-update.patch

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/b336e9272939

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b336e9272939