[GStreamer] Do not automatically show PackageKit codec installation notifications

• Component: Media
• Status: RESOLVED FIXED

[Michael Catanzaro]

I don't see the point of using PackageKit codec installation dialogs:

* Distros that include patent-encumbered codecs in their repos, like Debian and Ubuntu, typically install them all by default, so the dialogs do not appear and are not needed.
* Distros that do not install patent-encumbered codecs by default, like Red Hat and SUSE distros, do not include them in their repos, so the dialogs always fail to find the needed codec.

I'm not sure there's any situation where this dialog is actually helpful, unless used in combination with an alternate package source (rpmfusion for Fedora, Packman for openSUSE), but instructions on how to install the alternate package source typically also include instructions on how to install all the codecs you want.

These dialogs are a bad user experience for several reasons outlined at [1]. Please either do not show these dialogs ever, or only show the dialog if you can predict that codec installation will succeed, or make it possible to disable at compile time.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=726326

[Philippe Normand]

Sebastian, what do you think about this?

[Sebastian Dröge (slomo)]

See my comment there: https://bugzilla.gnome.org/show_bug.cgi?id=726326#c4


I think from a WebKit point of view it would be good if the codec installer is not popped up immediately... but instead a hint is given to the browser. Then the browser can show something unintrusive in the UI (like Firefox does currently). And only when the user clicks on that, the codec installer would be started... which then would need some feedback mechanism in WebKit to allow the application to notify about the user agreeing to install some codecs.

[Michael Catanzaro]

(In reply to comment #2)
> I think from a WebKit point of view it would be good if the codec installer
> is not popped up immediately... but instead a hint is given to the browser.

Absolutely; we should use a signal and let the browser handle it how it wants. For Epiphany I think we'll use an info bar (for now; probably info bars will be replaced with in-app notifications in the future) to ask the user to install the codec; that would be much better than the pop-up dialog.

The default behavior should probably be to do nothing and fail to play the video; it *could* be to run the codec installer, but the pop-up dialog is going to be replaced with GNOME Software in the near future, and it would be undesirable for a web browser to launch GNOME Software every time I visit a page with multimedia. (I don't really care what's default, though.)

[Sebastian Dröge (slomo)]

The main problem I see here is that you don't want to have Epiphany (or other apps using WebKitGTK) to depend on GStreamer directly just for this. And also you don't want to expose the fact that WebKitGTK is actually using GStreamer for media stuff.

So you would need two things here:
a) a signal that tells Epiphany that codecs or $stuff is missing (I assume you could use the same mechanism to complain about e.g. missing Flash plugin or Java or Hangouts or whatever). This signal would pass some information about what is missing to the app so it can show something to the user if it wants to, and some opaque data structure.
b) a method on WebKit somewhere that can be called with this opaque data structure to actually trigger installation of the missing things.

[Sebastian Dröge (slomo)]

And this opaque data structure would also contain enough information for WebKit to restart/reload whatever needed these codecs/plugins/$stuff once the installation was successful.

[Michael Catanzaro]

Lowering the priority of this since it's a significant amount of work to implement, and because in GNOME 3.16 we will no longer pop up the codec installation dialog, but will instead just display a notification in the notification area. It would still be nice to use an info bar in the browser instead of the notification, as we are doing with Totem. That means implementing Sebastian's suggestion in comment #4, but we won't suck if we don't do this.

[Carlos Garcia Campos]

Adding API for this requires to move the plugins installation to the UI process, so this depends on bug #144099.

[Carlos Garcia Campos]

I think we can use the permission request API for this.

[Michael Catanzaro]

(In reply to comment #8)
> I think we can use the permission request API for this.

Great idea; I hadn't considered that.

[Carlos Garcia Campos]

Created attachment 257566 [details]
Patch

This patch adds new GTK+ API without unit tests, because I don't think it's possible to test this. It depends on gst plugins installed  on the host. It's handled by the MiniBrowser, so it can be tested manually with MB.

[Carlos Garcia Campos]

Created attachment 257639 [details]
Updated patch

Fixed and added missing api docs and added a unit test, by removing plugins from the registry as suggested by Philippe (thanks!)

[Carlos Garcia Campos]

Comment on attachment 257639 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257639&action=review

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:203
> +    webkit_settings_set_enable_developer_extras(webkit_web_view_get_settings(test->m_webView), TRUE);

This is not needed, I added it just for debugging and forgot to remove it.

[Michael Catanzaro]

Comment on attachment 257639 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257639&action=review

I think there are two ways we can go with this API, for applications that want custom prompts:

1) Applications should DENY the permission request and handle the plugin installation themselves: link to GStreamer, create their own GstInstallPluginsContext, show their own confirmation request, then call gst_install_plugins_context_set_confirm_search() (note that's new is GStreamer 1.6) to disable the confirmation notification. In this case, there is one missing piece to the API: a way to trigger gst_update_registry() in the web process, and then have the web process start playing the media. I think with the API as you have it, Epiphany would need to destroy the web view and make a new one after plugin installation in order to make the media play successfully.

2) Applications should call some function to tell WebKit that the plugin installation should affirmatively be run without any further user interaction and then APPROVE the permission request. Then we don't have the problem above, and applications don't have to mess with GStreamer themselves. E.g. webkit_install_missing_media_plugins_permission_request_should_request_user_confirmation(). I think this is the way to go.

