Enable openh264 via GMP on Android

RESOLVED FIXED in mozilla37

Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

unspecified
mozilla37
All
Android
Points:
---

Firefox Tracking Flags

(relnote-firefox 37+, fennec37+)

Details

Attachments

(4 attachments, 4 obsolete attachments)

H264 is required for WebRTC compliance now, so we need to get this going.
Created attachment 8535115 [details] [diff] [review]
Use the default message pump for GMP children
Assignee: nobody → snorp
Attachment #8535115 - Flags: review?(rjesup)
Attachment #8535115 - Flags: review?(joshmoz)
Created attachment 8535116 [details] [diff] [review]
Don't crash in NotifyEvent() on Android if we have no nsAppShell
Attachment #8535116 - Flags: review?(nchen)
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.
Created attachment 8535209 [details] [diff] [review]
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.
Attachment #8535115 - Attachment is obsolete: true
Attachment #8535115 - Flags: review?(rjesup)
Attachment #8535115 - Flags: review?(joshmoz)
Attachment #8535209 - Flags: review?(rjesup)
Attachment #8535209 - Flags: review?(joshmoz)
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?
Attachment #8535116 - Flags: review?(nchen) → review+
(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

3 years ago
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?
Attachment #8535209 - Flags: review?(joshmoz) → review+
(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 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.
Attachment #8535209 - Flags: review?(rjesup) → review+
Created attachment 8536797 [details] [diff] [review]
Use the default message pump for GMP children r=joshmoz,jesup
Attachment #8535209 - Attachment is obsolete: true
Attachment #8536797 - Flags: review+
Gonna leave this open. Still need to do whatever is necessary to download the plugins.
Whiteboard: [leave open]
Created attachment 8537478 [details] [diff] [review]
Make GMPInstallManager default prefs available everywhere
Attachment #8537478 - Flags: review?(netzen)
Created attachment 8537479 [details] [diff] [review]
Enable GMPInstallManager on Android
Attachment #8537479 - Flags: review?(netzen)
Attachment #8537479 - Flags: review?(mark.finkle)
With these last two patches, openh264 will be automatically downloaded and installed if available.
Created attachment 8537509 [details] [diff] [review]
Enable GMPInstallManager on Android

This version actually enables OpenH264Provider so the addon shows up in about:addons
Attachment #8537479 - Attachment is obsolete: true
Attachment #8537479 - Flags: review?(netzen)
Attachment #8537479 - Flags: review?(mark.finkle)
Attachment #8537509 - Flags: review?(netzen)
Attachment #8537509 - Flags: review?(mark.finkle)
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.
Attachment #8537478 - Flags: review?(bhearsum)
Attachment #8537478 - Flags: review?(netzen)
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?
Attachment #8537509 - Flags: review?(netzen)
Just pigning for needinfo to take a pass at the 2 patches I just went through in case you see anything else.
Flags: needinfo?(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 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 ?
Flags: needinfo?(gfritzsche)
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 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 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.
Attachment #8537509 - Flags: review?(mark.finkle) → review-
(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.
Created attachment 8540415 [details] [diff] [review]
Enable GMPInstallManager on Android
Attachment #8537509 - Attachment is obsolete: true
Attachment #8540415 - Flags: review?(mark.finkle)
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.
Attachment #8540415 - Flags: review?(mark.finkle) → review+
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
Attachment #8537478 - Flags: review?(bhearsum) → review+

Updated

3 years ago
Depends on: 1118747
tracking-fennec: --- → ?
Status: NEW → RESOLVED
tracking-fennec: ? → 37+
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla37
relnoted as "OpenH264 support added to WebRTC for Android"
relnote-firefox: --- → 37+
You need to log in before you can comment on or make changes to this bug.