registry: don't recreate features on first use. Fixes #584389

The first time one calls gst_element_factory_make(), gst recreates the plugin
feature and the element factory. As a side effect we ref the class to fill
in detail we already have filled from the registry cache. This patch changes
the behaviour to just update the existing entries. The factory is now attached
to the type and set in gst_element_base_class_init().

4 files changed, 71 insertions, 28 deletions

diff --git a/gst/gstelement.c b/gst/gstelement.c
index be848b20bb..2a55d87d75 100644
--- a/gst/gstelement.c
+++ b/gst/gstelement.c
@@ -145,6 +145,9 @@ static void gst_element_restore_thyself (GstObject * parent, xmlNodePtr self);
 static GstObjectClass *parent_class = NULL;
 static guint gst_element_signals[LAST_SIGNAL] = { 0 };
 
+/* this is used in gstelementfactory.c:gst_element_register() */
+GQuark _gst_elementclass_factory = 0;
+
 GType
 gst_element_get_type (void)
 {
@@ -167,6 +170,9 @@ gst_element_get_type (void)
 
     _type = g_type_register_static (GST_TYPE_OBJECT, "GstElement",
         &element_info, G_TYPE_FLAG_ABSTRACT);
+
+    _gst_elementclass_factory =
+        g_quark_from_static_string ("GST_ELEMENTCLASS_FACTORY");
     g_once_init_leave (&gst_element_type, _type);
   }
   return gst_element_type;
@@ -252,6 +258,13 @@ gst_element_base_class_init (gpointer g_class)
    */
   memset (&element_class->details, 0, sizeof (GstElementDetails));
   element_class->padtemplates = NULL;
+
+  /* set the factory, see gst_element_register() */
+  element_class->elementfactory =
+      g_type_get_qdata (G_TYPE_FROM_CLASS (element_class),
+      _gst_elementclass_factory);
+  GST_DEBUG ("type %s : factory %p", G_OBJECT_CLASS_NAME (element_class),
+      element_class->elementfactory);
 }
 
 static void
diff --git a/gst/gstelementfactory.c b/gst/gstelementfactory.c
index be8f7655e6..123e3e1b31 100644
--- a/gst/gstelementfactory.c
+++ b/gst/gstelementfactory.c
@@ -79,6 +79,9 @@ static GstPluginFeatureClass *parent_class = NULL;
 
 /* static guint gst_element_factory_signals[LAST_SIGNAL] = { 0 }; */
 
