1110344 - setValueCurveAtTime is off by one frame

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

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

Attachment: automationbug.html

It appears the implementation of the curves are off by one frame. According to the spec, if I do:

param.setValueCurveAtTime(new Float32Array([-1, 0, 1]), 0.7, 0.3);

And plug in the values for how the curve is calculated:

v(t) = values[N * (t - startTime) / duration]
v(0.7) = [-1, 0, 1][3 * (0.7 - 0.7) / 0.3] = -1
v(0.8) = [-1, 0, 1][3 * (0.8 - 0.7) / 0.3] = 0
v(0.9) = [-1, 0, 1][3 * (0.9 - 0.7) / 0.3] = 1

I should get those results on exactly that time, but it doesn't change values until the frame after 0.7, 0.8, 0.9 etc.

Attached is a script that runs what Firefox produces on those values (0, -1, and 0).

Even the test also incorrectly tests that t=0.9 should equal `0`[1]. Chrome too has this issue. Pinging Paul, as we talked about this in PDX and wanting to confirm these assumptions are correct.

[1] https://github.com/mozilla/gecko-dev/blob/04a67ff6ab14fb9b49044a5c902673a440d7f222/dom/media/webaudio/compiledtest/TestAudioEventTimeline.cpp#L137

Comment 1

[Karl Tomlinson (:karlt)]

Comment on attachment 8535173 [details]
automationbug.html

>            var s = "Value at " + indexes[i] + "s: " + buf[Math.floor(indexes[i] * 44100)] + " (expected " + curve[i] + ")\n";

Check that floating point rounding error is not causing floor() to round the sample down.  It might also be Gecko that is not rounding well.

Comment 2

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

0.7 * 44100 = 30869.999999999996 => flooring => 30869
0.8 * 44100 = 35280 => flooring => 35280
0.9 * 44100 = 39690 =>> flooring => 39690

Looks like the rounding could be an issue for 0.7, which should be 30870 -- and this matches the test in `TestAudioEventTimeline.cpp`, so that's a false positive.

0.9, however, should be equal to `1`, and in the TestAudioEventTimeline case, asserts that it should be 0

Comment 3

[Baptiste Emmanuel]

Attachment: bug_1110334.patch.v1

Hi, I take a look and it seems that the test was a false positive indeed.

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 8614592 [details] [diff] [review]
bug_1110334.patch.v1

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

Good catch with the type!

Comment 5

[Maire Reavy [:mreavy]]

Paul -- r+'d patch - is anything blocking this?  Thanks

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5978282a3c0a

Comment 7

[Paul Adenot (:padenot)]

I just forgot to push it for some reason.

Comment 8

[Paul Adenot (:padenot)]

(clearing NI)

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/5978282a3c0a