After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 491501 - Reduce usage of base_init in GstElement subclasses
Reduce usage of base_init in GstElement subclasses
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal enhancement
: 0.11.0
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 396774
Blocks:
 
 
Reported: 2007-10-29 20:08 UTC by Shixin Zeng
Modified: 2011-04-16 14:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
details_class_init.patch (2.22 KB, patch)
2008-01-07 10:52 UTC, Sebastian Dröge (slomo)
none Details | Review
details_class_init.patch (2.21 KB, patch)
2008-01-09 06:29 UTC, Sebastian Dröge (slomo)
none Details | Review
pad_templates_class_init.diff (3.98 KB, patch)
2008-01-09 06:36 UTC, Sebastian Dröge (slomo)
none Details | Review
pad_templates_class_init.diff (5.08 KB, patch)
2008-01-09 06:54 UTC, Sebastian Dröge (slomo)
none Details | Review
no_base_init.diff (12.76 KB, patch)
2008-01-09 06:56 UTC, Sebastian Dröge (slomo)
committed Details | Review
pad_templates_class_init.diff (5.20 KB, patch)
2008-01-28 14:07 UTC, Sebastian Dröge (slomo)
none Details | Review
pad_templates_class_init.diff (5.22 KB, patch)
2008-01-29 10:45 UTC, Sebastian Dröge (slomo)
committed Details | Review
gstelement-test.diff (6.12 KB, patch)
2008-01-29 11:04 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Shixin Zeng 2007-10-29 20:08:17 UTC
Please describe the problem:
The document for gst_element_class_set_details says "This function is for use in _base_init functions only." and it is indeed used in base_init functions. However, because this function set a field "details" of type "GstElementDetails" in GstElementClass which is not dynamically allocated, it should be initialized in class_init function instead of base_init() function to avoid redundant initialization.

At least it was spotted in following case:
GstPipeline is derived from GstBin which is derived from GstElement

In GstBin's base_init function:
static void
gst_bin_base_init (gpointer g_class)
{
 GstElementClass *gstelement_class = GST_ELEMENT_CLASS (g_class);

 gst_element_class_set_details (gstelement_class, &gst_bin_details);
}

in GstPipeline's base_init function:
gst_pipeline_base_init (gpointer g_class)
{
 GstElementClass *gstelement_class = GST_ELEMENT_CLASS (g_class);

 gst_element_class_set_details (gstelement_class, &gst_pipeline_details);
}

So, during registering the type GstPipeline,
gst_element_class_set_details will firstly be called via
gst_bin_base_init () and secondly via gst_pipeline_base_init().


Steps to reproduce:



Actual results:


Expected results:


Does this happen every time?


Other information:
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2007-10-30 11:52:02 UTC
I've added some debug to the gstbin.c
$ GST_DEBUG="*:2" gst-inspect >/dev/null
bin gstbin.c:331:gst_bin_base_init: bin_base_init
bin gstbin.c:385:gst_bin_class_init: bin_class_init
bin gstbin.c:331:gst_bin_base_init: bin_base_init

and I can confirm it. It also works fine when I move the
gst_element_class_set_details() to
gst_bin_class_init() and remove the
gst_bin_base_init().

I need to check if there is a reason for that beeing in _base_init() at all.
Comment 2 Sebastian Dröge (slomo) 2007-10-31 09:47:40 UTC
It's probably in base_init() because the class_init() of GstElement needs the details already... that's just a guess though :)
Comment 3 Sebastian Dröge (slomo) 2008-01-02 19:24:03 UTC
Ok, as far as I understand it the difference between class_init and base_init is the following:

class_init is only called once, that is when creating the class struct for exactly this class.
base_init is called once for the class itself and once for each subclass.

As the class struct of subclasses is created by memcpy this means that if the details are set in class_init they will be exactly the same (same memory region, etc) for every subclass too, if they're in base_init every subclass will have a copy of them.

