Closed Bug 888271 Opened 11 years ago Closed 11 years ago

Don't offset twice by mOffset in AudioBufferSourceNode


(Core :: Web Audio, defect)

Not set



Tracking Status
firefox23 --- unaffected
firefox24 + fixed
firefox25 + fixed


(Reporter: padenot, Assigned: padenot)



(Whiteboard: [blocking-webaudio+][qa-])


(2 files, 1 obsolete file)

Attached patch Don't offset by mOffset twice. (obsolete) — Splinter Review
We do that when we `buffer.loop == false`.
Attachment #768928 - Flags: review?(ehsan)
With intended changes only, now. Test incoming.
Attachment #768930 - Flags: review?(ehsan)
Attachment #768928 - Attachment is obsolete: true
Attachment #768928 - Flags: review?(ehsan)
Attachment #768930 - Flags: review?(ehsan) → review+
Can you please explain the user impact here for us to make sure this is tracking worthy ? Not clear why we'd track, but we'd take a low risk uplift if needed for Aurora.
Well, this is a bug in our implementation. A developer willing to use the API would see weird behavior trying to start and AudioBufferSourceNode, the sound stopping at unexpected time.

I've tested this manually, but as I said in comment 1, I'll write a mochitest.
Tracking, as we do not want to ship webaudio without this bug resolved.
Whiteboard: [blocking-webaudio+]
Now with a test.
Attachment #772551 - Flags: review?(ehsan)
Comment on attachment 772551 [details] [diff] [review]
Don't substract twice by mOffset in AudioBufferSourceNode. r=

Review of attachment 772551 [details] [diff] [review]:

::: content/media/webaudio/test/test_audioBufferSourceNodeOffset.html
@@ +30,5 @@
> +  source.onended = function(e) {
> +    // The timing at which the audioprocess and ended listeners are called can
> +    // change, hence the fuzzy equal here.
> +    var errorRatio = samplesFromSource / (0.5 * context.sampleRate);
> +    ok(errorRatio > 0.97 && errorRatio < 1.03, "Correct number of samples received.");

Please make this log the value of errorRatio as well, since this might fail intermittently I believe, and that would make it easier for us to adjust the value accordingly.
Attachment #772551 - Flags: review?(ehsan) → review+
Increase the errorRate allowance to avoid intermittent test failures.
(In reply to comment #8)
> Increase the errorRate allowance to avoid intermittent test failures.

I made the test even fuzzier because of failures. Note that it still tests the patch: without this, |samplesFromSource| would be 0.
Keywords: checkin-needed
Assuming this does not need QA verification due to in-testsuite coverage. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs to be manually verified.
Whiteboard: [blocking-webaudio+] → [blocking-webaudio+][qa-]
Depends on: 895720, 906752
You need to log in before you can comment on or make changes to this bug.