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.

It's probably in base_init() because the class_init() of GstElement needs the details already... that's just a guess though :)

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?

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?

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

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.

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.

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.

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...

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).

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?

Created attachment 102437 [details] [review]
details_class_init.patch

of course the old patch was completely wrong... this one is correct.

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.

Created attachment 102442 [details] [review]
pad_templates_class_init.diff

corrected patch

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...

The base_init's are still there at some places because of GST_BOILERPLATE btw...

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?

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) :)

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.

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?

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 ;)

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.

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 :)

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 :)

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 :)

Created attachment 103949 [details] [review]
gstelement-test.diff

unit test for GstElementClass that checks several possible pad template additions/replacements

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 ;)

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.

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.

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.