888271 - Don't offset twice by mOffset in AudioBufferSourceNode

Status: RESOLVED FIXED

Description

[Paul Adenot (:padenot)]

Created attachment 768928 [details] [diff] [review]
Don't offset by mOffset twice.

We do that when we `buffer.loop == false`.

Comment 1

[Paul Adenot (:padenot)]

Created attachment 768930 [details] [diff] [review]
Don't send a duration offset by mOffset to the stream. r=

With intended changes only, now. Test incoming.

Comment 2

[bhavana bajaj [:bajaj]]

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.

Comment 3

[Paul Adenot (:padenot)]

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.

Comment 4

[bhavana bajaj [:bajaj]]

Tracking, as we do not want to ship webaudio without this bug resolved.

Comment 5

[Paul Adenot (:padenot)]

Created attachment 772551 [details] [diff] [review]
Don't substract twice by mOffset in AudioBufferSourceNode. r=

Now with a test.

Comment 6

[:Ehsan Akhgari]

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.

Comment 7

[Paul Adenot (:padenot)]

Pushed with some logging.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4863e8d1f0a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Increase the errorRate allowance to avoid intermittent test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/242e0891cd29

Comment 9

[:Ehsan Akhgari]

(In reply to comment #8)
> Increase the errorRate allowance to avoid intermittent test failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/242e0891cd29

Thanks!

Comment 10

[Paul Adenot (:padenot)]

I made the test even fuzzier because of failures. Note that it still tests the patch: without this, |samplesFromSource| would be 0.

https://hg.mozilla.org/integration/mozilla-inbound/rev/13b49b25ebda

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4863e8d1f0a
https://hg.mozilla.org/mozilla-central/rev/242e0891cd29
https://hg.mozilla.org/mozilla-central/rev/13b49b25ebda

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e7172fc9aa4

Comment 14

[Anthony Hughes (:ashughes) [QA]]

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.