GNOME Bugzilla – Bug 459466
double memory usage for pluginfeature names
Last modified: 2011-12-06 23:56:01 UTC
GstPluginFeature keeps a copy of the name, instead of using the object name. Renaming the Feature is not allowed. Also GObjects can't b renamed once they are sunken (have parent).
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