[EME] Test instantiating a GMPVideoDecoder works before reporting EME keysystem available

RESOLVED FIXED in Firefox 40

Status

()

Core
Audio/Video
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed, firefox41 fixed)

Details

Attachments

(8 attachments)

(Assignee)

Description

3 years ago
Sometimes if the system has broken video codecs, we can't actually tell by looking for the DLLs on system to see if the video decoder will start up. 

This results in crashes in mozilla::gmp::GMPChild::ProcessingError() because we fail to get a GMPVideoDecoder pointer from the GMP, so GMPContentChild::RecvPGMPVideoDecoderConstructor returns false which is interpreted as an error.

So we can start up the EME GMP and request a GMPVideoDecoder and run a frame through and check we get what we expected results before we report that EME for that keySystem is available.
(Assignee)

Updated

3 years ago
Assignee: nobody → cpearce
(Assignee)

Comment 1

3 years ago
Created attachment 8612795 [details] [diff] [review]
Patch 1: Change GMP*Parent::ParentId() to a more consistent GMP*Parent::GetPluginId().

Sometimes we use ParentID() when we really mean PluginId(), and sometimes the ParentID() returns the address of the parent, and other times it returns the pluginId (a counter, incremented every time we create a GMPParent).

This patch makes it consistent.
Attachment #8612795 - Flags: review?(edwin)
(Assignee)

Comment 2

3 years ago
Created attachment 8612796 [details] [diff] [review]
Patch 2: Make GMPService's GMP crash handlers easier to register. r?

Make it easier to register a crash handler for a GMP API object consumer, by just enabling the owner of the GMP* object to associate a DOM window with the GMP's pluginId, and if the GMP corresponding to that pluginId crashes, direct the 'PluginCrashed' event towards that window.

This makes it easier for us to add a crash handler when we do a trial create in the next patch.

It also turns out that the GMP can crash before the handler is set; the crash is reported, but we don't show chrome. So we remember when GMP's crash, and if we add a handler for an already crashed GMP we'll then dispatch the 'PluginCrashed' event.
Attachment #8612796 - Flags: review?(gsquelart)
(Assignee)

Comment 3

3 years ago
Created attachment 8612797 [details] [diff] [review]
Patch 3: Trial CDM video decoder instantiation

Attempt to create the GMPVideoDecoder from the EME plugin on the first time it's requested in navigator.requestMediaKeySystemAccess(). We only do this on Windows Vista and later, since other platforms can't decode yet.

We stick a test frame through the decoder, and check that it returns what we expect. We have to run the decoder, since just getting a GMPVideoDecoderProxy from the GMP doesn't mean it hasn't crashed or failed to initialize.
Attachment #8612797 - Flags: review?(edwin)
(Assignee)

Comment 4

3 years ago
Created attachment 8612798 [details] [diff] [review]
Patch 4: Reset trial-create prefs when GMP updates

Reset the prefs set when we do a trial create of the GMP whenever we update the GMP. So if a later version starts working, we'll give it another chance to be used.
Attachment #8612798 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8612796 [details] [diff] [review]
Patch 2: Make GMPService's GMP crash handlers easier to register. r?

Review of attachment 8612796 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a couple of minor nits, and one question that I trust you'll resolve (or r?me again if it's a big change).

::: dom/media/eme/MediaKeys.cpp
@@ +8,2 @@
>  #include "GMPService.h"
>  #include "mozilla/EventDispatcher.h"

EventDispatcher.h is probably not needed anymore.

::: dom/media/gmp/GMPService.cpp
@@ +303,5 @@
> +    }
> +  } else {
> +    // Plugins don't crash twice!
> +    MOZ_ASSERT(!mUnhandledPluginCrashes.Contains(PluginCrash(aPluginId, aPluginName)));
> +  }

With this code we handle either: Add callbacks then crash, XOR crash then add callbacks.
Couldn't there be a situation where we handle some callbacks when a crash happens, but more callbacks are added a bit later?
- If yes, then you don't need a 'handled' variable in this method, you'd *always* store the crash, and you should probably rename mUnhandledPluginCrashes to mPluginCrashes.
- If no, please add a note about this assumption.

