Closed Bug 1172396 Opened 5 years ago Closed 4 years ago

[EME] GMPVideoDecoderTrialCreator doesn't work with e10s on

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
e10s + ---
firefox41 --- unaffected
firefox42 --- wontfix
firefox43 --- fixed

People

(Reporter: cpearce, Assigned: eflores)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

GMPVideoDecoderTrialCreator doesn't work if e10s is enabled. Oops.
Assignee: cpearce → edwin
Depends on: 1174987
Attached patch wip - e10s-trial-creator.patch (obsolete) — Splinter Review
Still needs cleanup and a few TODOs.
Attachment #8635226 - Attachment is obsolete: true
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.
Attachment #8642227 - Flags: review?(cpearce) → review+
Comment on attachment 8642226 [details] [diff] [review]
1172396-1-enable-trialcreator-e10s.patch

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

Assuming the rest works...
Attachment #8642226 - Flags: review?(cpearce) → review+
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.
Attachment #8642229 - Flags: review?(cpearce) → review+
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.
Attachment #8642230 - Flags: review?(cpearce) → review+
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;
}
Attachment #8642231 - Flags: review?(cpearce) → review+
[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
Lowering priority of bugs that don't need to block initial EME rollout.
Priority: P1 → P2
We don't need to track or uplift for 42.
Component: Audio/Video → Audio/Video: Playback
You need to log in before you can comment on or make changes to this bug.