> Source/WebKit2/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp:120
> +    // Default behaviour when no decision has been made is denying the request.

OK, I know deny is the default behavior for all existing permission requests, but in this case I think Allow would be a better default: since we replaced the super annoying pop-up dialog with a desktop notification, it's no longer harmful for applications to display codec notifications by default, and indeed the notification is intended to be displayed by default for applications that don't do something else themselves. But if the application wants to do something better, like we do for Epiphany, then the app can handle the permission request itself. This is sufficiently different from a web site accessing your location or taking control of your webcam, where deny by default is obviously the right thing to do, that it makes sense to break the pattern of denying all requests by default.

I'm not opposed to adding the API and defaulting to deny if that's what you want to do, though. I want the API either way. :D

> Source/WebKit2/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp:144
> + * Gets the details about the missing plugins provided by the media backend when a media couldn't be played.

Are we trying to avoid reference to GStreamer in the API? Seems like it would be good to mention how to use details.

> Source/WebKit2/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp:146
> + * Returns: a string with the details provided by the media backend, or %NULL if the media backend didn't provide any details

(nullable)

> Source/WebKit2/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.h:62
> +webkit_install_missing_media_plugins_permission_request_get_details (WebKitInstallMissingMediaPluginsPermissionRequest *request);

Thinking about the API... could we generalize this to WebKitInstallMissingPluginsPermissionRequest? We'd only support GStreamer plugins for now, but in the future we could hook up support to tell browsers to install more things:

WEBKIT_API const gchar *
webkit_install_missing_plugins_permission_request_get_details(WebKitInstallMissingPluginsPermissionRequest *request);

WEBKIT_API WebKitMissingPluginType
webkit_install_missing_plugins_permission_request_get_type(WebKitInstallMissingPluginsPermissionRequest *request);

enum WebKitMissingPluginType {
    WEBKIT_MISSING_GSTREAMER_PLUGIN,
    WEBKIT_MISSING_FLASH_PLUGIN,
    WEBKIT_MISSING_JAVA_PLUGIN,
};

Without needing to add a new PermissionRequest class for each thing. Maybe the _get_details() function would only make sense for GStreamer codecs, though....

> Source/WebKit2/UIProcess/gstreamer/InstallMissingMediaPluginsPermissionRequest.cpp:36
> +InstallMissingMediaPluginsPermissionRequest::InstallMissingMediaPluginsPermissionRequest(WebPageProxy& page,  const String& details)

