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).

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

And now without forgetting to hg add the new files.

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...

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.

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.

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.

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 on attachment 8475883 [details] [diff] [review] fix-mp3-com-windows Review of attachment 8475883 [details] [diff] [review]: ----------------------------------------------------------------- You guys should just use SharedThreadPool...

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.

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

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

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.

(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.

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 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

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).

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

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.

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 on attachment 8475883 [details] [diff] [review] fix-mp3-com-windows I'll take the verification on inbound. Approved for Aurora and Beta.

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

(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.

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

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

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

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

This does not meet ESR landing criteria.