gst_element_class_set_details() calls __gst_element_details_set() which will for every char * in GstElementDetails set a copy of the given element details. gst_element_base_class_init() OTOH will be called for every subclass of GstElement and will reset the element details of the parent class to zeroes. So currently with GstBin and GstPipeline the following happens:

GstBin base_init() sets the element details in it's class struct. Then GstPipeline's class struct is allocated, gets GstBin's memcpy'd in and all base_init's are called on it. gst_element_base_class_init will then reset the element details to zeroes, GstBin's base_init() will again set the GstBin details (well, a copy of them) and GstPipeline's base_init() overwrites them with it's own.

So the reason the details should be set in base_init() seems to be, that this way every subclass has it's own copy of them and subclasses will get the ones of their parent class if they don't set them on their own.

IMHO this behaviour should be changed so that it will be called only in class_init() and every GstElement subclass _must_ set element details and won't inherit them from their parent class if they don't set them. It makes no sense to have the same details for the parent and the subclass anyway.

So I would propose to move all the details setting to class_init but this might have to wait for 0.11 as today GstElement subclasses can expect to inherit the details from their parent class (although this doesn't make sense at all).


On another topic, the situation is similar with the pad templates, not sure yet if it makes sense to be called in base_init too (vs. copying the pad template list only in gst_element_base_class_init), any thoughts on this?
Comment 4 Sebastian Dröge (slomo) 2008-01-02 20:37:27 UTC
Ok, my proposal for 0.10 is to move gst_element_class_set_details() calls to class_init() and for 0.11 to a) move the add_pad_template() calls to class_init() and b) let gst_element_base_class_init() copy the template list and increase the refcount for all pad templates.

This would remove the need of base_init() in the most common cases in 0.11 and wouldn't cause any problems IMHO.


We can't do the pad template stuff for 0.10 as otherwise all elements will have all pad templates twice...

What do you think?
Comment 5 Sebastian Dröge (slomo) 2008-01-03 07:22:16 UTC
Bug #396774 talks about an approach of overwriting the element details in subclasses but only some fields. If we want to allow this (and overwriting in general) we could still move it from base_init to class_init: gst_element_base_class_init() will copy the element details and the subclass will change whatever it wants in class_init.

From how I understand [0] this is how it should be done anyway, the class that has the class structure field that contains something dynamic (and which has to be copied) should care about doing whatever is necessary in base_init.

http://library.gnome.org/devel/gobject/unstable/gobject-Type-Information.html#GClassInitFunc
Comment 6 Sebastian Dröge (slomo) 2008-01-03 07:31:31 UTC
Also this makes things easier for bindings that want to create their own GstElement subclasses, most languages don't have a concept of base_init and there's no intuitive (or even possible) way of calling things in base_init there.
Comment 7 David Schleef 2008-01-03 19:57:06 UTC
We could add gst_element_class_copy_pad_templates_from_parent_class() (ugh!), which would be easy to remove globally in 0.10->0.12.
Comment 8 Sebastian Dröge (slomo) 2008-01-07 10:52:03 UTC
Created attachment 102309 [details] [review]
details_class_init.patch

patch to do the copying of the element details at the central place. All elements should be converted to set them in class_init if this patch looks ok, maybe after getting bug #396774 fixed so the details setting in all elements must be cleaned up only once.

This is completely backwards compatible.


