GstPadTemplate and element details changes in CVS

[Sebastian Dröge]

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Michael Smith-32]

On Mon, 2008-02-04 at 19:54 +0100, Sebastian Dröge wrote:
> 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.

I think (a) should be considered a hack, that just sounds horrid. And we
should fix it in our sources. However, it does work, and we do do it.
Likewise (b) and (c) - we have plugins doing this stuff, so it's
unreasonable to assume that nobody else does.

API/ABI stability is something we tout as a major benefit of using gstreamer.
Breaking it should not be an option (it can happen accidently; we're not perfect)

Just like the many other not-quite-ideal bits of API, this change should
be marked as an intended change for 0.11, and we should do it then.

That's my 2 cents, anyway :-)

Mike

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Michael Smith-32]

On Mon, 2008-02-04 at 19:54 +0100, Sebastian Dröge wrote:
> Or we could revert the changes and keep everything as is until 0.11.

Oh, and the other (biggest?) reason to revert this stuff: the benefits
you describe seem small; there's nothing essential here. They're just
"it'd be a bit nicer if...". Breaking API should, I think, only even be
_considered_ if there's absolutely no other choice.

Mike

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Stefan Sauer]

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 <[hidden email]>:

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Jan Schmidt-2]

<quote who="Michael Smith">

> On Mon, 2008-02-04 at 19:54 +0100, Sebastian Dröge wrote:
> > 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.
>
> I think (a) should be considered a hack, that just sounds horrid. And we
> should fix it in our sources. However, it does work, and we do do it.
> Likewise (b) and (c) - we have plugins doing this stuff, so it's
> unreasonable to assume that nobody else does.
>
> API/ABI stability is something we tout as a major benefit of using gstreamer.
> Breaking it should not be an option (it can happen accidently; we're not perfect)
>
> Just like the many other not-quite-ideal bits of API, this change should
> be marked as an intended change for 0.11, and we should do it then.
>
> That's my 2 cents, anyway :-)
>

As usual, I agree with everything Mike says. Our goal with ABI/API stability
is that you should always be able to upgrade GStreamer core and base and
existing elements continue to work without re-compiling, even if those
elements are relying on reasonable-but-accidental behaviour.

The only alternative I can offer is to consider the idea of a new variant
of gst_element_class_add_pad_template() to provide new semantics while
preserving the existing behaviour in the old function. The idea would then
be to move over elements explicitly and avoid breakage.

Another idea, add something to GstElementClass that allows sub-classes to
signal which behaviour they want (flags or such), with the default being
the old behaviour of leaving an extra ref on the pad template.

I think it's fine to consider pad template's immutable, and document it
as such - at least the GstPadTemplate base-class portions. Obviously
sub-classes can do what they like with their private pieces.

J.
--
Jan Schmidt                                    [hidden email]

I had my head shaved while Kerri-Anne Kennerly was close enough to touch.
 - Peter Hardy

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel