After more debating, the following comes up:

_ loading a .so (which would have to load libpython and so forth) at every registry_check would mean extra memory usage for all gstreamer-based application. So we need to avoid that.

We therefore need a way to check for other files without loading extra .so . A way to do it would be to allow plugins to tell the registry that it should also look for files:
_ with a given extension (ex:.py)
_ AND/OR some extra paths

If some file size/timestamp have changed, then the whole registry gets reloaded. This *doesn't* mean the registry will look for gstreamer plugins in more paths than the way it's currently configured, but it will be used to figure out if it is 'dirty' or not.
Also, if system-wide python plugins are installed in $GST_PLUGINS_PATH/python , the registry will already be looking in that directory, since it goes into the subdirectories of $GST_PLUGINS_PATH.
Another information the plugins could also give is environnement variables to check for extra plugins path (ex : GST_PYTHON_PLUGINS_PATH).

I'll give a try at implementing that once I have the basic python plugin loader working.

Comments welcome

See also Bug #350095

*** Bug 350095 has been marked as a duplicate of this bug. ***

I've been thinking more about it.

Those extra path/extensions/env should be stored globally for the registry (along ith the plugin that registered it). Because if they are only checked when the registry checks the specific plugin, the following case is going to happen:
_ registry starts scanning plugin directory (checking standard .so extensions)
_ the scanning hits a plugin that registers an extension
_ the registry starts scanning the directory again for that extension
... whereas it could figure out from the start that it needs to scan the plugin directory with all registered extensions.

<ensonic> bilboed, I think the extra extensions will also have an extra path, when the registry comes across a new gst-plugin (that registers custom path/ext) it needs to scan anyway
<ensonic> bilboed, the custom paths need only be scanned with the custom extension registered for it
<bilboed> ensonic, yeah, was thinking about it again ... and there's no way to avoid having to scan the directories seperately/again
<ensonic> bilboed, i don'T see the 'again'
<bilboed> ensonic, if an existing plugin modifies a registered path for ex
<natyaht> bilboed: the whole point of registering a plugin path should be to mark the plugin that 'owns' that path as dirty
<bilboed> ensonic, or scanning an existing path with modified extensions
<bilboed> natyaht, the combination of extension and path, yes
<natyaht> yes, but that should just trigger the plugin being loaded in order to register all its elements
<bilboed> natyaht, of course
<natyaht> so, if a plugin is registering a new path...
<natyaht> that implies it has just been loaded
<natyaht> which implies it has just registered all the elements it knows about
<bilboed> natyaht, aaah, true
<natyaht> which means you don't need to go back and scan the directories
<ensonic> ... and then the registration info is corect anyway
<natyaht> so, I think these things should be a new type of plugin feature

basically we need a feature in order that the registry can figure that this pluging needs special treatment.

Idea1:
What about having a GstWrapperFactory that derives from GstPluginFeature.

Idea2:
What about using a GstWrapperIface (like GstUriHandler). 

Plugins like LADSPA, LibVisual, ... would register an instance or implement the Iface. In both versions the registry code can invoke a iface/feature method which determines if a rescan of the registry is needed. For the sake of simplicity we might want to rescan the whole registry.
 

Stefan, rescanning the whole registry every time is a BAD IDEA (tm) ! Just imagine everybody complaining about how slow gst-based apps start.

It would be much saner IMHO to let the registry scanner (the part that decides that a plugin has changed) also check for modifications in other files and store that information in the registry, and not some plugin code (since it would mean redundancy in most of those plugins, and also poses the problem of how those plugins are going to figure out if the subplugins have changed or not).

An interface would be the easiest way to do this, with a method where the factory can return:
* paths : These are the paths to search for the 'subplugins' the plugin uses.
* extensions : A list of extension for those 'subplugins' (most of the time it would be .so/.dll, but a python plugin loader could also use '.py').
* env variable : An environment variable which can contain some extra paths to look into.

Then the registry scanner can look in the various paths for files with the given extensions, get the size and file timestamp, and compare those values to those already stored in the registry. If it's different, reload the given plugin.

Also, if the registry scanner sees that the current env variable value is different from the previously stored one, then it also needs to force a reload.

He Edward, you misunderstood that. I mean only rebuild the registry if stuff has changed. But then rebuild the whole registry, because we haven't yet the means to unregister on plugin and re-register it.

But the iface is exactly what I was thinking of. If this is fine for python too, we should go this way \o/.

What about the naming. I could start with a first patch. To clarify I would treat path as "<path>:<path>:...", basically join path and the contents of the env-var and then process it. How should the extension list look like? a glob '*.{dll,so}' or just a list (dll,so,...). Most of the time its gonna be one extension (per platform) only anyway.

