1110271 - Enable openh264 via GMP on Android

Status: RESOLVED FIXED

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

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

H264 is required for WebRTC compliance now, so we need to get this going.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use the default message pump for GMP children

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Don't crash in NotifyEvent() on Android if we have no nsAppShell

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

These patches result in being able to load openh264 in GMP on Android, but we still need to be solve downloading of the module, etc.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use the default message pump for GMP children

This version also removes a place where we call MessagePumpForUI::current() to check if we're on the main thread. Since we don't use any form of TYPE_UI now, that doesn't work. This was the only caller of that, AFAICT.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8535116 [details] [diff] [review]
Don't crash in NotifyEvent() on Android if we have no nsAppShell

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

Do you know why we don't have an app shell?

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jim Chen [:jchen] from comment #5)
> Comment on attachment 8535116 [details] [diff] [review]
> Don't crash in NotifyEvent() on Android if we have no nsAppShell
> 
> Review of attachment 8535116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you know why we don't have an app shell?

Yeah. With GMP we don't create a widget, so we don't create an app shell.

Comment 7

[Josh Aas]

Comment on attachment 8535209 [details] [diff] [review]
Use the default message pump for GMP children

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

::: toolkit/xre/nsEmbedFunctions.cpp
@@ -740,5 @@
>  
>  void
>  XRE_ShutdownChildProcess()
>  {
> -  NS_ABORT_IF_FALSE(MessageLoopForUI::current(), "Wrong thread!");

Can you replace this instead of getting rid of it?

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Josh Aas (Mozilla Corporation) from comment #7)
> Comment on attachment 8535209 [details] [diff] [review]
> Use the default message pump for GMP children
> 
> Review of attachment 8535209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/xre/nsEmbedFunctions.cpp
> @@ -740,5 @@
> >  
> >  void
> >  XRE_ShutdownChildProcess()
> >  {
> > -  NS_ABORT_IF_FALSE(MessageLoopForUI::current(), "Wrong thread!");
> 
> Can you replace this instead of getting rid of it?

Maybe? I was thinking NS_IsMainThread() won't work, but after looking at the code some, it appears that it should.

Comment 9

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

Comment on attachment 8535209 [details] [diff] [review]
Use the default message pump for GMP children

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

I'm agnostic about Josh's comment; I don't know the ramifications.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use the default message pump for GMP children r=joshmoz,jesup

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5ab080b742
https://hg.mozilla.org/integration/mozilla-inbound/rev/139e84c2fe81

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Gonna leave this open. Still need to do whatever is necessary to download the plugins.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bb5ab080b742
https://hg.mozilla.org/mozilla-central/rev/139e84c2fe81

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Make GMPInstallManager default prefs available everywhere

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Enable GMPInstallManager on Android

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

With these last two patches, openh264 will be automatically downloaded and installed if available.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Enable GMPInstallManager on Android

This version actually enables OpenH264Provider so the addon shows up in about:addons

Comment 18

[Brian R. Bondy [:bbondy]]

Comment on attachment 8537478 [details] [diff] [review]
Make GMPInstallManager default prefs available everywhere

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

This looks ok to me but we should check to make sure releng is setup for the other platforms with binaries built and available there.

Comment 19

[Brian R. Bondy [:bbondy]]

Comment on attachment 8537509 [details] [diff] [review]
Enable GMPInstallManager on Android

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

The GMPInstallManager changes look good, but I'm not a peer for mobile.  Deferring the final call to mfinkle.

::: mobile/android/chrome/content/browser.js
@@ +341,5 @@
>            Cc["@mozilla.org/login-manager;1"].getService(Ci.nsILoginManager);
>          }, Ci.nsIThread.DISPATCH_NORMAL);
>  
> +        BrowserApp.gmpInstallManager = new GMPInstallManager();
> +        BrowserApp.gmpInstallManager.simpleCheckAndInstall().then(null, () => {});

nit: It seems strange to use BrowserApp explicitly here but `this.gmpInstallManager` below. Can this change to `this.gmpInstallManager` to be consistent?

Comment 20

[Brian R. Bondy [:bbondy]]

Just pigning for needinfo to take a pass at the 2 patches I just went through in case you see anything else.

Comment 21

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8537509 [details] [diff] [review]
Enable GMPInstallManager on Android

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

As mentioned before, please coordinate with bug 1089867 which is generalizing the OpenH264Provider now (and will rename it on the way).

::: toolkit/mozapps/extensions/extensions.manifest
@@ +16,1 @@
>  category addon-provider-module PluginProvider resource://gre/modules/addons/PluginProvider.jsm

