Closed Bug 964376 Opened 6 years ago Closed 6 years ago
Buffer Source Node .playback Rate does not sound great anymore
1.36 KB, patch
|Details | Diff | Splinter Review|
1.88 KB, patch
|Details | Diff | Splinter Review|
1.71 KB, text/html
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
No longer blocks: 947796
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)
Attachment #8366988 - Flags: review?(roc) → review+
(Previous was wrong patch.)
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.
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.
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.
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.
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
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.