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)
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: cpearce, Assigned: cpearce)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
2.66 KB,
patch
|
eflores
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
3.05 KB,
patch
|
eflores
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.41 KB,
patch
|
eflores
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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).
Assignee | ||
Comment 1•6 years ago
|
||
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?(edwin) → review+
Assignee | ||
Comment 2•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1f85ee4ba6
Assignee | ||
Updated•6 years ago
|
Attachment #8577574 -
Flags: review?(ajones)
Comment 3•6 years ago
|
||
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.
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=363c140efb40
Assignee | ||
Comment 7•6 years ago
|
||
Let's see that again... https://treeherder.mozilla.org/#/jobs?repo=try&revision=363c140efb40
Flags: needinfo?(cpearce)
Assignee | ||
Comment 8•6 years ago
|
||
Test my theory that bug 1147730 fixes this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=256f829e63a5
Updated•6 years ago
|
Priority: -- → P1
Assignee | ||
Comment 9•6 years ago
|
||
(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.
Assignee | ||
Comment 10•6 years ago
|
||
Looking at the logs, it appears we're stuck waiting in MediaDataDecoderProxy::Flush(), presumably for the GMP to send a ResetComplete() callback.
Assignee | ||
Comment 11•6 years ago
|
||
... 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
Assignee | ||
Comment 12•6 years ago
|
||
Adding more null-checks... https://treeherder.mozilla.org/#/jobs?repo=try&revision=00866de02e96
Assignee | ||
Comment 13•6 years ago
|
||
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)
Attachment #8585272 -
Flags: review?(edwin) → review+
Assignee | ||
Comment 14•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=933a1d13fae0
Assignee | ||
Comment 15•6 years ago
|
||
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
Assignee | ||
Comment 16•6 years ago
|
||
(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
Assignee | ||
Comment 17•6 years ago
|
||
Injected more MOZ_CRASH()s to detect the specific code path that is failing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b289a3412bf
Assignee | ||
Comment 18•6 years ago
|
||
(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.
Assignee | ||
Comment 19•6 years ago
|
||
Another crack at data gathering: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cb0460ab870
Assignee | ||
Comment 20•6 years ago
|
||
The test machines are Win7 Enterprise N, SP1.
Assignee | ||
Comment 21•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dabe72883ea2
Assignee | ||
Comment 22•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ecd13d42f6f
Assignee | ||
Comment 23•6 years ago
|
||
With patch 2 and the H264 decoder UUID hard coded. https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa2a1b8d74e1
Assignee | ||
Comment 24•6 years ago
|
||
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
Assignee | ||
Comment 25•6 years ago
|
||
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
Updated•6 years ago
|
Priority: P1 → P2
Assignee | ||
Comment 26•6 years ago
|
||
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.
Assignee | ||
Comment 27•6 years ago
|
||
"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. :)
Assignee | ||
Comment 28•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c09700211a6
Assignee | ||
Comment 29•6 years ago
|
||
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.
Assignee | ||
Comment 30•6 years ago
|
||
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)
Assignee | ||
Comment 31•6 years ago
|
||
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)
Assignee | ||
Comment 32•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cb538c93150
Attachment #8591409 -
Flags: review?(edwin) → review+
Attachment #8591408 -
Flags: review?(edwin) → review+
Assignee | ||
Comment 33•6 years ago
|
||
(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. :/
Assignee | ||
Comment 34•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3ba3fc3d8dc https://hg.mozilla.org/integration/mozilla-inbound/rev/152fba7d47e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/4ea4d4dbc367 https://hg.mozilla.org/integration/mozilla-inbound/rev/1c76e0af5ed2
https://hg.mozilla.org/mozilla-central/rev/f3ba3fc3d8dc https://hg.mozilla.org/mozilla-central/rev/152fba7d47e2 https://hg.mozilla.org/mozilla-central/rev/4ea4d4dbc367 https://hg.mozilla.org/mozilla-central/rev/1c76e0af5ed2
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 36•6 years ago
|
||
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?
Assignee | ||
Comment 37•6 years ago
|
||
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?
Assignee | ||
Comment 38•6 years ago
|
||
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?
Assignee | ||
Comment 39•6 years ago
|
||
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?
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(cpearce)
Updated•6 years ago
|
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Comment 40•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8591409 -
Flags: approval-mozilla-beta?
Attachment #8591409 -
Flags: approval-mozilla-beta+
Attachment #8591409 -
Flags: approval-mozilla-aurora?
Attachment #8591409 -
Flags: approval-mozilla-aurora+
Updated•6 years ago
|
Attachment #8591408 -
Flags: approval-mozilla-beta?
Attachment #8591408 -
Flags: approval-mozilla-beta+
Attachment #8591408 -
Flags: approval-mozilla-aurora?
Attachment #8591408 -
Flags: approval-mozilla-aurora+
Updated•6 years ago
|
Attachment #8577574 -
Flags: approval-mozilla-beta?
Attachment #8577574 -
Flags: approval-mozilla-beta+
Attachment #8577574 -
Flags: approval-mozilla-aurora?
Attachment #8577574 -
Flags: approval-mozilla-aurora+
Comment 41•6 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ab3281f684b3 https://hg.mozilla.org/releases/mozilla-aurora/rev/9bcd3da408cd https://hg.mozilla.org/releases/mozilla-aurora/rev/25f535d3d061 https://hg.mozilla.org/releases/mozilla-aurora/rev/f549efda6b17
Comment 42•6 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/f9f96ba1dbdb https://hg.mozilla.org/releases/mozilla-beta/rev/5779893b39a5 https://hg.mozilla.org/releases/mozilla-beta/rev/dfce472edd1e https://hg.mozilla.org/releases/mozilla-beta/rev/3beb9cbddb3f
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(cpearce)
You need to log in
before you can comment on or make changes to this bug.
Description
•