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

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8357a34aef2d

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

I didn't mean to ni? spohl. Dunno how that happened.

Comment 9

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 6: Use nsACString in DOMException::Create() instead of nsCString

This makes it easier to pass arbitrary nsCString types.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd1f5d67291

Comment 15

[Edwin Flores [inactive from 2016-12-01] [:eflores] [: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

Comment 16

[Edwin Flores [inactive from 2016-12-01] [:eflores] [: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.

Comment 17

[Stephen A Pohl [:spohl]]

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.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Any chance patch 5 can be uplifted to newer versions once it lands, since we have reports of it crashing Openh264 in 38.*?

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yes, I'm aiming to uplift all of this since it's required for EME.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41bf995f486f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffbb039a76bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/29445c63bb5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f514cab5623b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a861d5540616
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6af3c677479
https://hg.mozilla.org/integration/mozilla-inbound/rev/833b59427f2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/464965232c98

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/41bf995f486f
https://hg.mozilla.org/mozilla-central/rev/ffbb039a76bc
https://hg.mozilla.org/mozilla-central/rev/29445c63bb5f
https://hg.mozilla.org/mozilla-central/rev/f514cab5623b
https://hg.mozilla.org/mozilla-central/rev/a861d5540616
https://hg.mozilla.org/mozilla-central/rev/e6af3c677479
https://hg.mozilla.org/mozilla-central/rev/833b59427f2e
https://hg.mozilla.org/mozilla-central/rev/464965232c98

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Chris, which versions does this bug affect?  Up to 39?  38? Thanks.

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Aurora try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eccee467933

Comment 29

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/78ac03fd7e40
https://hg.mozilla.org/releases/mozilla-aurora/rev/adec40afb563
https://hg.mozilla.org/releases/mozilla-aurora/rev/5d77c8d4b3b5
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d1c159449ff
https://hg.mozilla.org/releases/mozilla-aurora/rev/a1fdda0d28c9
https://hg.mozilla.org/releases/mozilla-aurora/rev/f558e69a68de
https://hg.mozilla.org/releases/mozilla-aurora/rev/5149cc22d3ba

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks for landing Ryan!