[gst-devel] Re: [gst-cvs] rbultje gstreamer: gstreamer/gst/ gstreamer/gst/elements/ gstreamer/testsuite/elements/

[Ronald Bultje]

Hi Thomas,

On Tue, 2002-09-10 at 12:29, Thomas Vander Stichele wrote:
> > Log message:
> > This changes an important part of the plugin API, gst_pad_try_set_caps()
> > no longer returns a boolean, it now returns a GstPadConnectReturn,
> > which makes much more sense than a boolean. All plugins have also
> > been changed, so don't worry ;)
> 
> Hm, I have a few questions re: this change.  I might have some things 
> wrong so feel free to explain to me if I am ;)

I'll try. :-).

> First of all, it seems to mix stuff from the pads with stuff from the 
> caps.  Should a _caps function return a pad return value ?

Not really. The function is a pad function: gst_pad_try_set_caps. It
tries to set a caps on a pad, so it's really a pad thing, not a caps
thing. So returning a pad value seems OK to me.

> Second, the fixes to the plug-ins all depend on the numerical value of the
> current GstPadConnectReturn enum, which I think is a bad idea.
> First of all, the tests lose real-world meaning - I think they should at 
> least be done with the actual enum names.  I'd rather not even be them
> inequality comparisons but actual comparisons with or.

Completely agree here. I'll try to go through the plugins and fix that
up.

> Third, while the idea itself looks valid to change this, is there an 
> actual need for it ? I don't like changing the API at all when going to 
> 0.4.0 to 0.4.1 unless it's been discussed and it has shown to be a 
> problem.  I had hoped this to be mainly a bugfix release.
> (If it does fix a valid problem, however, it should of course be 
> considered).

I think Wim and I have been talking about this change since 0.3.0 or so.
We never did since I didn't do it and Wim probably had better things to
do. I've been wanting this change for a long time.

I've discussed this with Wim yesterday and he agreed here. I personally
think you're correct about not changing the API, especially after 0.5.0.
That's why I wanted this fixed before 0.5.0, because it really is a bug.

The problem is that gst_pad_try_set_caps() cna give three useful return
values: OK/DONE (these pretty much mean the same as response value for
another plugin), DELAYED or REFUSED. If OK/DONE, we're happy. That's
what TRUE used to mean. DELAYED and REFUSED, however, are not the same,
so returning FALSE for both is a terrible thing. If the plugin returns
DELAYED, then the caller of gst_pad_try_set_caps should also return
DELAYED. Most plugins still don't do this (I just make quick changes),
but some already do now. This distinction is very important, and can be
imlemented now. With that, a reply of DELAYED can cause capsnego to be
triggered for a second time, while a reply of REFUSED means that we're
dead and the connecting plugins don't like each other.

I hope you agree on the necessity of this distinction.

With my changes to the plugins, all plugins now behave the same like
they used to (which is still semi-broken, since some still think that
REFUSED and DELAYED is the same), but it's not 'fixable'. Plugins can be
changed one at a time to behave differently on DELAYED and REFUSED, if
that's necessary. So everything should now behave the same. Some plugins
have already been changed in their behaviour, and I'm planning to fix at
least *some* more plugins (guess which ones? ;-) ). It'll make gstreamer
better on the whole, imho.

> Basically, I'm saying a change this big makes me feel uneasy about doing a 
> release in three days time ;) I'd like some more explanations on the 
> patch, and at least have it look better than it does now.

Sorry I did it so close to the release, but this one really *needed* to
go in before 0.5.0 at all cost. After 0.5.0, I want to stay away from
the API and never ever change it again. ;-). Until 0.5.0, I personally
feel that necessary changes can go in. After all, it doesn't affect
applications (since they shouldn't use gst_pad_try_set_caps()), but only
some plugins.

I hope I've explained the patch a bit better now.

Ronald