Closed
Bug 1156131
Opened 9 years ago
Closed 9 years ago
[EME] gmp-clearkey doesn't (and can't) use WMF to decode on Win Vista
Categories
(Core :: Audio/Video, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: cpearce, Assigned: cpearce)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
1.24 KB,
patch
|
bobowen
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
11.13 KB,
patch
|
eflores
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
11.13 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
1.35 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
gmp-clearkey doesn't DecryptAndDecode on Win Vista; it fails to initialize, and falls back to just Decrypting and Gecko doing the decoding. We should use the same code path that Adobe's CDM will use, so that we ensure that we don't regress anything that Adobe depends on, and we should ensure that it's possible to use WMF inside the sandbox on Vista.
Assignee | ||
Comment 1•9 years ago
|
||
The problems here are: * On Vista the H.264 decoder is in mfh264dec.dll, and the AAC decoder is in mfheaacdec.dll, and we don't allow, and gmp-clearkey doesn't ask for, those to be preloaded. * We have a call to MFGetStrideForBitmapInfoHeader which gets hit on Vista (but not on other platforms I believe) and that function lives in evr.dll on Vista, whereas we assumed it was in mfplat.dll. On Win7 or later MFGetStrideForBitmapInfoHeader lives in mfplat.dll, and evr.dll exports a stub for MFGetStrideForBitmapInfoHeader that calls into mfplat.dll. So we can afford to only look for MFGetStrideForBitmapInfoHeader in evr.dll. evr.dll needs to be added to our whitelist. * We also didn't include mfplat.dll in the whitelist, so it wasn't being loaded on Vista (it must be being pulled in by some other DLL on other Windows versions). So none of the WMF functions were working on Vista anyway. * We didn't realise that WMF wasn't working inside the sandbox because if we failed to load WMF we'd fallback to let Gecko decode. We should abort if we can't load WMF when we think we should be able to.
Assignee | ||
Comment 2•9 years ago
|
||
Expand the whitelist of DLLs allowed to be preloaded by GMPs. We need these to make decoding work on Vista. See comment #1 for a more detailed explanation...
Attachment #8594566 -
Flags: review?(bobowen.code)
Assignee | ||
Comment 3•9 years ago
|
||
* Use the right DLL for the right Windows version. * Fail if we can't init WMF in gmp-clearkey.
Attachment #8594568 -
Flags: review?(edwin)
Attachment #8594568 -
Flags: review?(edwin) → review+
Comment 4•9 years ago
|
||
Comment on attachment 8594566 [details] [diff] [review] Patch: Expand list of WMF DLLs allowed for preloading by GMPs Review of attachment 8594566 [details] [diff] [review]: ----------------------------------------------------------------- r=bobowen with the nit addressed. Also, ... (In reply to Chris Pearce (:cpearce) from comment #1) > The problems here are: > * We also didn't include mfplat.dll in the whitelist, so it wasn't being > loaded on Vista (it must be being pulled in by some other DLL on other > Windows versions). So none of the WMF functions were working on Vista anyway. mfplat.dll isn't in the "Libraries:" line in clearkey.info.in in the other patch, should it be? ::: dom/media/gmp/GMPChild.cpp @@ +311,5 @@ > "d3d9.dll", // Create an `IDirect3D9` to get adapter information > "dxva2.dll", // Get monitor information > + "evr.dll", // MFGetStrideForBitmapInfoHeader > + "mfheaacdec.dll", // AAC decoder (on Windows Vista) > + "mfh264dec.dll", // H.264 decoder (on Windows Vista) nit: If I remember correctly, our search through the whitelist doesn't depend on it at the moment, but given the comment at the top, this line should be before the one above.
Attachment #8594566 -
Flags: review?(bobowen.code) → review+
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #4) > Comment on attachment 8594566 [details] [diff] [review] > Patch: Expand list of WMF DLLs allowed for preloading by GMPs > > Review of attachment 8594566 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=bobowen with the nit addressed. > > Also, ... > > (In reply to Chris Pearce (:cpearce) from comment #1) > > The problems here are: > > > * We also didn't include mfplat.dll in the whitelist, so it wasn't being > > loaded on Vista (it must be being pulled in by some other DLL on other > > Windows versions). So none of the WMF functions were working on Vista anyway. > > mfplat.dll isn't in the "Libraries:" line in clearkey.info.in in the other > patch, should it be? Hmm... We seem to be working without it, but for completeness, I think yes, it should be there. > > ::: dom/media/gmp/GMPChild.cpp > @@ +311,5 @@ > > "d3d9.dll", // Create an `IDirect3D9` to get adapter information > > "dxva2.dll", // Get monitor information > > + "evr.dll", // MFGetStrideForBitmapInfoHeader > > + "mfheaacdec.dll", // AAC decoder (on Windows Vista) > > + "mfh264dec.dll", // H.264 decoder (on Windows Vista) > > nit: If I remember correctly, our search through the whitelist doesn't > depend on it at the moment, but given the comment at the top, this line > should be before the one above. Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9cee33500ba https://hg.mozilla.org/integration/mozilla-inbound/rev/9c9759bb9ae8
https://hg.mozilla.org/mozilla-central/rev/d9cee33500ba https://hg.mozilla.org/mozilla-central/rev/9c9759bb9ae8
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8594566 [details] [diff] [review] Patch: Expand list of WMF DLLs allowed for preloading by GMPs Approval Request Comment [Feature/regressing bug #]: EME [User impact if declined]: Adobe EME won't work on Vista; Adobe's EME plugin will crash. [Describe test coverage new/current, TreeHerder]: We have no automated tests on Vista. I've tested locally. [Risks and why]: We've lots of EME tests, so I'm pretty confident this doesn't break anything on other platforms. [String/UUID change made/needed]: None.
Attachment #8594566 -
Flags: approval-mozilla-beta?
Attachment #8594566 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8594568 [details] [diff] [review] Patch: Make gmp-clearkey decode on Vista Approval Request Comment [Feature/regressing bug #]: EME [User impact if declined]: Adobe EME won't work on Vista; Adobe's EME plugin will crash. [Describe test coverage new/current, TreeHerder]: We have no automated tests on Vista. I've tested locally. [Risks and why]: We've lots of EME tests, so I'm pretty confident this doesn't break anything on other platforms. [String/UUID change made/needed]: None.
Attachment #8594568 -
Flags: approval-mozilla-beta?
Attachment #8594568 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 10•9 years ago
|
||
Patch 1, rebased to apply on both Aurora and Beta. Ready to be uplifted.
Attachment #8595126 -
Flags: review+
Assignee | ||
Comment 11•9 years ago
|
||
Patch 2, rebased to apply on both Aurora and Beta. Ready to be uplifted.
Attachment #8595127 -
Flags: review+
Updated•9 years ago
|
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Comment 12•9 years ago
|
||
Comment on attachment 8594566 [details] [diff] [review] Patch: Expand list of WMF DLLs allowed for preloading by GMPs Should in 38 beta 7
Attachment #8594566 -
Flags: approval-mozilla-beta?
Attachment #8594566 -
Flags: approval-mozilla-beta+
Attachment #8594566 -
Flags: approval-mozilla-aurora?
Attachment #8594566 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8594568 -
Flags: approval-mozilla-beta?
Attachment #8594568 -
Flags: approval-mozilla-beta+
Attachment #8594568 -
Flags: approval-mozilla-aurora?
Attachment #8594568 -
Flags: approval-mozilla-aurora+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/7b222c30b30f https://hg.mozilla.org/releases/mozilla-aurora/rev/4e71f335964f
Comment 14•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/e7210d2ce8a9 https://hg.mozilla.org/releases/mozilla-beta/rev/5712fefbace8
Updated•9 years ago
|
Flags: qe-verify-
Assignee | ||
Comment 15•9 years ago
|
||
I'm not sure what's going on here, but this change does not appear in my up-to-date Firefox Beta build. Specifically, there should be changes to the bin/gmp-clearkey/0.1/clearkey.info file that appear in the beta release candidate, but not in the Beta build on my system. What's going on here? When will these changesets be available on the Beta channel? Is this something to do with the early uplift of 38 to release?
Flags: needinfo?(sledru)
Flags: needinfo?(ryanvm)
Comment 16•9 years ago
|
||
This landed on beta prior to the merge to release. Assuming this is the change in question: https://hg.mozilla.org/releases/mozilla-beta/diff/e7210d2ce8a9/media/gmp-clearkey/0.1/clearkey.info.in I downloaded 38.0b8 (the newest official beta release) from FTP: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/38.0b8/win32/en-US/ --> Shows the same as the above diff. I downloaded the latest win32 dep build from mozilla-release: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-release-win32/1430164933/ --> Shows the same as the above diff. I downloaded the latest win32 dep build from mozilla-beta: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-win32/1430163974/ --> Shows the same as the above diff. Is there something I'm missing here?
Flags: needinfo?(sledru)
Flags: needinfo?(ryanvm)
Comment 17•9 years ago
|
||
All 3 are 100% identical to each other and also identical to my trunk build from 24-Apr.
Comment 18•9 years ago
|
||
Going through the FTP, 38b7 was the first beta to contain this fix.
Assignee | ||
Comment 19•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16) > This landed on beta prior to the merge to release. My analysis matched yours here. I asked in #relman, and lmandel said they had "some issues" and we should be shipping the next beta tomorrow assuming there's no more issues.
Comment 21•9 years ago
|
||
I landed a trivial fixup for cross compiling on case sensitive OSes.
Updated•9 years ago
|
status-firefox38.0.5:
--- → fixed
Comment 25•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr38/rev/b9f3bdfbf395
status-firefox-esr38:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•