::: dom/media/gmp/GMPService.h
@@ +115,5 @@
> +    nsWeakPtr mParentWindowWeakPtr;
> +    nsWeakPtr mDocumentWeakPtr;
> +  };
> +
> +  struct PluginCrash {

Style: Move |{| brace to next line, to match GMPCrashCallback style above.
Attachment #8612796 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 7

3 years ago
(In reply to Gerald Squelart [:gerald] from comment #6)
> With this code we handle either: Add callbacks then crash, XOR crash then
> add callbacks.
> Couldn't there be a situation where we handle some callbacks when a crash
> happens, but more callbacks are added a bit later?

A yes... A plugin could be being used in multiple same-origin tabs concurrently, and crash before both crash handlers are set... With the patch as written now, we'd only display the crash report approval dialog on the first tab to register the crash handler after the crash.
(Assignee)

Comment 8

3 years ago
I didn't mean to ni? spohl. Dunno how that happened.
Flags: needinfo?(spohl.mozilla.bugs)
Comment on attachment 8612797 [details] [diff] [review]
Patch 3: Trial CDM video decoder instantiation

Review of attachment 8612797 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/eme/GMPVideoDecoderTrialCreator.cpp
@@ +21,5 @@
> +{
> +  nsCOMPtr<mozIGeckoMediaPluginService> gmps =
> +    do_GetService("@mozilla.org/gecko-media-plugin-service;1");
> +  if (!gmps) {
> +    return false;

return nullptr;

I'm surprised this compiles.

@@ +43,5 @@
> +}
> +#endif
> +
> +static const char*
> +TrailCreatePrefName(const nsAString& aKeySystem)

nit: 'Trail'

@@ +56,5 @@
> +  return "";
> +}
> +
> +/* static */
> +GMPVideoDecoderTrialCreator::TrailCreateState

nit: 'Trail'

@@ +63,5 @@
> +  switch (Preferences::GetInt(TrailCreatePrefName(aKeySystem), (int)Pending)) {
> +    case 0: return Pending;
> +    case 1: return Succeeded;
> +    case 2: return Failed;
> +    default: return Pending;

Might want to assert on the default case.

@@ +376,5 @@
> +}
> +
> +void
> +TestGMPVideoDecoder::ActorCreated(GMPVideoDecoderProxy* aGMP,
> +GMPVideoHost* aHost)

nit: indent

@@ +401,5 @@
> +}
> +
> +void
> +TestGMPVideoDecoder::InitGMPDone(GMPVideoDecoderProxy* aGMP,
> +                                                              GMPVideoHost* aHost)

nit: indent

::: dom/media/eme/GMPVideoDecoderTrialCreator.h
@@ +38,5 @@
> +
> +  ~GMPVideoDecoderTrialCreator() {}
> +
> +  // Note: Keep this in sync with GetCreateTrialState.
> +  enum TrailCreateState {

nit: Trail
Attachment #8612797 - Flags: review?(edwin) → review+
(Assignee)

Comment 10

3 years ago
Created attachment 8615089 [details] [diff] [review]
Patch 5: Make GMPParent hold a self ref while child process is alive

While writing a test for this, I discovered that when we kill the child process during the "force delete in use" gtest, we end up releasing the last ref to the GMPParent (held by GMPService), before all the child process has had a chance to shutdown, which results in us not getting the "gmp-directory-deleted" notification, and the test can hang.

This patch makes the GMPParent hold a self ref while its child process is alive, so that this doesn't happen.
Attachment #8615089 - Flags: review?(edwin)
(Assignee)

Comment 11

3 years ago
Created attachment 8615090 [details] [diff] [review]
Patch 6: Use nsACString in DOMException::Create() instead of nsCString

This makes it easier to pass arbitrary nsCString types.
Attachment #8615090 - Flags: review?(bobbyholley)
(Assignee)

Comment 12

3 years ago
Created attachment 8615091 [details] [diff] [review]
Patch 7: Make GMPVideoDecoderTrialCreator accept a "promise-like" template

This makes writing the test easier, as we can pass something other than a DOM promise to GMPVideoDecoderTrialCreator::Await..() in a gtest, and it'll work the same.

Also, update our promise rejects to pass a message, so DetailedPromise callers work.
Attachment #8615091 - Flags: review?(edwin)
(Assignee)

Comment 13

3 years ago
Created attachment 8615095 [details] [diff] [review]
Patch 8: GMPVideoDecoderTrialCreator gtest

Make gmp-fake advertise in its .info file that it supports decode-video, but fail when requested to mimic what happens when WMF fails, and then test that we get this behaviour.

We have to jump through to hoops to ensure the prefs aren't a problem here, and to ensure that gtests that expect decode-video to work get the fake-openh264 plugin.

Also ensure we call Close() on proxy objects, so that we don't leak the GMPs.
Attachment #8615095 - Flags: review?(edwin)
Comment on attachment 8615091 [details] [diff] [review]
Patch 7: Make GMPVideoDecoderTrialCreator accept a "promise-like" template

Review of attachment 8615091 [details] [diff] [review]:
-----------------------------------------------------------------

Wrong one
Attachment #8615091 - Flags: review+ → review?(edwin)
Comment on attachment 8615089 [details] [diff] [review]
Patch 5: Make GMPParent hold a self ref while child process is alive

Review of attachment 8615089 [details] [diff] [review]:
-----------------------------------------------------------------

Right one.
Attachment #8615089 - Flags: review?(edwin) → review+
Comment on attachment 8612798 [details] [diff] [review]
Patch 4: Reset trial-create prefs when GMP updates

Review of attachment 8612798 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, this fell off my radar for a while.
Attachment #8612798 - Flags: review?(spohl.mozilla.bugs) → review+
Attachment #8615090 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 18

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #14)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd1f5d67291

The test_interfaces failure in this push Went Away when I re-pulled from m-c.
Any chance patch 5 can be uplifted to newer versions once it lands, since we have reports of it crashing Openh264 in 38.*?
Flags: needinfo?(cpearce)
(Assignee)

Comment 20

3 years ago
Yes, I'm aiming to uplift all of this since it's required for EME.
Flags: needinfo?(cpearce)
Duplicate of this bug: 1164739
(Assignee)

Updated

3 years ago
Depends on: 1174987
Chris, which versions does this bug affect?  Up to 39?  38? Thanks.
(Assignee)

Updated

3 years ago
Blocks: 1176148
(Assignee)

Comment 25

3 years ago
(In reply to Liz Henry (:lizzard) from comment #24)
> Chris, which versions does this bug affect?  Up to 39?  38? Thanks.

Up to 39. I think it's a little too risky to uplift to 39, since we're so late in the cycle. Will wait for the results of the next EME trial, but if it looks like we really need it, we should uplift to 40. I will do the uplift in Bug 1176148, since the code changed here in 40 is different enough that it basically requires a rewrite some of the patches.
(Assignee)

Comment 26

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #25)
> (In reply to Liz Henry (:lizzard) from comment #24)
> > Chris, which versions does this bug affect?  Up to 39?  38? Thanks.
> 
> Up to 39. I think it's a little too risky to uplift to 39, since we're so
> late in the cycle. Will wait for the results of the next EME trial, but if
> it looks like we really need it, we should uplift to 40. I will do the
> uplift in Bug 1176148, since the code changed here in 40 is different enough
> that it basically requires a rewrite some of the patches.

Actually I'm wrong. EME e10s support landed in 40, not 41, so rebasing onto Aurora is easy, so we may as well do it in this bug.

I think we should take these patches on Aurora, but they seem too risky for beta at this stage. We also need bug 1173225 and bug 1175006 on Aurora.
Flags: needinfo?(cpearce)
(Assignee)

Comment 27

3 years ago
Comment on attachment 8612795 [details] [diff] [review]
Patch 1: Change GMP*Parent::ParentId() to a more consistent GMP*Parent::GetPluginId().

Requesting blanket approval for uplifting all remaining patches in this bug to Firefox 40/Aurora.

(Note: Patch 5 has already been uplift to fix Bug 1164245)

Approval Request Comment
[Feature/regressing bug #]: EME

[User impact if declined]:

Without this patch, I believe some Windows machines will report that Adobe EME works when it actually doesn't.

On some Windows configurations it's possible for Firefox's code that guesses whether the Adobe EME plugin will be able to use Windows' platform libraries to decode video/audio to guess wrong. So when the Adobe EME plugin tries run it will fail. What these patches do is the first time a site tries to use Adobe EME we start up an instance of the Adobe EME plugin and test that it can decode some audio and video and then shut down the plugin, before we report to JavaScript that Adobe EME works. This means we won't report that Adobe EME works unless we know for certain that it actually does work.

[Describe test coverage new/current, TreeHerder]: Patches include a gtest to test the failure path. Exiting mochitest test the success path.

[Risks and why]: Moderate; if this code is wrong, EME sites won't work. However they will be able to fall back to Silverlight, so it's not the end of the world. And with bug 1175006 we will be able to retry the detection the next time Firefox updates, so I think the risk is well contained.

However, I do think the risk is great enough that we should keep this back from Beta39, unless it's shown we need it.

[String/UUID change made/needed]: None.
Attachment #8612795 - Flags: approval-mozilla-aurora?
status-firefox40: --- → affected
Comment on attachment 8612795 [details] [diff] [review]
Patch 1: Change GMP*Parent::ParentId() to a more consistent GMP*Parent::GetPluginId().

OK, let's try that in 40.
Attachment #8612795 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 31

3 years ago
Thanks for landing Ryan!
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.