+/* this is defined in gstelement.c */
+extern GQuark _gst_elementclass_factory;
+
 #define _do_init \
 { \
   GST_DEBUG_CATEGORY_INIT (element_factory_debug, "GST_ELEMENT_FACTORY", \
@@ -193,8 +196,7 @@ gst_element_factory_cleanup (GstElementFactory * factory)
 
   __gst_element_details_clear (&factory->details);
   if (factory->type) {
-    g_type_class_unref (g_type_class_peek (factory->type));
-    factory->type = 0;
+    factory->type = G_TYPE_INVALID;
   }
 
   for (item = factory->staticpadtemplates; item; item = item->next) {
@@ -249,6 +251,8 @@ gboolean
 gst_element_register (GstPlugin * plugin, const gchar * name, guint rank,
     GType type)
 {
+  GstPluginFeature *existing_feature;
+  GstRegistry *registry;
   GstElementFactory *factory;
   GType *interfaces;
   guint n_interfaces, i;
@@ -258,6 +262,24 @@ gst_element_register (GstPlugin * plugin, const gchar * name, guint rank,
   g_return_val_if_fail (name != NULL, FALSE);
   g_return_val_if_fail (g_type_is_a (type, GST_TYPE_ELEMENT), FALSE);
 
+  registry = gst_registry_get_default ();
+
+  /* check if feature already exists, if it exists there is no need to update it
+   * when the registry is getting updated, outdated plugins and all there
+   * feature are removed and readded.
+   */
+  existing_feature = gst_registry_lookup_feature (registry, name);
+  if (existing_feature) {
+    GST_DEBUG_OBJECT (registry, "update existing feature %p (%s)",
+        existing_feature, name);
+    factory = GST_ELEMENT_FACTORY_CAST (existing_feature);
+    factory->type = type;
+    existing_feature->loaded = TRUE;
+    g_type_set_qdata (type, _gst_elementclass_factory, factory);
+    gst_object_unref (existing_feature);
+    return TRUE;
+  }
+
   factory =
       GST_ELEMENT_FACTORY_CAST (g_object_new (GST_TYPE_ELEMENT_FACTORY, NULL));
   gst_plugin_feature_set_name (GST_PLUGIN_FEATURE_CAST (factory), name);
@@ -284,7 +306,7 @@ gst_element_register (GstPlugin * plugin, const gchar * name, guint rank,
         g_list_append (factory->staticpadtemplates, newt);
   }
   factory->numpadtemplates = klass->numpadtemplates;
-  klass->elementfactory = factory;
+  g_type_set_qdata (type, _gst_elementclass_factory, factory);
 
   /* special stuff for URI handling */
   if (g_type_is_a (type, GST_TYPE_URI_HANDLER)) {
@@ -322,8 +344,7 @@ gst_element_register (GstPlugin * plugin, const gchar * name, guint rank,
   gst_plugin_feature_set_rank (GST_PLUGIN_FEATURE_CAST (factory), rank);
   GST_PLUGIN_FEATURE_CAST (factory)->loaded = TRUE;
 
-  gst_registry_add_feature (gst_registry_get_default (),
-      GST_PLUGIN_FEATURE_CAST (factory));
+  gst_registry_add_feature (registry, GST_PLUGIN_FEATURE_CAST (factory));
 
   return TRUE;
 
@@ -389,12 +410,12 @@ gst_element_factory_create (GstElementFactory * factory, const gchar * name)
 
   /* fill in the pointer to the factory in the element class. The
    * class will not be unreffed currently. 
-   * FIXME: This isn't safe and may leak a refcount on the factory if 2 threads
-   * create the first instance of an element at the same moment */
+   * Be thread safe as there might be 2 threads creating the first instance of
+   * an element at the same moment
+   */
   oclass = GST_ELEMENT_GET_CLASS (element);
-  if (G_UNLIKELY (oclass->elementfactory == NULL))
-    oclass->elementfactory = factory;
-  else
+  if (!g_atomic_pointer_compare_and_exchange (
+          (gpointer *) & oclass->elementfactory, NULL, factory))
     gst_object_unref (factory);
 
   if (name)
@@ -407,7 +428,8 @@ gst_element_factory_create (GstElementFactory * factory, const gchar * name)
   /* ERRORS */
 load_failed:
   {
-    GST_WARNING_OBJECT (factory, "loading plugin returned NULL!");
+    GST_WARNING_OBJECT (factory,
+        "loading plugin containing feature %s returned NULL!", name);
     return NULL;
   }
 no_type:
diff --git a/gst/gstplugin.c b/gst/gstplugin.c
index f80ddac50a..a27ef699c1 100644
--- a/gst/gstplugin.c
+++ b/gst/gstplugin.c
@@ -535,6 +535,7 @@ gst_plugin_load_file (const gchar * filename, GError ** error)
   gpointer ptr;
   struct stat file_status;
   GstRegistry *registry;
+  gboolean new_plugin = TRUE;
 
   g_return_val_if_fail (filename != NULL, NULL);
 
@@ -544,11 +545,12 @@ gst_plugin_load_file (const gchar * filename, GError ** error)
   plugin = gst_registry_lookup (registry, filename);
   if (plugin) {
     if (plugin->module) {
+      /* already loaded */
       g_static_mutex_unlock (&gst_plugin_loading_mutex);
       return plugin;
     } else {
-      gst_object_unref (plugin);
-      plugin = NULL;
+      /* load plugin and update fields */
+      new_plugin = FALSE;
     }
   }
 
@@ -586,13 +588,14 @@ gst_plugin_load_file (const gchar * filename, GError ** error)
     goto return_error;
   }
 
-  plugin = g_object_new (GST_TYPE_PLUGIN, NULL);
-
+  if (new_plugin) {
+    plugin = g_object_new (GST_TYPE_PLUGIN, NULL);
+    plugin->file_mtime = file_status.st_mtime;
+    plugin->file_size = file_status.st_size;
+  }
   plugin->module = module;
   plugin->filename = g_strdup (filename);
   plugin->basename = g_path_get_basename (filename);
-  plugin->file_mtime = file_status.st_mtime;
-  plugin->file_size = file_status.st_size;
 
   ret = g_module_symbol (module, "gst_plugin_desc", &ptr);
   if (!ret) {
@@ -606,15 +609,17 @@ gst_plugin_load_file (const gchar * filename, GError ** error)
   }
   plugin->orig_desc = (GstPluginDesc *) ptr;
 
-  /* check plugin description: complain about bad values but accept them, to
-   * maintain backwards compatibility (FIXME: 0.11) */
-  CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, name, filename);
-  CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, description, filename);
-  CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, version, filename);
-  CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, license, filename);
-  CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, source, filename);
-  CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, package, filename);
-  CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, origin, filename);
+  if (new_plugin) {
+    /* check plugin description: complain about bad values but accept them, to
+     * maintain backwards compatibility (FIXME: 0.11) */
+    CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, name, filename);
+    CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, description, filename);
+    CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, version, filename);
+    CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, license, filename);
+    CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, source, filename);
+    CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, package, filename);
+    CHECK_PLUGIN_DESC_FIELD (plugin->orig_desc, origin, filename);
+  }
 
   GST_LOG ("Plugin %p for file \"%s\" prepared, calling entry function...",
       plugin, filename);
@@ -645,8 +650,10 @@ gst_plugin_load_file (const gchar * filename, GError ** error)
   _gst_plugin_fault_handler_filename = NULL;
   GST_INFO ("plugin \"%s\" loaded", plugin->filename);
 
-  gst_object_ref (plugin);
-  gst_default_registry_add_plugin (plugin);
+  if (new_plugin) {
+    gst_object_ref (plugin);
+    gst_default_registry_add_plugin (plugin);
+  }
 
   g_static_mutex_unlock (&gst_plugin_loading_mutex);
   return plugin;
diff --git a/win32/common/libgstreamer.def b/win32/common/libgstreamer.def
index ac0c3b8945..7cb014c043 100644
--- a/win32/common/libgstreamer.def
+++ b/win32/common/libgstreamer.def
@@ -41,6 +41,7 @@ EXPORTS
 	_gst_debug_nameof_funcptr
 	_gst_debug_register_funcptr
 	_gst_element_error_printf
+	_gst_elementclass_factory DATA
 	_gst_plugin_register_static
 	_gst_trace_add_entry
 	_gst_trace_on DATA