it might be difficult to make the difference between path naming and env_variable naming. For that reason I would treat them as two different entities.
The list of extensions should be a list taking into account the extension separator. Ex : ".so,.dll" or ".py". I don't see the need for extra '{','}'. The coma should be escaped of course if it's part of the extension.

please also keep in mind that new plugins may have been installed, and so the list of plugins in the registry may be out of date.  indeed, new plugins may be installed which are several levels deep of dependency.

(for example, a new libvisual plugin, which totem needs to know to present in its preferences dialog.)

at various times, a complete disk scan needs to be made.

*** Bug 442246 has been marked as a duplicate of this bug. ***

This also affects the gnomevfs plugin btw: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=426694

thomas, do you mean new plugins loaded after an application is already running ? If so, you can already reload the registry currently. If not, with the to-come patch the new libvisual plugins will appear.

Thinking some more... I don't think we really can do this on plugin features.

Take ladspa for example, on my system that plugin provides 173 plugin features...  If we are to store 173 times some paths/extensions/files/timestamps which are ALL identical it seems a bit overkill, let alone grow the registry to an insane size which will mean taking longer to scan many times (173 times in this case) the same files to make sure they haven't changed.

So we need a plugin-global way of providing this information.

Why that. There should be exactly one GstPluginWrapperGlobFeature for plugins like  beeing discussed here. They could be a bit simmilar to
<feature typename="GstTypeFindFactory">
  <name>subparse_typefind</name>
  <rank>64</rank>
  <caps>application/x-subtitle</caps>
  <extension>srt</extension>
  <extension>sub</extension>
  <extension>mpsub</extension>
  <extension>mdvd</extension>
  <extension>smi</extension>
  <extension>txt</extension>
</feature>

without caps and rank.

Stefan, Why use a GstPluginFeature derivate to just give 3 strings ? It seems overkill...
Adding a method like:
gboolean gst_plugin_set_external_dependency_info(GstPlugin *plugin, gchar* paths, gchar* extensions, gchar *env);

would fill the job perfectly fine.

Or are you worried that adding 3 entries to GstPlugin would take too much room ?

And are we agreeing that these 3 items need to be plugin global ?

A flags argument might also be useful here (e.g. for a SCAN_RECURSIVELY flag).

I also think a simple list of extensions will be enough. If we allow globbing, we'll run into problems with character sets and filename/directory path encodings and UTF-8 and so one.

Instead of a function, one could also define a new GstPluginExternalDependencyInfo structure and then use a field in the GstPluginDesc to point to that, but I guess a function is probably a little less ugly.

Oh right. Yes lets put it into the plugin struct. I definitely want it plugin global. Thats what I menat wit my last comment (that there wouldn't be 173 times the info).

So this could look like:
gst_plugin_set_external_dependency_info(plugin, "/usr/lib/ladspa:/usr/local/lib/ladspa",".so,.so2","LADSPA_PLUGIN_PATH",FLAG_NONE);

(yes *.so2 is nonsense). Dunno if it also make sense to have a flag that defines wheter the env-var that holds a path too would be prefered over the path, prepended or appened to the path.

The registry cache would need to carry a timestamp for each directory (or even each file?) in order to check for changes.

(In reply to comment #18)
> Oh right. Yes lets put it into the plugin struct. I definitely want it plugin
> global. Thats what I menat wit my last comment (that there wouldn't be 173
> times the info).

ok, was starting to get confused here :)

> 
> So this could look like:
> gst_plugin_set_external_dependency_info(plugin,
> "/usr/lib/ladspa:/usr/local/lib/ladspa",".so,.so2","LADSPA_PLUGIN_PATH",FLAG_NONE);
> 
> (yes *.so2 is nonsense). Dunno if it also make sense to have a flag that
> defines wheter the env-var that holds a path too would be prefered over the
> path, prepended or appened to the path.

  I don't really know what the flag would bring extra. Checking if (plugin->externalpath == NULL) should be enough and bring the same functionality.

> 
> The registry cache would need to carry a timestamp for each directory (or even
> each file?) in order to check for changes.
> 

For each file unfortunately. I can't see how we can get a fully working system with only recording the timestamps of only directories (it might work on unix/posix, but I doubt it'll  work on win32).


Tim : I don't think globbing would bring us anything extra, just checking the extensions (as we already do to check for plugins using .so,.dll,.dynlib,...) of the files should be enough.