Also, additional to this it might make sense (maybe as part of bug #396774) to set element details in all base classes.
Comment 9 Sebastian Dröge (slomo) 2008-01-07 11:20:21 UTC
Although the element details are still copied this now has the advantage that there will be exactly one copy per class instead of one copy for each subclass of the class btw...
Comment 10 Stefan Sauer (gstreamer, gtkdoc dev) 2008-01-08 11:12:14 UTC
One more benefit of moving the template stuff too would be that we can use
G_DEFINE_TYPE and save one reloc per element (because not using the base_init).
Comment 11 Sebastian Dröge (slomo) 2008-01-08 11:20:31 UTC
For the pad templates we could do the following to move that to class_init now already:

- copy the pad template list in GstElement's base_init (are GstPadTemplate meant to be immutable? in that case we can simply ref them in the new list, otherwise we have to copy them too)
- change _add_pad_template() so when it encounters a pad template name that was already added it also checks whether the pad template is completely the same and if so ignore the second addition silently, otherwise complain as we currently do.

This would allow to move it now already without breaking compatibility... what do you think?
Comment 12 Sebastian Dröge (slomo) 2008-01-09 06:29:50 UTC
Created attachment 102437 [details] [review]
details_class_init.patch

of course the old patch was completely wrong... this one is correct.
Comment 13 Sebastian Dröge (slomo) 2008-01-09 06:36:42 UTC
Created attachment 102439 [details] [review]
pad_templates_class_init.diff

Patch that does the stuff for the details and pad templates in a backward compatible way.
Comment 14 Sebastian Dröge (slomo) 2008-01-09 06:54:47 UTC
Created attachment 102442 [details] [review]
pad_templates_class_init.diff

corrected patch
Comment 15 Sebastian Dröge (slomo) 2008-01-09 06:56:50 UTC
Created attachment 102443 [details] [review]
no_base_init.diff

patch to remove all occurences of base_init from core/gst and core/libs.

Does someone know why GST_DEBUG_CATEGORY_INIT was used in base_init? I see absolutely no reason...
Comment 16 Sebastian Dröge (slomo) 2008-01-09 06:58:07 UTC
The base_init's are still there at some places because of GST_BOILERPLATE btw...
Comment 17 Sebastian Dröge (slomo) 2008-01-14 11:09:26 UTC
Unfortunately this does not work with ugly/ext/mad/id3tag because this element does very bad things:

It adds a pad template with name "sink" and "src" in class_init of the base class and then sets another one with the same name in class_init of the subclass. This currently works but breaks with my patch of course because before in class_init of the subclass there was no pad template with the name (all removed in GstElement's base_init) and now there are (copied at the same place).

Can we ignore this and simply fix the broken usage in that element (docs explicitely say that _add_pad_template() must be called in base_init) or does someone see another solution?
Comment 18 Sebastian Dröge (slomo) 2008-01-14 11:26:35 UTC
Another possibility to still support id3tag's weird behaviour would be to allow _add_pad_template() to be called a second time with the same name and this would then remove the old pad template with that name and add a new one.

Opinions please (for everything in this bug) :)
Comment 19 Sebastian Dröge (slomo) 2008-01-19 16:09:46 UTC
id3mux is fixed now in CVS, still the question holds whether we should support this broken behaviour or can simply ignore it? This seems to be the only case where the patch introduces regressions, otherwise it's safe.
Comment 20 Sebastian Dröge (slomo) 2008-01-27 06:23:56 UTC
Tim believes we can ignore this broken behaviour.

Unfortunately I found another instance of it:
GstDVDDemux inherits GstMPEGDemux and GstMPEGDemux adds a "sink" pad template in class_init while GstDVDDemux adds a "sink" template in base_init.

Comment in GstMPEGDemux: /* we have our own sink pad template, but don't use it in subclasses */

Of course this worked before as explained above but was always disallowed by the docs. I don't see an easy way around this unfortunately in this element.

What about allowing subclasses to overwrite existing pad templates? Is there any reason why we generally don't want this?
Comment 21 Sebastian Dröge (slomo) 2008-01-28 14:07:29 UTC
Created attachment 103880 [details] [review]
pad_templates_class_init.diff

Allow overwriting pad templates by adding a new one with the same name. According to Wim this should be the way to go in the future.

Also this fixes again all known regressions by the previous patch ;)
Comment 22 Sebastian Dröge (slomo) 2008-01-28 15:11:58 UTC
I'm currently working on a unit test for the pad templates, including a test for adding a pad template with the same name twice for the same class in class_init.
This unfortunately creates a memory leak currently for an unknown reason.

