1172396 - [EME] GMPVideoDecoderTrialCreator doesn't work with e10s on

Status: RESOLVED FIXED

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

Description

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

GMPVideoDecoderTrialCreator doesn't work if e10s is enabled. Oops.

Comment 1

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

Attachment: wip - e10s-trial-creator.patch

Still needs cleanup and a few TODOs.

Comment 2

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

Attachment: 1172396-1-enable-trialcreator-e10s.patch

Comment 3

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

Attachment: 1172396-2-remove-random-nodeid.patch

Comment 4

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

Attachment: 1172396-3-startplugin-intr.patch

Comment 5

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

Attachment: 1172396-4-update-pref.patch

Comment 6

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

Attachment: 1172396-5-present-on-disk.patch

Comment 7

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

Comment on attachment 8642230 [details] [diff] [review]
1172396-4-update-pref.patch

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

::: dom/media/gmp/GMPServiceParent.cpp
@@ +1587,5 @@
> +bool
> +GMPServiceParent::RecvUpdateGMPTrialCreateState(const nsString& aKeySystem,
> +                                                const uint32_t& aState)
> +{
> +  return NS_SUCCEEDED(mService->UpdateTrialCreateState(aKeySystem, aState));

Now that I look at it again, this should probably always return true. We don't want IPDL to freak out just because UpdateTrialCreateState failed.

Comment 8

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

Comment on attachment 8642226 [details] [diff] [review]
1172396-1-enable-trialcreator-e10s.patch

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

Assuming the rest works...

Comment 9

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

Comment on attachment 8642229 [details] [diff] [review]
1172396-3-startplugin-intr.patch

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

::: dom/media/gmp/PGMP.ipdl
@@ +33,5 @@
>  
>  child:
>    async BeginAsyncShutdown();
>    async CrashPluginNow();
> +  intr StartPlugin();

I have a bad feeling about this.

Comment 10

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

Comment on attachment 8642229 [details] [diff] [review]
1172396-3-startplugin-intr.patch

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

::: dom/media/gmp/GMPParent.cpp
@@ +167,5 @@
>        return NS_ERROR_FAILURE;
>      }
>      LOGD("%s: Sent node id to child process", __FUNCTION__);
>  
> +    ok = CallStartPlugin();

Also, add a comment *why* this has to be intr instead of async.

Comment 11

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

Comment on attachment 8642231 [details] [diff] [review]
1172396-5-present-on-disk.patch

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

::: dom/media/eme/MediaKeySystemAccess.cpp
@@ +152,5 @@
> +  if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +    // We need to be able to access the filesystem, so call this in the
> +    // main process via ContentChild.
> +    ContentChild* contentChild = ContentChild::GetSingleton();
> +    NS_ENSURE_TRUE(contentChild, false);

Style guide says we should be using:

if (NS_WARN_IF(!contentChild)) {
  return false;
}

Comment 12

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

We should uplift this e10s fix to Aurora 42. Without it, Beta users with e10s won't be able to test Netflix EME. e10s is currently enabled by default in Aurora 42 and will likely be enabled opt-in for Beta 42:

https://wiki.mozilla.org/Electrolysis#Schedule

Comment 13

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

Lowering priority of bugs that don't need to block initial EME rollout.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb81c8e930d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/a305c15a4102
https://hg.mozilla.org/integration/mozilla-inbound/rev/02920865b740
https://hg.mozilla.org/integration/mozilla-inbound/rev/c34355d685cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/65a61c33fa78

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4085ce7f5459

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8294d86c241d

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eb81c8e930d6
https://hg.mozilla.org/mozilla-central/rev/a305c15a4102
https://hg.mozilla.org/mozilla-central/rev/02920865b740
https://hg.mozilla.org/mozilla-central/rev/c34355d685cc
https://hg.mozilla.org/mozilla-central/rev/65a61c33fa78
https://hg.mozilla.org/mozilla-central/rev/4085ce7f5459
https://hg.mozilla.org/mozilla-central/rev/8294d86c241d

Comment 18

[Chris Peterson [:cpeterson]]

We don't need to track or uplift for 42.