1075182 - [EME] Make ClearKey GMP work outside of mochitests

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

We need our regular ClearKey GMP to work in normal web content (when EME is enabled) rather than only in our mochitests.

Comment 1

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

Attachment: clearkey-package.patch

First go.

Factored out most of OpenH264Provider into GMPProvider and generalised it slightly.

Still need to generalise GMPWrapper.findUpdates() and GMPInstallManager to make updates work with other GMPs, but outside scope of this bug.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8516351 [details] [diff] [review]
clearkey-package.patch

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

I'm not sure if introducing a new provider jsm for every GMP we add scales so well, especially with the minimal differences here that are mostly just different data.

Lets wait what Blair says, but i'd rather see one GMPProvider.jsm that is parameterized from a collection of GM plugins data instead of registering/instantiating/... a new provider for every new plugin.

::: toolkit/mozapps/extensions/internal/moz.build
@@ +21,5 @@
>  # Don't ship unused providers on Android
>  if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android':
>      EXTRA_JS_MODULES.addons += [
>          'OpenH264Provider.jsm',
>          'PluginProvider.jsm',

You should add ClearKeyProvider.jsm here.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8516351 [details] [diff] [review]
clearkey-package.patch

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

What Georg said. Providers should be considered heavyweight - each provider should relate to a reasonably wide category of the type of add-on it provides. Consider XPIProvider, for instance, which handles extensions, dictionaries, locales, etc - as there's only subtle differences between them. There isn't much implementation overlap between the Addon objects for OpenH264 and ClearKey, but they overlap logically. And the Addon object for ClearKey only needs to be a very simple object that just provides basic info.

However... I question why we're adding this to the Add-on Manager at all. It's a built-in component, no update facility, no useful info to show, and I don't see any use-case for someone disabling/enabling it separately from disabling/enabling EME as a whole. It feels like it just adds noise to the UI without any benefit.

Comment 4

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

Attachment: clearkey-package-easy.patch

Just add another |addPluginDirectory| call.

Will defer doing it properly for other GMPs to whenever we need it.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8517903 [details] [diff] [review]
clearkey-package-easy.patch

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

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +281,5 @@
>      }
>  
> +    gmpService.addPluginDirectory(OS.Path.join(OS.Constants.Path.libDir,
> +                                               CLEARKEY_PLUGIN_ID,
> +                                               CLEARKEY_VERSION));

Is the ClearKey GMP really bundled for all builds the addon manager is available in?
I think you want to at least catch exceptions from this call.

Comment 6

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

Attachment: clearkey-package-easy.patch

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> Is the ClearKey GMP really bundled for all builds the addon manager is
> available in?

All desktop builds, where MOZ_EME is enabled (which it is by default).

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8518575 [details] [diff] [review]
clearkey-package-easy.patch

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

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +286,5 @@
> +        gmpService.addPluginDirectory(OS.Path.join(OS.Constants.Path.libDir,
> +                                                   CLEARKEY_PLUGIN_ID,
> +                                                   CLEARKEY_VERSION));
> +      } catch (e) {
> +        this._log.warn("startup() - adding clearkey CDM failed with exception " + e.name);

Note that Log.jsm has support for neat formatting of errors, exceptions and more:
  this._log.warn("startup() - adding clearkey CDM failed", e);

Comment 8

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

Can this land?

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/306eb2d002ac

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/306eb2d002ac
https://hg.mozilla.org/mozilla-central/rev/c6fa46d62293

Comment 11

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

Mass update firefox-status to track EME uplift.