Closed Bug 1110344 Opened 6 years ago Closed 6 years ago

setValueCurveAtTime is off by one frame

Categories

(Core :: Web Audio, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jsantell, Assigned: baptiste.em)

Details

Attachments

(2 files)

Attached file 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 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.
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
Priority: -- → P2
Hi, I take a look and it seems that the test was a false positive indeed.
Attachment #8614592 - Flags: review?(padenot)
Comment on attachment 8614592 [details] [diff] [review]
bug_1110334.patch.v1

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

Good catch with the type!
Attachment #8614592 - Flags: review?(padenot) → review+
Paul -- r+'d patch - is anything blocking this?  Thanks
Rank: 25
Flags: needinfo?(padenot)
I just forgot to push it for some reason.
(clearing NI)
Flags: needinfo?(padenot)
Assignee: nobody → baptiste.em
https://hg.mozilla.org/mozilla-central/rev/5978282a3c0a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.