[gst-devel] GstPadTemplate and element details changes in CVS

[Stefan Kost]

Hi,

yes, lets definitely fix the warnings. If only to make it easier to  
make the changes for 0.11. It would be also nice to have at least a  
lightwight change that could warn, so that those plgins we don't know  
about can be fixed.

I really like the changes and there is many benefits mentioned in bugzilla.
About the problems:
c) is a ref-count bug in the plugin, it deserves to die
b) if this can be worked around its not an issue anymore
a) ths is definitely ugly but unfortunately was legal :/ although its  
a quite specific case.

b,c) are not blockers for me, but a) could be.

Stefan

Quoting Sebastian Dröge <slomo at circular-chaos.org>:

> Hi,
> as some of you might've noticed there were some pad template and element
> details related changes in core CVS on Sunday, see
> http://bugzilla.gnome.org/show_bug.cgi?id=491501 for the reasoning.
>
> To summarize: It's now possible _and_ recommended to add pad templates
> and set element details in the class_init functions instead of base_init
> as before. Also one can replace pad templates now by simply adding a new
> one with the same name. It's still _not_ possible remove a pad template
> as this would cause unexpected effects when a subclass removes a pad
> template but in some base class method it is used.
>
> The reasoning for this is, that
> a) most object oriented languages don't have a concept of a base_init
> function that is called once for every subclass
> b) it's cleaner and the GObject way of doing things, see
> http://library.gnome.org/devel/gobject/unstable/gobject-Type-Information.html#GClassInitFunc
> c) class_init gets the class_data passed while base_init does not. This
> is very helpful for wrapper plugins like ladspa/libvisual/ffmpeg which
> currently do their own hacks to get the same things for base_init.
>
> To make this changes possible in a backward compatible way it is now
> also possible to add a pad template with the same name twice (or more).
> The old one will be simply replaced. In the past some elements have
> forced this pad-template-replacing by a hack (added pad template in
> class_init which caused it to not show up in subclasses).
>
>
>
> Ok, now to the bad news: Unfortunately this changes are not 100%
> backward compatible which I didn't expect before. There are currently 3
> known issues:
>
> 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.
>
>
> So the question is what to do about this 3 open issues. We could either
> say that a) is an unsupported hack and people should properly design
> their stuff, fix b) in core and declare pad template instances as
> immutable and c) say that this behaviour is a bug and only worked
> properly before by accident.
>
> Or we could revert the changes and keep everything as is until 0.11.
>
> Currently I would prefer to keep the changes but if there are too many
> breakages caused by this there's no other way than reverting.
>
> Please comment :)
>