Hm, are you sure you are ok with pulling in the full PluginProvider just for this?
That seems pretty expensive, especially for mobile.

Comment 22

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8537478 [details] [diff] [review]
Make GMPInstallManager default prefs available everywhere

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

::: modules/libpref/init/all.js
@@ +4419,5 @@
>  // Enable meta-viewport support in remote APZ-enabled frames.
>  pref("dom.meta-viewport.enabled", false);
> +
> +// GMPInstallManager prefs
> +

#ifndef MOZ_B2G ?

Comment 23

[[e-mail] (:bhearsum)]

Comment on attachment 8537478 [details] [diff] [review]
Make GMPInstallManager default prefs available everywhere

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

I'm not exactly sure what I'm being asked here. bug 1112115 is on file to get us doing Android h264 builds - I'm not sure who's going to be tackling that though.

Comment 24

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8537509 [details] [diff] [review]
Enable GMPInstallManager on Android

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

::: toolkit/mozapps/extensions/test/browser/browser.ini
@@ -45,5 @@
>  [browser_newaddon.js]
>  skip-if = e10s
>  [browser_openH264.js]
> -# OpenH264Provider.jsm is not shipped on Android
> -skip-if = os == "android"

I'm pretty sure that these browser tests are not useful for Android anyway?

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8537509 [details] [diff] [review]
Enable GMPInstallManager on Android

>+        BrowserApp.gmpInstallManager = new GMPInstallManager();
>+        BrowserApp.gmpInstallManager.simpleCheckAndInstall().then(null, () => {});

Using "BrowserApp" is OK here since the method is not already bound to the BrowserApp object. If/When we convert to "fat arrows", we can change it.

>+    if (this.gmpInstallManager) {
>+      this.gmpInstallManager.uninit();
>+    }
>+

Why are you doing the uninit in the Tab.destroy method? We removed the BrowserApp.shutdown code path because it rarely ever got called. You have a patch to tweak the Fennec shutdown to be more "graceful", but does that mean a BrowserApp.shutdown code path would be call more often?

In any case, I don't think you want to gmpInstallManager.uninit() when a Tab is destroyed. Also, "this" is the Tab, not BrowserApp here.
>diff --git a/toolkit/modules/moz.build b/toolkit/modules/moz.build
>diff --git a/toolkit/mozapps/extensions/extensions.manifest b/toolkit/mozapps/extensions/extensions.manifest

>-#ifndef MOZ_WIDGET_ANDROID
> category addon-provider-module PluginProvider resource://gre/modules/addons/PluginProvider.jsm
> category addon-provider-module OpenH264Provider resource://gre/modules/addons/OpenH264Provider.jsm
> #endif
>-#endif

>diff --git a/toolkit/mozapps/extensions/internal/moz.build b/toolkit/mozapps/extensions/internal/moz.build

> EXTRA_JS_MODULES.addons += [

>+    'OpenH264Provider.jsm',
>+    'PluginProvider.jsm',

Can you verify that PluginProvider is required for GMPInstallManager?

r- for the shutdown cleanup code, but I'd like to know if PluginManager could be avoided. The new providers could affect startup time, so I'd like to be careful about adding them.

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Mark Finkle (:mfinkle) from comment #25)
> Why are you doing the uninit in the Tab.destroy method? We removed the
> BrowserApp.shutdown code path because it rarely ever got called. You have a
> patch to tweak the Fennec shutdown to be more "graceful", but does that mean
> a BrowserApp.shutdown code path would be call more often?

Oops. I meant to put that in BrowserApp.quit(), which appears to be the right thing (called by Main:Quit event handler). What thing did you guys end up removing?

> Can you verify that PluginProvider is required for GMPInstallManager?
> 
> r- for the shutdown cleanup code, but I'd like to know if PluginManager
> could be avoided. The new providers could affect startup time, so I'd like
> to be careful about adding them.

Yeah, PluginProvider is not required. I thought it was. Fixed now.

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Enable GMPInstallManager on Android

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8540415 [details] [diff] [review]
Enable GMPInstallManager on Android

Margaret removed BrowserApp.shutdown(), which used to be fired on XUL <window unload="..">

BrowserApp.quit is as good a place as any.

Let's keep an eye on Eideticker & Autophone after this lands.

Comment 29

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8537478 [details] [diff] [review]
Make GMPInstallManager default prefs available everywhere

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

just to be safe ifndef B2G

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9a92d9af40ae
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1a932cff0528

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/9a92d9af40ae
https://hg.mozilla.org/mozilla-central/rev/1a932cff0528

Comment 32

[Lawrence Mandel [:lmandel] (use needinfo)]

relnoted as "OpenH264 support added to WebRTC for Android"