Closed
Bug 1110271
Opened 10 years ago
Closed 9 years ago
Enable openh264 via GMP on Android
Categories
(Core :: WebRTC: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: snorp, Assigned: snorp)
References
Details
Attachments
(4 files, 4 obsolete files)
886 bytes,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
1.63 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
6.13 KB,
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
10.59 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
H264 is required for WebRTC compliance now, so we need to get this going.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → snorp
Attachment #8535115 -
Flags: review?(rjesup)
Attachment #8535115 -
Flags: review?(joshmoz)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8535116 -
Flags: review?(nchen)
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
(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 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+
Assignee | ||
Comment 8•10 years ago
|
||
(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•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8535209 -
Attachment is obsolete: true
Attachment #8536797 -
Flags: review+
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5ab080b742 https://hg.mozilla.org/integration/mozilla-inbound/rev/139e84c2fe81
Assignee | ||
Comment 12•10 years ago
|
||
Gonna leave this open. Still need to do whatever is necessary to download the plugins.
Whiteboard: [leave open]
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bb5ab080b742 https://hg.mozilla.org/mozilla-central/rev/139e84c2fe81
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8537478 -
Flags: review?(netzen)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8537479 -
Flags: review?(netzen)
Attachment #8537479 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 16•10 years ago
|
||
With these last two patches, openh264 will be automatically downloaded and installed if available.
Assignee | ||
Comment 17•10 years ago
|
||
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 18•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8537478 -
Flags: review?(netzen)
Comment 19•10 years ago
|
||
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)
Comment 20•10 years ago
|
||
Just pigning for needinfo to take a pass at the 2 patches I just went through in case you see anything else.
Updated•10 years ago
|
Flags: needinfo?(gfritzsche)
Comment 21•10 years ago
|
||
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•10 years ago
|
||
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 ?
Updated•10 years ago
|
Flags: needinfo?(gfritzsche)
Comment 23•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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-
Assignee | ||
Comment 26•9 years ago
|
||
(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.
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #8537509 -
Attachment is obsolete: true
Attachment #8540415 -
Flags: review?(mark.finkle)
Comment 28•9 years ago
|
||
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 29•9 years ago
|
||
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+
Assignee | ||
Comment 30•9 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a92d9af40ae remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1a932cff0528
https://hg.mozilla.org/mozilla-central/rev/9a92d9af40ae https://hg.mozilla.org/mozilla-central/rev/1a932cff0528
Assignee | ||
Updated•9 years ago
|
tracking-fennec: --- → ?
Updated•9 years ago
|
Status: NEW → RESOLVED
tracking-fennec: ? → 37+
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Updated•9 years ago
|
Target Milestone: --- → mozilla37
Comment 32•9 years ago
|
||
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.
Description
•