787788 - Downmixing weights for 6.1 Opus files are wrong

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Timothy B. Terriberry (:derf)]

-        /*7*/{ {0.4553f,0}, {0.322f,0.322f}, {0,4553}, {0.3943f,0.2277f}, {0.2277f,0.3943f}, {0.2788f,0.2788f}, {0.322f,0.322f}},
+        /*7*/{ {0.4553f,0}, {0.322f,0.322f}, {0,0.4553f}, {0.3943f,0.2277f}, {0.2277f,0.3943f}, {0.2788f,0.2788f}, {0.322f,0.322f}},

"Oops."

Comment 1

[Timothy B. Terriberry (:derf)]

Attachment: Fix downmixing weights for 6.1 Opus files

Comment 2

[Ralph Giles (:rillian)]

Comment on attachment 657665 [details] [diff] [review]
Fix downmixing weights for 6.1 Opus files

Oops indeed!

Comment 3

[Timothy B. Terriberry (:derf)]

Greenish on try: https://tbpl.mozilla.org/?tree=Try&rev=8c17144e8f18
https://hg.mozilla.org/integration/mozilla-inbound/rev/610d4f3abeeb

Comment 4

[Timothy B. Terriberry (:derf)]

Comment on attachment 657665 [details] [diff] [review]
Fix downmixing weights for 6.1 Opus files

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 748144
User impact if declined: 6.1 channel Opus files will produce very loud/nasty clipping/static
Testing completed (on m-c, etc.): Tested locally, greenish on Try, pushed to m-i
Risk to taking this patch (and alternatives if risky): Low. This changes a single constant in a table.
String or UUID changes made by this patch: None.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/610d4f3abeeb

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Let's let this bake a bit more on nightly and then revisit for branch uplift.

Comment 7

[Ralph Giles (:rillian)]

Thanks for reviewing!

Comment 8

[Timothy B. Terriberry (:derf)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/26534a98b05b

Comment 9

[u279076]

Timothy, can you please verify if this is now fixed for you in Firefox 17 and 18?

Comment 10

[Ralph Giles (:rillian)]

Verified fixed on MacOS X 10.7.5:

 Aurora 18.0a2 (2012-10-17)
 Beta 17.0b2

Comment 11

[Timothy B. Terriberry (:derf)]

For reference, I have a 6.1 test file here:
http://people.xiph.org/~tterribe/opus/multichannel/test-7-6.1.opus

Comment 12

[u279076]

Thanks Ralph and Timothy. Given we have a test file, is this something we can have an automated test for? If not, do we require a manual regression test?

Comment 13

[Ralph Giles (:rillian)]

For now this requires a manual regression test, since someone needs to listen to the test file and verify correct playback.

I've thought about adding some sort of automated verification of decoding quality, but that's likely to be both expensive to run, and require lossless reference audio in the test suite, to catch anything but the most egregious errors.