Extra space after the comma

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:307
> +    } else if (!g_strcmp0(methodName, "RemoveAVPluginsFromGSTRegistry")) {

Ha, nice test!

[Michael Catanzaro]

(In reply to comment #13)
> Thinking about the API... could we generalize this to
> WebKitInstallMissingPluginsPermissionRequest? We'd only support GStreamer
> plugins for now, but in the future we could hook up support to tell browsers
> to install more things:

Hm, I wrote this considering approach (1) above, before I realized approach (2) would be possible. I'm not sure this makes sense in combination with approach (2), which I think is better, since WebKit would need to use GStreamer for plugin installation, but PackageKit for Flash/Java, which it doesn't currently know about. In approach (1) that would be up to the application, so having the same WebKit API for them all makes more sense.

[Michael Catanzaro]

(In reply to comment #13) 
> 2) Applications should call some function to tell WebKit that the plugin
> installation should affirmatively be run without any further user
> interaction and then APPROVE the permission request. Then we don't have the
> problem above, and applications don't have to mess with GStreamer
> themselves. E.g.
> webkit_install_missing_media_plugins_permission_request_should_request_user_c
> onfirmation(). I think this is the way to go.

But maybe that can't use the permission request API, because it would need to be possible to handle it asynchronously.

[Carlos Garcia Campos]

(In reply to comment #13)
> Comment on attachment 257639 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257639&action=review
> 
> I think there are two ways we can go with this API, for applications that
> want custom prompts:
> 
> 1) Applications should DENY the permission request and handle the plugin
> installation themselves: link to GStreamer, create their own
> GstInstallPluginsContext, show their own confirmation request, then call
> gst_install_plugins_context_set_confirm_search() (note that's new is
> GStreamer 1.6) to disable the confirmation notification. In this case, there
> is one missing piece to the API: a way to trigger gst_update_registry() in
> the web process, and then have the web process start playing the media. I
> think with the API as you have it, Epiphany would need to destroy the web
> view and make a new one after plugin installation in order to make the media
> play successfully.
> 
> 2) Applications should call some function to tell WebKit that the plugin
> installation should affirmatively be run without any further user
> interaction and then APPROVE the permission request. Then we don't have the
> problem above, and applications don't have to mess with GStreamer
> themselves. E.g.
> webkit_install_missing_media_plugins_permission_request_should_request_user_c
> onfirmation(). I think this is the way to go.

I think you are mixing two different (but related) things here. One thing is the permission request API to avoid launching the installer automatically, and another one is allowing applications to handle the plugin installation themselves. Those would be different APIs. This patch is only for the first thing. For the second thing we could add a signal to the web view (install-missing-media-plugin) that will be emitted after the permission request and only when the user allowed the request. If the signal is not handled (returns FALSE) then we launch the installer as we currently do, otherwise we don't do anything. The signal would have a request object with a method to be called by the application to complete the request, that will send the message back to the web process to reset the pipeline. That's how our API works in other cases, see the notifications API for example.

> > Source/WebKit2/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp:120
> > +    // Default behaviour when no decision has been made is denying the request.
> 
> OK, I know deny is the default behavior for all existing permission
> requests, but in this case I think Allow would be a better default: since we
> replaced the super annoying pop-up dialog with a desktop notification, it's
> no longer harmful for applications to display codec notifications by
> default, and indeed the notification is intended to be displayed by default
> for applications that don't do something else themselves. But if the
> application wants to do something better, like we do for Epiphany, then the
> app can handle the permission request itself. This is sufficiently different
> from a web site accessing your location or taking control of your webcam,
> where deny by default is obviously the right thing to do, that it makes
> sense to break the pattern of denying all requests by default.
> 
> I'm not opposed to adding the API and defaulting to deny if that's what you
> want to do, though. I want the API either way. :D

No, I think it makes sense to default to allow in this particular case. I could keep the deny as default when the page client returns false, and allow in the GTK+ API.

> > Source/WebKit2/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp:144
> > + * Gets the details about the missing plugins provided by the media backend when a media couldn't be played.
> 
> Are we trying to avoid reference to GStreamer in the API? Seems like it
> would be good to mention how to use details.

Yes, I tried to avoid using GStreame, using media backend instead. I don't even know how to explain what the details are :-P

> > Source/WebKit2/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp:146
> > + * Returns: a string with the details provided by the media backend, or %NULL if the media backend didn't provide any details
> 
> (nullable)

Ok, allow-none until we can bump the g-i requirements

> > Source/WebKit2/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.h:62
> > +webkit_install_missing_media_plugins_permission_request_get_details (WebKitInstallMissingMediaPluginsPermissionRequest *request);
> 
> Thinking about the API... could we generalize this to
> WebKitInstallMissingPluginsPermissionRequest? We'd only support GStreamer
> plugins for now, but in the future we could hook up support to tell browsers
> to install more things:
> 
> WEBKIT_API const gchar *
> webkit_install_missing_plugins_permission_request_get_details(WebKitInstallMi
> ssingPluginsPermissionRequest *request);
> 
> WEBKIT_API WebKitMissingPluginType
> webkit_install_missing_plugins_permission_request_get_type(WebKitInstallMissi
> ngPluginsPermissionRequest *request);
> 
> enum WebKitMissingPluginType {
>     WEBKIT_MISSING_GSTREAMER_PLUGIN,
>     WEBKIT_MISSING_FLASH_PLUGIN,
>     WEBKIT_MISSING_JAVA_PLUGIN,
> };
> 
> Without needing to add a new PermissionRequest class for each thing. Maybe
> the _get_details() function would only make sense for GStreamer codecs,
> though....

Those other things are not even handled by WebKit, and I don't think we want to handle them in Web-Kit either. Once we have a better plugins API, the application could have enough information about the missing plugin not loaded to run package config itself when the missing plugin button is clicked, for example. And we don't need any permission request, since it will be handled by the application in response to a user action. Not having flash or java installed is indeed a feature, so I won't make things easier for the user to install them :-D

> > Source/WebKit2/UIProcess/gstreamer/InstallMissingMediaPluginsPermissionRequest.cpp:36
> > +InstallMissingMediaPluginsPermissionRequest::InstallMissingMediaPluginsPermissionRequest(WebPageProxy& page,  const String& details)
> 
> Extra space after the comma

oops, good catch.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:307
> > +    } else if (!g_strcmp0(methodName, "RemoveAVPluginsFromGSTRegistry")) {
> 
> Ha, nice test!

Thanks, phil told me how to do it.

[Michael Catanzaro]

OK, I agree!

[Carlos Garcia Campos]

(In reply to comment #16)
> (In reply to comment #13)
> > Comment on attachment 257639 [details]
> > Updated patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=257639&action=review
> > 
> > I think there are two ways we can go with this API, for applications that
> > want custom prompts:
> > 
> > 1) Applications should DENY the permission request and handle the plugin
> > installation themselves: link to GStreamer, create their own
> > GstInstallPluginsContext, show their own confirmation request, then call
> > gst_install_plugins_context_set_confirm_search() (note that's new is
> > GStreamer 1.6) to disable the confirmation notification. In this case, there
> > is one missing piece to the API: a way to trigger gst_update_registry() in
> > the web process, and then have the web process start playing the media. I
> > think with the API as you have it, Epiphany would need to destroy the web
> > view and make a new one after plugin installation in order to make the media
> > play successfully.
> > 
> > 2) Applications should call some function to tell WebKit that the plugin
> > installation should affirmatively be run without any further user
> > interaction and then APPROVE the permission request. Then we don't have the
> > problem above, and applications don't have to mess with GStreamer
> > themselves. E.g.
> > webkit_install_missing_media_plugins_permission_request_should_request_user_c
> > onfirmation(). I think this is the way to go.
> 
> I think you are mixing two different (but related) things here. One thing is
> the permission request API to avoid launching the installer automatically,
> and another one is allowing applications to handle the plugin installation
> themselves. Those would be different APIs. This patch is only for the first
> thing. For the second thing we could add a signal to the web view
> (install-missing-media-plugin) that will be emitted after the permission
> request and only when the user allowed the request. If the signal is not
> handled (returns FALSE) then we launch the installer as we currently do,
> otherwise we don't do anything. The signal would have a request object with
> a method to be called by the application to complete the request, that will
> send the message back to the web process to reset the pipeline. That's how
> our API works in other cases, see the notifications API for example.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp:120
> > > +    // Default behaviour when no decision has been made is denying the request.
> > 
> > OK, I know deny is the default behavior for all existing permission
> > requests, but in this case I think Allow would be a better default: since we
> > replaced the super annoying pop-up dialog with a desktop notification, it's
> > no longer harmful for applications to display codec notifications by
> > default, and indeed the notification is intended to be displayed by default
> > for applications that don't do something else themselves. But if the
> > application wants to do something better, like we do for Epiphany, then the
> > app can handle the permission request itself. This is sufficiently different
> > from a web site accessing your location or taking control of your webcam,
> > where deny by default is obviously the right thing to do, that it makes
> > sense to break the pattern of denying all requests by default.
> > 
> > I'm not opposed to adding the API and defaulting to deny if that's what you
> > want to do, though. I want the API either way. :D
> 
> No, I think it makes sense to default to allow in this particular case. I
> could keep the deny as default when the page client returns false, and allow
> in the GTK+ API.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp:144
> > > + * Gets the details about the missing plugins provided by the media backend when a media couldn't be played.
> > 
> > Are we trying to avoid reference to GStreamer in the API? Seems like it
> > would be good to mention how to use details.
> 
> Yes, I tried to avoid using GStreame, using media backend instead. I don't
> even know how to explain what the details are :-P

OK, I think we shouldn't even have this API here. We should only return the description, as returned by gst_missing_plugin_message_get_description(), and the details string should be part of the API to actually implement your own installer (the install-missing-media-plugin signal).

[Carlos Garcia Campos]

Created attachment 258057 [details]
Updated patch

Changed the default to allow for backwards compatibility as suggested by Michael, and replaced get_details by get_description in the API.

[Michael Catanzaro]

The documentation for WebKitWebView::permission-request will also need updated, since it says "[b]y default, if the signal is not handled, webkit_permission_request_deny() will be called over the WebKitPermissionRequest" which is no longer always true.

[Carlos Garcia Campos]

(In reply to comment #20)
> The documentation for WebKitWebView::permission-request will also need
> updated, since it says "[b]y default, if the signal is not handled,
> webkit_permission_request_deny() will be called over the
> WebKitPermissionRequest" which is no longer always true.

Good catch!

[Carlos Garcia Campos]

Created attachment 258159 [details]
Updated patch

Updated the documentation about the default behaviour when permission requests are not handled.

[Carlos Garcia Campos]

Committed r188121: <http://trac.webkit.org/changeset/188121>