> > The registry cache would need to carry a timestamp for each directory (or even
> > each file?) in order to check for changes.
> 
> For each file unfortunately. I can't see how we can get a fully working system
> with only recording the timestamps of only directories (it might work on
> unix/posix, but I doubt it'll  work on win32).

It depends on how efficient you want to be in the case where there is a change, ie. with what granularity we should detect a change.  Theoretically, you could probably get away with just storing one or two cumulative hashes of all paths/timestamps read; this is very space-efficient, but you'll only know whether something changed after scanning through all directories and stat'ing all files.  Or you could store a cumulative timestamp/filename hash for each directory - then you would know that something changed after you've scanned that particular directory and don't have to go through the others.  Or you could store things per-file, then you know something changed as soon as you hit the first new/removed/changed file.  This is all about optimising the unlikely case where something actually changed of course, which will be expensive anyway, since the plugin needs to be loaded, so I'm not sure if we really need file granularity here.

(My assumption here is that hash collisions are so unlikely that we could get away with using some sort of simple hash here)


> Tim : I don't think globbing would bring us anything extra, just checking the
> extensions (as we already do to check for plugins using .so,.dll,.dynlib,...)
> of the files should be enough.

That's what I was saying too.  I was just providing a rationale for dropping it (I didn't see one in the earlier comments).

OpenMAX bridge is affected too.

The OpenMAX case is a little different. I haven't really decided which is the best way to approach this, but probably it will stay in the current state.

Right now different OpenMAX IL implementations can be loaded at run-time, and each one of these can have different components. IMO it would be overkill to try to map all of the possible omx components, so instead what I'm doing is creating elements per standard component. These standard components are defined in the spec, examples include mp3 decdoder, h264 encoder, vorbis decoder, etc.

However, some mechanism to map GStreamer elements should be devised, for example:

gst_name=omx_mp3dec_mad
gst_class=GstOmxMp3Dec /* this can be guessed with OpenMAX IL facilities. */
omx_name=OMX.st.audio_decoder.mp3.mad
omx_library=libomxil.so.0

In this situation gst-openmax would require the registry to be updated when the previous information changes. The omx_name/library are only useful to gst-openmax, and gst_name/class are needed for GStreamer registry.

There are ways this how this could work but those are more esoteric.

Created attachment 124925 [details] [review]
Adds gst_plugin_add_external_dependency()

First attempt of implementing this. I took the hash-approach, since it seemed the easiest and most memory efficient approach. Do keep in mind that the actual implementation does not really matter at all - we can change that at any time, since that's all private anyway.

So the main question is if the API is good/sufficient/expressive enough (I'd say it's almost a bit too expressive, but then that seems preferable to something overly simple which is likely to fail to handle certain cases).

(For example, I was wondering if we need the suffix filter at all, or if specifying the dir where plugins/modules are wouldn't almost always be enough, as in the worst case we'd re-scan even if e.g. a non-module file got changed in that directory; but then we basically get this case for free anyway if we want to support the recursive flag, which I think we do).

I have not tried it yet, just read the patch and it looks really good!

Some naming things to discuss:

EXTERNAL_DEPENDENCY
is not readyy dependencies. What about "EXTERNAL_FEATURE"

GST_PLUGIN_EXTERNAL_DEPENDENCY_FLAG_PATHS_ARE_DEFAULT_ONLY
-> GST_PLUGIN_EXTERNAL_DEPENDENCY_FLAG_PATHS_AS_FALLBACK (not that good either)?

Also the G_STMT_START changes for the registry can be applied independently.
Last question, do you have a patch for e.g. libvisual plugin to test it?

Regarding the suffix, prefix. I think the extra code for it does not really hurt that much, but in reallity I guess most plugins will use something like this
ENVVAR="MY_PLUGIN_PATH"
PATH="LIBDIR/plugins"
NAMES="*.so"

> EXTERNAL_DEPENDENCY
> is not readyy dependencies. What about "EXTERNAL_FEATURE"

It is a dependency in the sense that the set of available gstreamer plugin features, or the capabilities of gstreamer plugin features, depend on the specified files/directories. This could be plugin files/directories, but also config files, for example.

I don't think the term 'external feature' really makes things much clearer, since we use 'feature' already for GstPluginFeatures, and those are usually not external to the plugin. Also, it doesn't really sound like it would cover config files to me.


> GST_PLUGIN_EXTERNAL_DEPENDENCY_FLAG_PATHS_ARE_DEFAULT_ONLY
> -> GST_PLUGIN_EXTERNAL_DEPENDENCY_FLAG_PATHS_AS_FALLBACK (not that good
> either)?

Admittedly the flag names are a bit hideous. A better name for the above must be out there somewhere  .


> Also the G_STMT_START changes for the registry can be applied independently.

