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)

x86_64
Windows Vista
defect

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- fixed
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed
firefox-esr38 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.
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.
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)
* 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+
(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!
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?
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?
Patch 1, rebased to apply on both Aurora and Beta. Ready to be uplifted.
Attachment #8595126 - Flags: review+
Patch 2, rebased to apply on both Aurora and Beta. Ready to be uplifted.
Attachment #8595127 - Flags: review+
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-
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.
(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.
I landed a trivial fixup for cross compiling on case sensitive OSes.
Depends on: 1329547
See Also: → 1608657
You need to log in before you can comment on or make changes to this bug.