Closed Bug 964376 Opened 6 years ago Closed 6 years ago

AudioBufferSourceNode.playbackRate does not sound great anymore

Categories

(Core :: Web Audio, defect, P1)

29 Branch
x86_64
All
defect

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox28 --- unaffected
firefox29 - affected

People

(Reporter: padenot, Assigned: karlt)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

STR:
- Open Firefox release
- Run http://padenot.github.io/loopa/
- Click play
- Change the playbackRate value
- Open Firefox Nightly
- Run http://padenot.github.io/loopa/
- Click play
- Change the playbackRate value

Expected:
The sound quality is more or less equivalent

Actual result:
It sounds horrible in nightly
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/c9b60fc06e4c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140107082035
Bad:
http://hg.mozilla.org/mozilla-central/rev/8cdeb1bcdb02
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140107082333
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c9b60fc06e4c&tochange=8cdeb1bcdb02


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/029a56c4ad4c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140106192108
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/90d56ff72f1a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140106193422
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=029a56c4ad4c&tochange=90d56ff72f1a

Suspected: Bug 943461,Bug 947796
Blocks: 943461, 947796
OS: Linux → All
Version: unspecified → 29 Branch
Priority: -- → P1
Assignee: nobody → karlt
Some code was depending on this for correct operation, but then failed in
non-debug builds.

Instead, distort only mDuration to check that ProduceAudioBlock sets chunks.

The following crashtests then detect this bug:
874915.html
874934.html
880342-1.html
offline-buffer-source-ended-1.html
Attachment #8366988 - Flags: review?(roc)
(Previous was wrong patch.)
Attachment #8366992 - Attachment is obsolete: true
Attachment #8366992 - Flags: review?(paul)
Attachment #8366993 - Flags: review?(paul)
Attachment #8366993 - Flags: review?(paul) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3660c2bcacbb
https://hg.mozilla.org/integration/mozilla-inbound/rev/d588d0299cb7

This particular bug would now show up as an assertion failure, but it's existence demonstrates that we don't have a test to check output from a looping buffer source after the playback rate changes from unit to non-unit.
Flags: in-testsuite?
Karl, I came to the same conclusion and wrote somewhat of a test for that. I'll clean it up, double check it, and upload it here.
Did you have something else in mind? This basically gives us the fundamental of the 440 sine wave, when slowed down by a factor of 50%.

It works here, but because of audio glitches, we would need to apply some more filtering.
Attachment #8367275 - Flags: feedback?(karlt)
https://hg.mozilla.org/mozilla-central/rev/3660c2bcacbb
https://hg.mozilla.org/mozilla-central/rev/d588d0299cb7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
I'm convinced that this if unfixed would have impacted a small segment of our users since it was fixed I don't think we need to track this at this time.
Comment on attachment 8367275 [details]
test_audioBufferSourceNodePlaybackRate.html

I think this particular bug only happened when playback had started with playbackRate = 1.

I think we can test precise output in the time domain, if we bear in mind that the playbackRate change happens on a 128 sample boundary and ensure that the buffer loops after the change in playbackRate.
Attachment #8367275 - Flags: feedback?(karlt)
I've asked rfw to look at making the changes in comment 11 and turning the test into a mochitest.
QA Contact: tony
I've unapplied the patch to see the behavior in the latest Nightly, but changing the playback rate results in an assertion failure, crashing Firefox:

    ###!!! ASSERTION: Invalid WebAudio chunk size: 'mLastChunks[i].GetDuration() == WEBAUDIO_BLOCK_SIZE', file /Users/Tony/mozilla-central/content/media/AudioNodeStream.cpp, line 445
Apologies, here's the one that actually crashed Firefox:

    Assertion failure: aDuration >= 0, at /Users/Tony/mozilla-central/media/webrtc/signaling/../../../content/media/MediaSegment.h:289
My bad, I didn't unapply both patches.
Attached file test_bug964376.html
This mochitest test case for whether or not the sound is distorted by altering the playback rate after a given amount of samples (in this case 128). It creates the expected waveform and compares the two. With the patch unapplied, there is a massive difference between the two waveforms.
Attachment #8367275 - Attachment is obsolete: true
Thanks, Tony.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a310ac56a47b
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.