Created attachment 92187 [details] [review]
reuse name from gst-object

2007-07-23  Stefan Kost  <ensonic@users.sf.net>

	* gst/gstpluginfeature.c: (gst_plugin_feature_finalize),
	(gst_plugin_feature_set_name):
	  Avoid double memory usage for pluginfeature names. Fixes #459466.

You might want to revert this until it works with registry loading.

David, can you explain a bit more what breaks?

There can be a problem if someone calls gst_object_set_name(feature); as then the feature is not locatable through the feature_hash. To avoid this we could parent the feature to the registry. gs_object_set_name would not succeed then.

Is this something that needs reverting before the release? (Haven't looked at the issue, just marking as blocker for now)

Stefan, I don't understand your description in #5. If there's no clarification/fix today, I'm going to revert this and we can re-address it after the release.

Jan, please have a look at the patch, its not big. Each plugin-feature has a name field which is the same as the GstObject::name. The patch avoids the copy, but keeps a pointer to the string in GstObject. What David pointed out is if I understand right, that the GstObject::name should be immutable. The name-ptr is also used to locate plugin features. My suggestion was to make sure the plugin-feature belong to the registry object (parentize). Then GstObject makes sure the name can't change.

The proposal would be to do
gst_object_set_parent(feature,registry) in
gstregistry.c::gst_registry_add_feature()

and 

gst_object_unparent(feature,registry) in
gstregistry.c::gst_registry_remove_feature()

As David pointed out I could also totaly get rid of the name var and replace it with GST_OBJECT_NAME(). 

Of course I looked at the patch. As you say, it's small - and seemingly straightforward, except that Dave seems to think there is a problem with it, and I can't see it.

So the current situation as I see it is:
* We have a patch applied, to which there is an objection that hasn't been clarified. 
* We only have guesses as to what Dave thinks is wrong (unless he explained things to you in some place I can't see)
* We're frozen, and I want to do releases, and this is marked as blocker bug.

Since it's such a trivial change, I'm going to revert it and release. We can re-apply immediately after figure out exactly what is wrong, if anything.

I just noticed that gstregistryxml.c does this too (which will need fixing when we put the patch back in):

read_string (reader, &feature->name, FALSE);
(line 286)

That will leak if the feature name string is not owned by the feature any more.

The problem in #11 is what I was talking about earlier.

Same issue is also in gstregistrybinary.c::gst_registry_binary_load_feature()

  /* unpack more plugin feature strings */
  unpack_string (*in, feature->name);

I'll make a new patch that fixes those issues, once there is consensus about parenting features to the registry.

Ping, any opinions on parenting features to the registry?

I like the idea, I mean those things should be consired read-only anyway, right?

Created attachment 182685 [details] [review]
reuse name from gst-object

Created attachment 182693 [details] [review]
reuse name from gst-object

Now unit tests pass also :)

We could take a similar route for GstPlugin (maybe in 0.11).

Created attachment 182700 [details] [review]
reuse name from gst-object

No more such warnings now.
GStreamer-CRITICAL **: 
Trying to dispose object "xxxx", but it still has a parent "registry0".
You need to let the parent manage the object instead of unreffing the object directly

Created attachment 186801 [details] [review]
reuse name from gst-object

updated patch, will push after freeze if no one objects

commit bd302bb63de30828fda78ba4eaf55e2447d48261
Author: Stefan Kost <ensonic@users.sf.net>
Date:   Thu Apr 28 11:34:39 2011 +0300

    pluginfeature: avoid duplicating feature->name
    
    The feature name is not supposed to change over time anyway. In order to enforce
    this parentize features to the registry and make the feature->name pointing to
    GstObject:name. In 0.11 we could consider of removing the feature->name variable
    (FIXME comment added).
    
    Fixes: #459466

commit c33b50e00fbfbd6dd21ef922320aa088fad3670f
Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
Date:   Tue Dec 6 23:52:53 2011 +0000

    indexfactory: fix memory leak
    
    Introduced by commit bd302bb6 pluginfeature: avoid duplicating feature->name
    
    https://bugzilla.gnome.org/show_bug.cgi?id=459466
    https://bugzilla.gnome.org/show_bug.cgi?id=665703