Closed Bug 999267 Opened 5 years ago Closed 5 years ago

AudioSegment::Resample() skips input samples

Categories

(Core :: Audio/Video, defect, P1)

31 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox31 + verified
firefox32 + verified
b2g-v2.0 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

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
Attached patch patch (obsolete) — Splinter Review
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
Attachment #8410867 - Flags: review?(paul)
Depends on: 997152
Attachment #8410867 - Flags: review?(paul) → review+
Oops, I r+ed the wrong patch.
Attachment #8410867 - Flags: review+ → review?(paul)
Depends on: 967606
Depends on: 1010000
No longer depends on: 967606
Regression in quality of MediaElementAudioSourceNode and getUserMedia().
Tracking this for now.
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.
Flags: needinfo?(paul)
Attachment #8410867 - Flags: review?(paul) → review+
Flags: needinfo?(paul)
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.
Depends on: 983062, 983023
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/bd5d24cb3246
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Target Milestone: mozilla33 → mozilla32
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!
Flags: needinfo?(karlt)
Attached audio 440.ogg (for testcase)
Flags: needinfo?(karlt)
Attached file testcase (obsolete) —
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.
Attached file testcase
Attachment #8449781 - Attachment is obsolete: true
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: petruta.rasa
You need to log in before you can comment on or make changes to this bug.