1056032 - Make sure COM is initialized when trying to decode an mp3 using decodeAudioData

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Paul Adenot (:padenot)]

Numerous people reported that mp3 decoding using decodeAudioData stopped working in Firefox 31, and it's probably because we switched from SharedThreadPool to nsIThreadPool or something (bug 821062).

Comment 2

[Paul Adenot (:padenot)]

Attachment: patch

This splits of the thread pool listener in its own file and use it in the same way you do it for the SharedThreadPool.

Comment 3

[Paul Adenot (:padenot)]

Attachment: test that we can decode an mp3

Comment 4

[Paul Adenot (:padenot)]

Attachment: fix-mp3-com-windows

And now without forgetting to hg add the new files.

Comment 5

[(no longer active)]

Comment on attachment 8475882 [details] [diff] [review]
test that we can decode an mp3

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

I assume this works on all platforms now...

Comment 6

[(no longer active)]

Is there any reason why we don't unconditionally initialize COM *always*?  This is super error prone, I'd prefer an alternative fix which addresses this at the more fundamental level.

Comment 7

[Benjamin Smedberg]

What thread is this running on? COM initialization is per-thread, and I'm about 99% certain that we already initialize COM on the main thread. But each worker thread that wants to use COM is responsible for initializing it themself.

Comment 8

[(no longer active)]

As discussed on IRC, taking this fix for now is fine.  But I think we should modify everywhere that we create threads to do this unconditionally.

Comment 9

[Benjamin Smedberg]

Oh, auto-initialize COM on all threads? No, that's not a good idea. We don't necessarily know what threading/apartment model various code will need, and the per-thread memory overhead is likely non-trivial.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8475883 [details] [diff] [review]
fix-mp3-com-windows

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

You guys should just use SharedThreadPool...

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yeah, the main thread is definitely inited with single compartment COM, whereas we need multi-threaded COM for DirectShow/WMF. We had a previous media bug on file for that.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15caaf563783
https://hg.mozilla.org/integration/mozilla-inbound/rev/18b0e0c795d4

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Had to back these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/83688052844e for Android m3 bustage: 

https://tbpl.mozilla.org/php/getParsedLog.php?id=46399245&tree=Mozilla-Inbound

Comment 14

[Paul Adenot (:padenot)]

Yay, another platform where we did not know mp3 did not work in decodeAudioData.

I'm going to reland with the test disabled there, and open another bug for investigation, because the issue on Android can't be related.

Comment 15

[Paul Adenot (:padenot)]

(In reply to Chris Pearce (:cpearce) from comment #10)
> Comment on attachment 8475883 [details] [diff] [review]
> fix-mp3-com-windows
> 
> Review of attachment 8475883 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You guys should just use SharedThreadPool...

Yes, and we did, but it caused shutdown hangs and had to be backed out. This is intended to be short a short term / easily upliftable patch.

The real fix is indeed to reland the SharedThreadPool patch, and make sure there is no shutdown hang anymore. Maybe Yoric's async shutdown patches will help.

Comment 16

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2423b6217200
https://hg.mozilla.org/integration/mozilla-inbound/rev/232e907a893f

Bug 1056706 has been opened for investigation.

Comment 17

[Paul Adenot (:padenot)]

Comment on attachment 8475883 [details] [diff] [review]
fix-mp3-com-windows

Approval Request Comment
[Feature/regressing bug #]: bug 1026325
[User impact if declined]: mp3 can't be decoded using decodeAudioData on Windows, this prevents a large number or games and other web application to play sound
[Describe test coverage new/current, TBPL]: a regression test has been added
[Risks and why]: Low risk, this is using code that is exercised elsewhere in the tree, and re-implements a behavior the code used to have before 1026325
[String/UUID change made/needed]: none

Comment 18

[Paul Adenot (:padenot)]

One can verify this is fix on a particular build by using the page http://paul.cx/public/1056032.html. 

Once the test has completed (very short, almost instantaneous), a word will appear in the page:

"Success": means the functionality works as intended, we can decode an mp3 file using decodeAudioData.
"Failure": means the functionality does not work as intended, we can't decode an mp3 file using decodeAudioData.

A Windows machine is required to test this.

Firefox 30 should show "Success".
Firefox 31 should show "Failure" (because of the regression).
A Firefox build >= 31, but that has the patch of this bug should show "Success" (because the regression is fixed).

Comment 19

[Ryan VanderMeulen [:RyanVM]]

FYI, this can be considered as "stuck" on inbound for approval purposes. It will be included in the next merge to m-c.

Comment 20

[juan becerra [:juanb]]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1408621635/firefox-34.0a1.en-US.win32.zip should have the fix. I will test this shortly.

Comment 21

[juan becerra [:juanb]]

I tested this using the information in comment #18.

Fx30: "Success"
Fx31: "Failure"
Fx32.0b8: "Failure"
mozilla-inbound build from comment #20: "Success"

So the patch should be good for m-c.

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8475883 [details] [diff] [review]
fix-mp3-com-windows

I'll take the verification on inbound. Approved for Aurora and Beta.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5eee74e00487
https://hg.mozilla.org/releases/mozilla-aurora/rev/d6b8c53504b3

https://hg.mozilla.org/releases/mozilla-beta/rev/f17ade17a846
https://hg.mozilla.org/releases/mozilla-beta/rev/53d300e03f5b

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Paul Adenot (:padenot) from comment #15)
> The real fix is indeed to reland the SharedThreadPool patch, and make sure
> there is no shutdown hang anymore. Maybe Yoric's async shutdown patches will
> help.

We should just require a minimum amount of plumbing to ensure SharedThreadPool works here, IIRC we just need to ensure that WebAudio registers with the MediaShutdownManager too, so that it knows to enforce shutdown.

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

(And yes, I think it would be good to use Yoric's async shutdown rather than rolling our own every time).

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2423b6217200
https://hg.mozilla.org/mozilla-central/rev/232e907a893f
https://hg.mozilla.org/mozilla-central/rev/6ded38cc54e3

Comment 27

[juan becerra [:juanb]]

Verified on Fx32b9(build1) and latest Aurora using the information in comment #18. Great success.

Comment 28

[Maire Reavy [:mreavy]]

Excellent, thanks for everyone's help in getting this fixed and into Beta so quickly!

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

This does not meet ESR landing criteria.