add_pad_template() will reference the added pad template (and did this since forever) and if a pad template with the same name is already found the old one is unreferenced and the new one referenced. This shouldn't leak the first added pad IF the old behaviour was correct already.

But the pad template that is given to add_pad_template() is never unreferenced by any caller I looked at so the question is: Who called the second _unref() before?

Now, when we remove the _ref() from add_pad_template() there is still no memory leak and no double _unref() which makes things even more mysterious.

Is there any reason WHY we call _ref() in add_pad_template() and WHY no caller of add_pad_template() calls _unref() on the pad template afterwards but still no memory is leaked? :)

Also, WHY don't we call gst_object_sink() after the _ref()? Shouldn't this be done to remove the FLOATING flag?

/me is confused :)

As I understand it we should _ref() and _sink() in _add_pad_template() and this should fix all confusion but would still be nice if someone understands the current behaviour.
Comment 23 Alessandro Decina 2008-01-28 17:07:03 UTC
I didn't dig into the issue but in gstpadtemplate.c:gst_pad_template_init(176) there's the following comment:

  /* We ensure that the pad template we're creating has a sunken reference.
   * Inconsistencies in pad templates being floating or sunken has caused
   * problems in the past with leaks, etc.
   *
   * For consistency, then, we only produce them  with sunken references
   * owned by the creator of the object
   */

So now we know a) why _sink is not needed and b) that pad templates have caused problems before :)
Comment 24 Sebastian Dröge (slomo) 2008-01-29 10:27:51 UTC
Ok, that still doesn't explain why it is referenced by _add_pad_template() and whether this is done or not doesn't cause a memleak or failure :)
Comment 25 Sebastian Dröge (slomo) 2008-01-29 10:45:00 UTC
Created attachment 103946 [details] [review]
pad_templates_class_init.diff

Ok, new patch without memory leak this time. I still don't understand it but it doesn't cause any problems if we don't ref() the pad templates. Let's get this in immediately after freeze and see if we find any problems. I didn't notice any yet :)
Comment 26 Sebastian Dröge (slomo) 2008-01-29 11:04:18 UTC
Created attachment 103949 [details] [review]
gstelement-test.diff

unit test for GstElementClass that checks several possible pad template additions/replacements
Comment 27 Sebastian Dröge (slomo) 2008-01-29 14:45:15 UTC
Ok, found the reason why there is no memory leak before. The pad templates are always listed under "still reachable" by valgrind after exit as the classes are never destructed. So in my case, either unref twice when replacing or don't ref when adding... I prefer the latter as is done in the patch ;)
Comment 28 Sebastian Dröge (slomo) 2008-02-03 10:48:00 UTC
2008-02-03  Sebastian Dröge  <slomo@circular-chaos.org>

	* gst/gstelement.c: (gst_element_base_class_init),
	(gst_element_class_add_pad_template):
	* gst/gstpadtemplate.c:
	Make it possible (and recommended) to set element details and add
	pad templates in the class_init functions by copying the details/pad
	templates in GstElement's base_init.

	Also make it possible to replace existing pad templates by adding
	a new one with the same name. This was done in a hackish fashion
	in same elements before already.

	Don't reference pad templates that are added a second time. A
	new pad template has a refcount of one and is not floating anymore
	and to be owned by the element's class. Make this more explicit by
	mentioning it in the docs of gst_element_class_add_pad_template().

	These changes are backwards compatible. Fixes bug #491501.

	* tests/check/gst/gstelement.c:
	Add unit test for setting element details, adding pad templates and
	replacing them in a subclass.
Comment 29 Sebastian Dröge (slomo) 2008-02-05 14:15:09 UTC
Ok, as showed up after applying the patches there are quite a few elements that break with these changes. The causes are all ugly hacks but still we should keep compatibility for this until 0.11.

The problems that showed up are the following (copied from my mail to gstreamer-devel):

a) The "pango" issue: pango's GstTextOverlay currently has a really ugly
hack to have a pad template in a base class but not in it's subclasses.
It adds the pad template in base_init, but only if the type it is called
with is not GstClockOverlay or GstTimeOverlay. This has the very weird
effect of having a "text_src" pad template in the base class but not in
the subclasses.

