Closed Bug 999267 Opened 6 years ago Closed 6 years ago
Segment::Resample() skips input samples
Detected by test_mediaElementAudioSourceNode.html after adding MOZ_ASSERT(inFrames == c.mDuration); at http://hg.mozilla.org/mozilla-central/annotate/5010b38abf18/content/media/AudioSegment.h#l203
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Attachment #8409979 - Flags: review?(paul)
Comment on attachment 8409979 [details] [diff] [review] patch Review of attachment 8409979 [details] [diff] [review]: ----------------------------------------------------------------- Good catch, thanks.
Attachment #8409979 - Flags: review?(paul) → review+
The previous version was demonstrating frequent instances of bug 750258 and bug 967606. https://tbpl.mozilla.org/?tree=Try&rev=18c97956d21a https://tbpl.mozilla.org/?tree=Try&rev=2357ccf39b50 I noticed that the patch was not handling different channels consistently, and I suspect this could produce different results with the resampler stopping at the earlier of consuming input or filling output. This version depends on bug 997152.
Attachment #8409979 - Attachment is obsolete: true
Hmm. Still the same test failures. https://tbpl.mozilla.org/?tree=Try&rev=b8f80bc28c15 https://tbpl.mozilla.org/?tree=Try&rev=5f96699eed9f
Oops, I r+ed the wrong patch.
Bug 1010000 has a fix for the test failures. https://tbpl.mozilla.org/?tree=Try&rev=f70e095bc1e8 https://tbpl.mozilla.org/?tree=Try&rev=5d967ba4400a
Regression in quality of MediaElementAudioSourceNode and getUserMedia().
Paul, it would be good to fix this before it reaches too many beta builds, but I still think this second patch is the better fix. Is there anything I can do to help with review? (In reply to Karl Tomlinson (needinfo?:karlt) from comment #3) > I noticed that the patch was not handling different channels consistently, > and I suspect this could produce different results with the resampler > stopping at the earlier of consuming input or filling output. What I'm addressing here is that sometimes the resampler needs to be able to read extra input samples without producing any output samples. If that happens with the first patch, outFrames would be reset after the first channel to the number of out frames that were produced. For the next channel, that limit on output can lead to olen being 0 at http://hg.mozilla.org/mozilla-central/annotate/882826199076/media/libspeex_resampler/src/resample.c#l891 which would mean that remaining available input would not be read.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd5d24cb3246 This can be tested with Web Audio but bug 983062 and bug 983023 mean we don't get the expected results at the beginning and end of a track. A file with some silence at each end could be used, if those bugs are not fixed.
Comment on attachment 8410867 [details] [diff] [review] v2, handling of multiple channels [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 818822 User impact if declined: Regression in quality of MediaElementAudioSourceNode and getUserMedia(). Testing completed (on m-c, etc.): on m-c Risk to taking this patch (and alternatives if risky): Moderate risk due to some assumptions about the parameters passed to this method. Those assumptions are already present in the code, but this patch could change behavior if those assumptions are violated. String or IDL/UUID changes made by this patch: none.
Attachment #8410867 - Flags: approval-mozilla-beta?
Attachment #8410867 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Is there a way QA could manually verify this? Thank you!
This should sound like playing 440.ogg directly (expect for a click every few seconds or so due to other bugs). Before this bug was fixed there was a chatter from the distortion.
Thanks for the testcase, Karl. I could hear the distortion using a Nightly 31 build from 2014-04-17 very well on Win 7 and Ubuntu, but it wasn't so clear on Mac. That distortion is no longer present on Firefox 31 beta 6 and latest Aurora 32.0a2 20140702004004 under Win 7 64-bit and Ubuntu 12.10 32-bit although from time to time a small distortion sound can be heard. Marking as verified since the behavior is very much improved.
You need to log in before you can comment on or make changes to this bug.