Closed Bug 1143278 Opened 6 years ago Closed 6 years ago

[EME] gmp-clearkey doesn't decrypt_and_decode on Win7

Categories

(Core :: Audio/Video, defect, P2)

Product:
Core
Component:
Audio/Video
Platform:
x86_64
Windows 7
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Patch
2.66 KB, patch
eflores
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
Patch 2: Add more nullchecks to gmp-clearkey
3.05 KB, patch
eflores
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
Patch 3: Try different CLSID for H.264 MFT instantiation
2.41 KB, patch
eflores
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
Patch 4: Accept IYUV as well as I420 output from H.264 decoder MFT
1.20 KB, patch
eflores
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review 
Turns out on Win7 our clearkey plugin is only reporting DECRYPT capability, not DECRYPT_AND_DECODE.

This is because we only assume that we can decode if msauddecmft.dll is available, but it's only present on Win8 (and later I assume).
Attached patch Patch — — Splinter Review 
First takers for r+...

Make gmp-clearkey only require one of msauddecmft.dll or msmpeg2adec.dll for us to report that we can decode audio, instead of requiring both.

This means on Win7 where msauddecmft.dll doesn't exist, we will report that we can decode instead of not.
Attachment #8577574 - Flags: review?(edwin)
Attachment #8577574 - Flags: review?(ajones)
Attachment #8577574 - Flags: review?(ajones)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ada27d1eac50 for Win7 near-permaorange "test_eme_canvas_blocked.html | bipbop-cenc-videoinit.mp4-1 got error event; [object Event] - expected PASS" etc., a la https://treeherder.mozilla.org/logviewer.html#?job_id=7632921&repo=mozilla-inbound. More nearly perma on debug than opt or PGO, so probably more likely to happen the slower the browser is.
(In reply to Chris Pearce (:cpearce) from comment #0)
> This is because we only assume that we can decode if msauddecmft.dll is
> available, but it's only present on Win8 (and later I assume).

Seems like a safeish assumption. Decrypt and decode works on Windows 10.
comment 3
Flags: needinfo?(cpearce)
Priority: -- → P1
(In reply to Chris Pearce (:cpearce) from comment #8)
> Test my theory that bug 1147730 fixes this:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=256f829e63a5

My theory is proven wrong.
Looking at the logs, it appears we're stuck waiting in MediaDataDecoderProxy::Flush(), presumably for the GMP to send a ResetComplete() callback.
... because the GMP crashed in ~VideoDecoder():


01:40:57  WARNING -  PROCESS-CRASH | dom/media/test/test_eme_persistent_sessions.html | application crashed [@ VideoDecoder::~VideoDecoder()]
01:40:57     INFO -  Crash dump filename: c:\users\cltbld\appdata\local\temp\tmptcqnkz.mozrunner\minidumps\cae60805-2f71-4eb5-8407-ff2b0d2f9b0c.dmp
01:40:57     INFO -  Operating system: Windows NT
01:40:57     INFO -                    6.1.7601 Service Pack 1
01:40:57     INFO -  CPU: x86
01:40:57     INFO -       GenuineIntel family 6 model 30 stepping 5
01:40:57     INFO -       8 CPUs
01:40:57     INFO -  Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
01:40:57     INFO -  Crash address: 0x0
01:40:57     INFO -  Assertion: Unknown assertion type 0x00000000
01:40:57     INFO -  Thread 0 (crashed)
01:40:57     INFO -   0  clearkey.dll!VideoDecoder::~VideoDecoder() [VideoDecoder.cpp:256f829e63a5 : 43 + 0x9]
01:40:57     INFO -      eip = 0x6a3b41f1   esp = 0x002bf54c   ebp = 0x002bf558   ebx = 0x002bf96c
01:40:57     INFO -      esi = 0x00cd7c80   edi = 0x002bf96c   eax = 0x6a3d299c   ecx = 0x00000000
01:40:57     INFO -      edx = 0x002bf59c   efl = 0x00210202
01:40:57     INFO -      Found by: given as instruction pointer in context
01:40:57     INFO -   1  clearkey.dll!VideoDecoder::`scalar deleting destructor'(unsigned int) + 0xa
01:40:57     INFO -      eip = 0x6a3b49ca   esp = 0x002bf554   ebp = 0x002bf558
01:40:57     INFO -      Found by: call frame info
01:40:57     INFO -   2  clearkey.dll!VideoDecoder::Destroy() [AudioDecoder.cpp:256f829e63a5 : 272 + 0x9]
01:40:57     INFO -      eip = 0x6a3b5ecd   esp = 0x002bf560   ebp = 0x002bf578
01:40:57     INFO -      Found by: call frame info
01:40:57     INFO -   3  xul.dll!mozilla::gmp::Runnable::Run() [GMPPlatform.cpp:256f829e63a5 : 40 + 0x7]
01:40:57     INFO -      eip = 0x65f98ead   esp = 0x002bf568   ebp = 0x002bf578
01:40:57     INFO -      Found by: call frame info
01:40:57     INFO -   4  xul.dll!MessageLoop::RunTask(Task *) [message_loop.cc:256f829e63a5 : 361 + 0x10]
01:40:57     INFO -      eip = 0x6569a5d5   esp = 0x002bf570   ebp = 0x002bf578
01:40:57     INFO -      Found by: call frame info
Looking like the try push is going green.

I'm guessing we're calling DecodingComplete before InitDecode() has been called, so the deref of the GMPMutes in ~VideoDecoder is a null pointer deref.
Attachment #8585272 - Flags: review?(edwin)
Sigh, adding the null checks didn't fix it, we still timeout. However we no longer crash dereffing VideoDecoder::mMutex. So we could be failing to create the decoder in VideoDecoder::InitDecode() and bailing out before the mutex is created. That would explain why we're not reaching loadeddata.

To test this theory, I added a MOZ_CRASH in the path where we failed to create the video decoder:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9b1b7208aa1
(In reply to Chris Pearce (:cpearce) from comment #15)
> To test this theory, I added a MOZ_CRASH in the path where we failed to
> create the video decoder:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9b1b7208aa1

Bingo.

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/cpearce@mozilla.com-f9b1b7208aa1/try-win32/try_win7-ix_test-mochitest-3-bm109-tests1-windows-build509.txt.gz


20:33:45     INFO -  Crash reason:  EXCEPTION_BREAKPOINT
20:33:45     INFO -  Crash address: 0x63f86ab5
20:33:45     INFO -  Assertion: Unknown assertion type 0x00000000
20:33:45     INFO -  Thread 0 (crashed)
20:33:45     INFO -   0  clearkey.dll!VideoDecoder::InitDecode(GMPVideoCodec const &,unsigned char const *,unsigned int,GMPVideoDecoderCallback *,int) [VideoDecoder.cpp:f9b1b7208aa1 : 58 + 0x0]
20:33:45     INFO -      eip = 0x63f86ab5   esp = 0x0017f450   ebp = 0x0017f458   ebx = 0x0017f694
20:33:45     INFO -      esi = 0x01a1c49c   edi = 0x00ad7aa8   eax = 0x80004005   ecx = 0x62eea7dd
20:33:45     INFO -      edx = 0x626f6ee8   efl = 0x00200286
20:33:45     INFO -      Found by: given as instruction pointer in context
20:33:45     INFO -   1  xul.dll!mozilla::gmp::GMPVideoDecoderChild::RecvInitDecode(GMPVideoCodec const &,nsTArray<unsigned char> &&,int const &) [GMPVideoDecoderChild.cpp:f9b1b7208aa1 : 122 + 0x1c]
20:33:45     INFO -      eip = 0x665ecd2b   esp = 0x0017f460   ebp = 0x0017f47c
20:33:45     INFO -      Found by: call frame info
Injected more MOZ_CRASH()s to detect the specific code path that is failing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b289a3412bf
(In reply to Chris Pearce (:cpearce) from comment #17)
> Injected more MOZ_CRASH()s to detect the specific code path that is failing:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b289a3412bf

This isn't very enlightening. It seems that CreateMFT is returning a failure code, but all the paths in CreateMFT that fail have a MOZ_CRASH on them.
The test machines are Win7 Enterprise N, SP1.
I think we're calling GMPVideoDecoder::Reset without having first calling InitDecode, so the GMP can't call us back to tell us the Reset is finished. Testing that theory:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40f96e01eb85
With the same fix for audio, and not killing the plugin if Reset/Drain is called before DecodingComplete.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aec8207406e6
Priority: P1 → P2
The H.264 decoder IMFTransform on Windows Enterprise Server N supports these output types (FCC):

NV12
YV12
IYUV
YUY2

Whereas we query for I420, so WMFH264Decoder::SetDecoderOutputType() fails.
"IYUV is identical to I420" according to https://msdn.microsoft.com/en-us/library/windows/desktop/dd391027%28v=vs.85%29.aspx 

Searching for that instead of I420 works. :)
So there are two problems here. The test machines run Windows Enterprise Server N, and on that OS the H.264 MFT can only be created using CLSID {62ce7e72-4c71-4d20-00003b99450}, whereas on other Windows' we're creating using the expression __uuidof(CMSH264DecoderMFT) which is {62ce7e72-4c71-4d20-00003b9f2d0}, i.e. different. The second problem is as comment 26 and comment 27; the output format reports a different YUV format.

Note that the MP4Reader's WMF PDM is creating the IMFTransform using a different mechanism (that doesn't work inside the sandbox), so it doesn't fall victim to the CLSID being different.
If we fail to create the MFT using the existing path, try the CLSID that Win 7 Enterprise N expects.
Attachment #8591408 - Flags: review?(edwin)
The IMFTransform on Win 7 Enterprise N doesn't list that it supports I420 when you enumerate it output types, which is what we query for. But it does advertise IYUV, which is identical, so also accept that.
Attachment #8591409 - Flags: review?(edwin)
(In reply to Chris Pearce (:cpearce) from comment #32)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cb538c93150

Forgot to include patches in this push. :/
Comment on attachment 8577574 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: These patches make our ClearKey EME plugin decode video inside the plugin on Windows 7, instead of relying on Firefox to decode. These patches also make EME work in "Enterprise" versions of Windows. Without these patches we won't be testing the same code paths that Adobe's EME plugin uses to decode video on Windows 7, meaning we won't have regression test coverage to ensure we don't regress EME video decoding. Note that we already do this on Win8.
[Describe test coverage new/current, TreeHerder]: This makes all our EME mochitest use a the same code path as we use on Win8.
[Risks and why]: Could break ClearKey EME, but since that's a toy DRM that no one uses for real, it doesn't matter. I think the risk of not taking this is higher; if there is a regression or problem with decoding in EME, we want to know about it!
[String/UUID change made/needed]: None.
Attachment #8577574 - Flags: approval-mozilla-beta?
Attachment #8577574 - Flags: approval-mozilla-aurora?
Comment on attachment 8585272 [details] [diff] [review]
Patch 2: Add more nullchecks to gmp-clearkey

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: These patches make our ClearKey EME plugin decode video inside the plugin on Windows 7, instead of relying on Firefox to decode. These patches also make EME work in "Enterprise" versions of Windows. Without these patches we won't be testing the same code paths that Adobe's EME plugin uses to decode video on Windows 7, meaning we won't have regression test coverage to ensure we don't regress EME video decoding. Note that we already do this on Win8.
[Describe test coverage new/current, TreeHerder]: This makes all our EME mochitest use a the same code path as we use on Win8.
[Risks and why]: Could break ClearKey EME, but since that's a toy DRM that no one uses for real, it doesn't matter. I think the risk of not taking this is higher; if there is a regression or problem with decoding in EME, we want to know about it!
[String/UUID change made/needed]: None.
Attachment #8585272 - Flags: approval-mozilla-beta?
Attachment #8585272 - Flags: approval-mozilla-aurora?
Comment on attachment 8591408 [details] [diff] [review]
Patch 3: Try different CLSID for H.264 MFT instantiation

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: These patches make our ClearKey EME plugin decode video inside the plugin on Windows 7, instead of relying on Firefox to decode. These patches also make EME work in "Enterprise" versions of Windows. Without these patches we won't be testing the same code paths that Adobe's EME plugin uses to decode video on Windows 7, meaning we won't have regression test coverage to ensure we don't regress EME video decoding. Note that we already do this on Win8.
[Describe test coverage new/current, TreeHerder]: This makes all our EME mochitest use a the same code path as we use on Win8.
[Risks and why]: Could break ClearKey EME, but since that's a toy DRM that no one uses for real, it doesn't matter. I think the risk of not taking this is higher; if there is a regression or problem with decoding in EME, we want to know about it!
[String/UUID change made/needed]: None.
Attachment #8591408 - Flags: approval-mozilla-beta?
Attachment #8591408 - Flags: approval-mozilla-aurora?
Comment on attachment 8591409 [details] [diff] [review]
Patch 4:  Accept  IYUV as well as I420 output from H.264 decoder MFT

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: These patches make our ClearKey EME plugin decode video inside the plugin on Windows 7, instead of relying on Firefox to decode. These patches also make EME work in "Enterprise" versions of Windows. Without these patches we won't be testing the same code paths that Adobe's EME plugin uses to decode video on Windows 7, meaning we won't have regression test coverage to ensure we don't regress EME video decoding. Note that we already do this on Win8.
[Describe test coverage new/current, TreeHerder]: This makes all our EME mochitest use a the same code path as we use on Win8.
[Risks and why]: Could break ClearKey EME, but since that's a toy DRM that no one uses for real, it doesn't matter. I think the risk of not taking this is higher; if there is a regression or problem with decoding in EME, we want to know about it!
[String/UUID change made/needed]: None.
Attachment #8591409 - Flags: approval-mozilla-beta?
Attachment #8591409 - Flags: approval-mozilla-aurora?
Flags: needinfo?(cpearce)
Comment on attachment 8585272 [details] [diff] [review]
Patch 2: Add more nullchecks to gmp-clearkey

We want to help our partners. Taking it.

Should be in 38 beta 5.
Attachment #8585272 - Flags: approval-mozilla-beta?
Attachment #8585272 - Flags: approval-mozilla-beta+
Attachment #8585272 - Flags: approval-mozilla-aurora?
Attachment #8585272 - Flags: approval-mozilla-aurora+
Attachment #8591409 - Flags: approval-mozilla-beta?
Attachment #8591409 - Flags: approval-mozilla-beta+
Attachment #8591409 - Flags: approval-mozilla-aurora?
Attachment #8591409 - Flags: approval-mozilla-aurora+
Attachment #8591408 - Flags: approval-mozilla-beta?
Attachment #8591408 - Flags: approval-mozilla-beta+
Attachment #8591408 - Flags: approval-mozilla-aurora?
Attachment #8591408 - Flags: approval-mozilla-aurora+
Attachment #8577574 - Flags: approval-mozilla-beta?
Attachment #8577574 - Flags: approval-mozilla-beta+
Attachment #8577574 - Flags: approval-mozilla-aurora?
Attachment #8577574 - Flags: approval-mozilla-aurora+
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.