I see no way of solving this in a sane way without changing the pango
plugin. IMHO this is a really ugly hack and no documented way of doing
things. Instead the plugin should've been designed to have a
GstPangoOverlay base class without the "text_src" pad template and the
subclasses GstTextOverlay (with the pad template in question) and the
other two classes without it.

If people don't disagree or there are not more cases of this hack I
would say that we can say that this is simply not supported and people
should design their elements properly ;)


b) the "ladspa part I" issue: the ladspa plugin currently creates a
subclass of GstPadTemplate to store some additional information. But
GstElement's base_init "copies" all pad templates by creating a new one
with the same properties. Unfortunately this copy is not ladspa pad
template and doesn't have the special ladspa informations.

The only way to fix this that I currently see is the following: we don't
copy the pad templates in GstElement's base_init but simply take the
same ones as there are already in the base class and increase the
refcount. This has the effect that we must consider instances of
GstPadTemplate immutable, i.e. the properties of it must not be changed
after it was added to a element class.

IMHO we already do this implicitely as
gst_element_class_add_pad_template() takes owernership of the pad
template but I don't know if this is really intended. Oppinions?
I don't know of a single case were a pad template is changed after
adding it (except the replacing which is still supported).

c) the "ladspa part II" issue: the ladspa elements did the following in
the past:

> GstPadTemplate *tmpl = gst_pad_template_new(...);
> ...
> gst_element_class_add_pad_template (klass, tmpl);
> gst_object_unref (tmpl);

In the past this caused no problems as, although the pad template
addition took the ownership of the pad template it was referenced a
second time. Now it isn't anymore and this causes the pad template to be
freed far too early. People always told me to not ever unref the pad
templates after adding it to an element and the semantics already were
like this since forever so it'd say this is a bug in the elements and
should be fixed there (and already was for ladspa in CVS).
I see no way of fixing this in core as we would leak all pad templates
that are not unreferenced after adding.


Comment 30 Sebastian Dröge (slomo) 2011-04-16 14:01:19 UTC
commit a70df76c5386aa57d12cd064a75d7fe6439474ac
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Sat Apr 16 15:28:53 2011 +0200

    element: Add test for inheriting metadata/pad templates

commit 9b90d3d9c01e9730622a85aa2a156606a0e6749b
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Sat Apr 16 15:24:01 2011 +0200

    base: Update docs to say class_init instead of base_init
    
    And remove a useless base_init in basesrc

commit 64f6a18bada3b3dd6792199e6c58875223515b32
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Sat Apr 16 15:23:46 2011 +0200

    net: Use G_DEFINE_TYPE

commit a9e69dc3be6c6bd67e064a5d6a44f779954e6d06
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Sat Apr 16 15:23:19 2011 +0200

    gst: Don't use base_init and use G_DEFINE_TYPE instead of GST_BOILERPLATE


commit aad57970deb6a6212dbd0a7c046d656d37dc9c09
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Sat Apr 16 15:03:33 2011 +0200

    element: Inherit element metadata and pad templates from parent classes
    
    This allows to add pad templates and set metadata in class_init instead of
    base_init. base_init is a concept that is not supported by almost all
    languages and copying the templates/metadata for subclasses is the more
    intuitive way of doing things.
    
    Subclasses can override pad templates of parent classes by adding a new
    template with the same now.
    
    Also gst_element_class_add_pad_template() now takes ownership of the
    pad template, which was assumed by all code before anyway.
    
    Fixes bug #491501.




commit 92d10cbb8c17ba3b03fc6e9aa541f6a1f123c664
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Sat Apr 16 15:56:55 2011 +0200

    pango: Create a new base class for all the elements
    
    This prevents the ugly hack where the text_sink pad template
    was only added for textoverlay but not for the subclasses.
    
    Also makes this work with the core change that made
    subclasses inherit the templates of their parent class.