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

RESOLVED FIXED in Firefox 32

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

unspecified
mozilla34
x86_64
Windows 8
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox31 wontfix, firefox32+ verified, firefox33+ verified, firefox34+ fixed, firefox-esr24 unaffected, firefox-esr31 wontfix)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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).
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1046270
(Assignee)

Comment 2

4 years ago
Created attachment 8475880 [details] [diff] [review]
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.
Assignee: nobody → paul
Attachment #8475880 - Flags: review?(cpearce)
(Assignee)

Comment 3

4 years ago
Created attachment 8475882 [details] [diff] [review]
test that we can decode an mp3
Attachment #8475882 - Flags: review?(ehsan)
(Assignee)

Comment 4

4 years ago
Created attachment 8475883 [details] [diff] [review]
fix-mp3-com-windows

And now without forgetting to hg add the new files.
Attachment #8475880 - Attachment is obsolete: true
Attachment #8475880 - Flags: review?(cpearce)
Attachment #8475883 - Flags: review?(cpearce)
status-firefox31: --- → wontfix
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox-esr24: --- → unaffected
status-firefox-esr31: --- → affected
tracking-firefox32: --- → +
tracking-firefox33: --- → +
tracking-firefox34: --- → +
tracking-firefox-esr31: --- → 32+

Comment 5

4 years ago
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...
Attachment #8475882 - Flags: review?(ehsan) → review+

Comment 6

4 years ago
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.
Flags: needinfo?(benjamin)

Comment 7

4 years ago
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.
Flags: needinfo?(benjamin)

Comment 8

4 years ago
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

4 years ago
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...
Attachment #8475883 - Flags: review?(cpearce) → review+
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.
(Assignee)

Comment 14

4 years ago
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.
Flags: needinfo?(paul)
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Comment 17

4 years ago
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
Attachment #8475883 - Flags: approval-mozilla-beta?
Attachment #8475883 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 18

4 years ago
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.
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.
Attachment #8475883 - Flags: approval-mozilla-beta?
Attachment #8475883 - Flags: approval-mozilla-beta+
Attachment #8475883 - Flags: approval-mozilla-aurora?
Attachment #8475883 - Flags: approval-mozilla-aurora+
(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).
Verified on Fx32b9(build1) and latest Aurora using the information in comment #18. Great success.
status-firefox32: fixed → verified
status-firefox33: fixed → verified
Excellent, thanks for everyone's help in getting this fixed and into Beta so quickly!
This does not meet ESR landing criteria.
status-firefox-esr31: affected → wontfix
tracking-firefox-esr31: 32+ → ---
You need to log in before you can comment on or make changes to this bug.