Created attachment 81366 [details] [review]
patch adding plugin install API and unit tests

Patch adds gst_install_plugins_*() API and some basic unit tests.

Shame we can't keep the old naming (or can we?) - gst_gimme_codec or / gst_gimme_plugin is much snazzier IMHO.

Created attachment 81433 [details] [review]
updated patch

New patch, addressing some of the issues raised on IRC:

The main issue is the XID argument to gst_plugin_install_{sync|async}. We should replace this with something that's not platform-specific. or get rid of it completely. IMHO the latter is not a good idea, because it's something distros seem to want (to make it transient to the application window while the plugin installation is in progress) and they'll do it one way or another, so let's just make it part of the API instead of forcing them to patch stuff all over the place to make this work.

So instead of an xid we now pass a GstPluginInstallContext argument, and apps can set their XID on the context before passing it in. This allows us to add variants that make sense on win32/OSX/BeOS/whatever later. Also, it allows us to easily add other useful things later on.

Also improved the docs a bit:
 - mention that gst_install_plugins_sync() should usually not be used
 - some additinos to gst_install_plugins_async()
 - for both mention where the installer detail strings come from

Also added a function:
 - gst_install_plugins_installation_in_progress()

(In reply to comment #2)
> Created an attachment (id=81433) [edit]
> updated patch
> 
> New patch, addressing some of the issues raised on IRC:
> 
> The main issue is the XID argument to gst_plugin_install_{sync|async}. We
> should replace this with something that's not platform-specific. or get rid of
> it completely. IMHO the latter is not a good idea, because it's something
> distros seem to want (to make it transient to the application window while the
> plugin installation is in progress) and they'll do it one way or another, so
> let's just make it part of the API instead of forcing them to patch stuff all
> over the place to make this work.

"
GdkNativeWindow
Used to represent native windows (Windows for the X11 backend, HWNDs for Win32). 
"

Can't use Gdk in GStreamer?

> "GdkNativeWindow: Used to represent native windows (Windows for the
> X11 backend, HWNDs for Win32)."
>
> Can't use Gdk in GStreamer?

In GStreamer as such yes, but not really in this utility lib. Seems a bit heavy-handed to link to or header-wise depend on Gdk plus the entire dependency chain just to get something that basically amounts to a pointer typedef (?). Also, I am not sure it actually helps us get the information we need (something to pass on to another process so it knows which window to make itself transient to).

IMHO it's best (easiest) to punt the implementation details for OSX/WIN32 for now and know it can easily be added in future if/when the need arises.

As an alternative to the context stuff we could just turn _install_*sync() into a vararg function with documented key/type/value triplets, but that's not very bindings friendly as far as I am aware.

Created attachment 81622 [details] [review]
updated patch, take #3

Updated again. Changes to previous version:

 - add --with-install-plugins-helper option to configure so
   distros can easily set the install helper to use (defaults
   to something based on prefix otherwise - which makes sense
   since a helper in a non-prefix would typically install
   plugins in that other prefix and not in ours).

 - some small docs fixes/additions; add bits to generate API docs.

 - attach diffs for each file only once and not twice

Committed, with a few more small changes (add 'Since' to docs; document return enums better, shuffle a few of the return enums around a bit):

2007-02-02  Tim-Philipp Müller  <tim at centricular dot net>

	* gst-libs/gst/utils/Makefile.am:
	* gst-libs/gst/utils/base-utils.h:
	* gst-libs/gst/utils/install-plugins.c:
	(gst_install_plugins_context_set_xid),
	(gst_install_plugins_context_new),
	(gst_install_plugins_context_free),
	(gst_install_plugins_get_helper),
	(gst_install_plugins_spawn_child),
	(gst_install_plugins_return_from_status),
	(gst_install_plugins_installer_exited),
	(gst_install_plugins_async), (gst_install_plugins_sync),
	(gst_install_plugins_return_get_name),
	(gst_install_plugins_installation_in_progress):
	* gst-libs/gst/utils/install-plugins.h:
	  API: add API for applications to initiate installation of missing
	  plugins, ie. gst_install_plugins_async() primarily.
	  Based on libgimme-codec by Ryan Lortie.

	* configure.ac:
	  Add --with-install-plugins-helper configure option so distros can specify
	  the path of the helper script or program to call when plugin installation
	  is requested (distros: please do any argument munging in this helper
	  script instead of patching GStreamer to pass arguments differently
	  to another program directly).

	* docs/libs/gst-plugins-base-libs-docs.sgml:
	* docs/libs/gst-plugins-base-libs-sections.txt:
	  Build and document new API.

	* tests/check/libs/utils.c: (result_cb),
	(test_base_utils_install_plugins_do_callout), (GST_START_TEST),
	(libgstbaseutils_suite):
	  Some simple checks for the new API.