Closed Bug 1056032 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Web Audio, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox31 --- wontfix
firefox32 + verified
firefox33 + verified
firefox34 + fixed
firefox-esr24 --- unaffected
firefox-esr31 --- wontfix

People

(Reporter: padenot, Assigned: padenot)

References

Details

Attachments

(2 files, 1 obsolete file)

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).
Duplicate of this bug: 1046270
Attached patch patch (obsolete) — Splinter Review
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)
Attachment #8475882 - Flags: review?(ehsan)
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)
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+
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)
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)
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...
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.
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)
(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 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?
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.
Excellent, thanks for everyone's help in getting this fixed and into Beta so quickly!
This does not meet ESR landing criteria.
Depends on: 1056706
You need to log in before you can comment on or make changes to this bug.