999267 - AudioSegment::Resample() skips input samples

Status: VERIFIED FIXED

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

Description

[Karl Tomlinson (:karlt)]

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

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: patch

Comment 2

[Paul Adenot (:padenot)]

Comment on attachment 8409979 [details] [diff] [review]
patch

Review of attachment 8409979 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch, thanks.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: v2, handling of multiple channels

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.

Comment 4

[Karl Tomlinson (:karlt)]

Hmm.  Still the same test failures.
https://tbpl.mozilla.org/?tree=Try&rev=b8f80bc28c15
https://tbpl.mozilla.org/?tree=Try&rev=5f96699eed9f

Comment 5

[Paul Adenot (:padenot)]

Oops, I r+ed the wrong patch.

Comment 6

[Karl Tomlinson (:karlt)]

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

Comment 7

[Karl Tomlinson (:karlt)]

Regression in quality of MediaElementAudioSourceNode and getUserMedia().

Comment 8

[Benjamin Kerensa [:bkerensa]]

Tracking this for now.

Comment 9

[Karl Tomlinson (:karlt)]

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.

Comment 10

[Karl Tomlinson (:karlt)]

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 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bd5d24cb3246

Comment 12

[Karl Tomlinson (:karlt)]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6c748dab47b8

Comment 14

[Petruta Horea [:phorea], Desktop Test Engineering]

Is there a way QA could manually verify this? Thank you!

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: 440.ogg (for testcase)

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: testcase

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.

Comment 17

[Karl Tomlinson (:karlt)]

Attachment: testcase

Comment 18

[Petruta Horea [:phorea], Desktop Test Engineering]

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.