[EME] gmp-clearkey doesn't (and can't) use WMF to decode on Win Vista

RESOLVED FIXED in Firefox 38

Status

()

P2
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 1 bug)

unspecified
mozilla40
x86_64
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox38 fixed, firefox38.0.5 fixed, firefox39 fixed, firefox40 fixed, firefox-esr38 fixed)

Details

Attachments

(4 attachments)

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

4 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

4 years ago
Created attachment 8594566 [details] [diff] [review]
Patch: Expand list of WMF DLLs allowed for preloading by GMPs

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

4 years ago
Created attachment 8594568 [details] [diff] [review]
Patch: Make gmp-clearkey decode on Vista

* Use the right DLL for the right Windows version.
* Fail if we can't init WMF in gmp-clearkey.
Attachment #8594568 - Flags: review?(edwin)
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

4 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/mozilla-central/rev/d9cee33500ba
https://hg.mozilla.org/mozilla-central/rev/9c9759bb9ae8
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 8

4 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

4 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

4 years ago
Created attachment 8595126 [details] [diff] [review]
[Aurora & Beta] Patch: Expand list of WMF DLLs allowed for preloading by GMPs

Patch 1, rebased to apply on both Aurora and Beta. Ready to be uplifted.
Attachment #8595126 - Flags: review+
(Assignee)

Comment 11

4 years ago
Created attachment 8595127 [details] [diff] [review]
[Aurora & Beta] Patch: Make gmp-clearkey decode on Vista

Patch 2, rebased to apply on both Aurora and Beta. Ready to be uplifted.
Attachment #8595127 - Flags: review+
status-firefox38: --- → affected
status-firefox39: --- → affected
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+
Attachment #8594568 - Flags: approval-mozilla-beta?
Attachment #8594568 - Flags: approval-mozilla-beta+
Attachment #8594568 - Flags: approval-mozilla-aurora?
Attachment #8594568 - Flags: approval-mozilla-aurora+
Flags: qe-verify-
(Assignee)

Comment 15

4 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)
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)
All 3 are 100% identical to each other and also identical to my trunk build from 24-Apr.
Going through the FTP, 38b7 was the first beta to contain this fix.
(Assignee)

Comment 19

4 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

4 years ago
I landed a trivial fixup for cross compiling on case sensitive OSes.
status-firefox38.0.5: --- → fixed
Depends on: 1329547
You need to log in before you can comment on or make changes to this bug.