Done.


> Last question, do you have a patch for e.g. libvisual plugin to test it?

Yes, I'll attach them.

Created attachment 125805 [details] [review]
example: use new API in libvisual plugin

As for the names: we could just drop the 'external' everywhere to make it all a bit shorter if people think that'd be preferable?

I am more unhappy about the word 'dependency' than 'external' as its external (non-gstreamer) files that impact the feature set of the plugin.

I agree that _FEATURE_ is ambigous. I looked at synonyms for feature:
http://thesaurus.reference.com/browse/feature
but haven't found something good.

The extension is for meta-plugins. Plugins where the offered features are dependend on external modules. The external stuff is a soft dependency - the plugin is there in any way, it just offers more of less.

What about:
GST_PLUGIN_FUNCTIONALITY_xxx
GST_PLUGIN_SOFT_DEPENDENCY_xxx
GST_PLUGIN_EXTRA_xxx
GST_PLUGIN_EXTENSION_xxx
GST_PLUGIN_EXTERNAL_xxx
GST_PLUGIN_MODULE_xxx

Anyway, if there are some +1 for GST_PLUGIN_DEPENDENCY_xxx, thats fine for me too then.

I'm happy with GST_PLUGIN_DEPENDENCY_*.

I don't think it's ambiguous - it'll be defined in the docs.

+1 to commit as-is, or with the shortened names as you like.

Created attachment 125872 [details] [review]
updated patch: adds gst_plugin_add_dependency() (ie. same as before just without the 'external' in the names)

Committed:

 2009-01-06  Tim-Philipp Müller  <tim.muller at collabora co uk>

	* docs/gst/gstreamer-sections.txt::
	* gst/gst_private.h: (GstPluginDep), (_GstPluginPrivate):
	* gst/gstplugin.c: (gst_plugin_init), (gst_plugin_finalize),
	  (gst_plugin_class_init), (gst_plugin_list_free),
	  (gst_plugin_ext_dep_get_env_vars_hash),
	  (_priv_plugin_deps_env_vars_changed),
	  (gst_plugin_ext_dep_extract_env_vars_paths),
	  (gst_plugin_ext_dep_get_hash_from_stat_entry),
	  (gst_plugin_ext_dep_direntry_matches),
	  (gst_plugin_ext_dep_scan_dir_and_match_names),
	  (gst_plugin_ext_dep_scan_path_with_filenames),
	  (gst_plugin_ext_dep_get_stat_hash),
	  (_priv_plugin_deps_files_changed), (gst_plugin_ext_dep_free),
	  (gst_plugin_ext_dep_strv_equal), (gst_plugin_ext_dep_equals),
	  (gst_plugin_add_dependency), (gst_plugin_add_dependency_simple):
	* gst/gstplugin.h: (GstPluginPrivate), (GstPluginFlags),
	  (GST_PLUGIN_DEPENDENCY_FLAG_NONE),
	  (GST_PLUGIN_DEPENDENCY_FLAG_RECURSE),
	  (GST_PLUGIN_DEPENDENCY_FLAG_PATHS_ARE_DEFAULT_ONLY),
	  (GST_PLUGIN_DEPENDENCY_FLAG_FILE_NAME_IS_SUFFIX),
	  (GstPluginDependencyFlags), (GstPluginFilter):
	* gst/gstregistry.c: (gst_registry_scan_path_level):
	* gst/gstregistrybinary.c: (gst_registry_binary_save_feature),
	  (gst_registry_binary_save_plugin_dep),
	  (gst_registry_binary_save_plugin),
	  (gst_registry_binary_load_feature),
	  (gst_registry_binary_load_plugin_dep_strv),
	  (gst_registry_binary_load_plugin_dep),
	  (gst_registry_binary_load_plugin):
	* gst/gstregistrybinary.h: (GST_MAGIC_BINARY_VERSION_STR),
	  (GstBinaryPluginElement), (_GstBinaryDep), (GstBinaryDep):
	* gst/gstregistryxml.c: (gst_registry_xml_save_plugin):
	  Add API for making a GStreamer plugin 'dependent' on external files,
	  directories or environment variables, so that GStreamer knows when
	  it needs to re-load GStreamer plugins that wrap other plugin systems.
	  Fixes bug #350477.
	  API: add gst_plugin_add_dependency()
	  API: add gst_plugin_add_dependency_simple()


 2009-01-06  Tim-Philipp Müller  <tim.muller at collabora co uk>

	* configure.ac:
	* ext/libvisual/visual.c: (plugin_init):
	  Use new core API to make registry re-scan the plugin
	  whenever visualisations are added or removed (see #350477).