Re: gstreamer: baseparse: add getcaps function

[Sebastian Dröge-7]

On Di, 2011-10-18 at 04:29 -0700, Thiago Sousa Santos wrote:

> Module: gstreamer
> Branch: master
> Commit: e3f2d7db71dfb14441648033eb6cf3324ccc98b3
> URL:    http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=e3f2d7db71dfb14441648033eb6cf3324ccc98b3
>
> Author: Thiago Santos <[hidden email]>
> Date:   Mon Oct 17 14:42:08 2011 -0300
>
> baseparse: add getcaps function
>
> [...]
>
> +  if (klass->get_sink_caps)
> +    caps = klass->get_sink_caps (parse);
> +  else
> +    caps = gst_pad_proxy_getcaps (pad);

Hi,

this is completely breaking backwards compatibility with the old
baseparse subclasses. Let's take two examples:

1) h264parse allows to convert between different stream-formats. If
downstream now requires a specific stream-format,
gst_pad_proxy_getcaps() will propagate this singla stream-format to
upstream and if upstream wants to provide something different
negotiation will fail. But it shouldn't, h264parse can convert between
the stream-formats. f3f9e4b9786d00299e0c6909180c4cd326489bdf fixes this
in h264parse but it's nonetheless API/ABI breakage.

2) Consider a parser and downstream requests parsed=true on the caps.
gst_pad_proxy_getcaps() will propagate the parsed=true field to the
sinkpad caps and if upstream provides unparsed input negotiation will
fail again.

_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

[Thiago Santos]

On Wed, 2011-11-16 at 10:51 -0800, Sebastian Dröge wrote:

> On Di, 2011-10-18 at 04:29 -0700, Thiago Sousa Santos wrote:
> > Module: gstreamer
> > Branch: master
> > Commit: e3f2d7db71dfb14441648033eb6cf3324ccc98b3
> > URL:    http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=e3f2d7db71dfb14441648033eb6cf3324ccc98b3
> >
> > Author: Thiago Santos <[hidden email]>
> > Date:   Mon Oct 17 14:42:08 2011 -0300
> >
> > baseparse: add getcaps function
> >
> > [...]
> >
> > +  if (klass->get_sink_caps)
> > +    caps = klass->get_sink_caps (parse);
> > +  else
> > +    caps = gst_pad_proxy_getcaps (pad);
>
> Hi,

Hello,

>
> this is completely breaking backwards compatibility with the old
> baseparse subclasses. Let's take two examples:
>
> 1) h264parse allows to convert between different stream-formats. If
> downstream now requires a specific stream-format,
> gst_pad_proxy_getcaps() will propagate this singla stream-format to
> upstream and if upstream wants to provide something different
> negotiation will fail. But it shouldn't, h264parse can convert between
> the stream-formats. f3f9e4b9786d00299e0c6909180c4cd326489bdf fixes this
> in h264parse but it's nonetheless API/ABI breakage.

There was a patch for that in the h264parse in
https://bugzilla.gnome.org/show_bug.cgi?id=661874 waiting for review.
Was waiting it to be accepted before using the same approach on other
parsers that can do conversions.

>
> 2) Consider a parser and downstream requests parsed=true on the caps.
> gst_pad_proxy_getcaps() will propagate the parsed=true field to the
> sinkpad caps and if upstream provides unparsed input negotiation will
> fail again.

I guess removing parsed/framed entries on caps can be added as a default
handling on baseparse.


The other option is to revert that patch and add a getcaps to all
parsers as the default getcaps gets the templatecaps. Template caps
won't have any other downstream restrictions on width/height/framerate
propagated.

--
Thiago

> _______________________________________________
> gstreamer-devel mailing list
> [hidden email]
> http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel


_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

[Sebastian Dröge-7]

On Mi, 2011-11-16 at 17:21 -0300, Thiago Sousa Santos wrote:

> On Wed, 2011-11-16 at 10:51 -0800, Sebastian Dröge wrote:
> > On Di, 2011-10-18 at 04:29 -0700, Thiago Sousa Santos wrote:
> > > Module: gstreamer
> > > Branch: master
> > > Commit: e3f2d7db71dfb14441648033eb6cf3324ccc98b3
> > > URL:    http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=e3f2d7db71dfb14441648033eb6cf3324ccc98b3
> > >
> > > Author: Thiago Santos <[hidden email]>
> > > Date:   Mon Oct 17 14:42:08 2011 -0300
> > >
> > > baseparse: add getcaps function
> > >
> > > [...]
> > >
> > > +  if (klass->get_sink_caps)
> > > +    caps = klass->get_sink_caps (parse);
> > > +  else
> > > +    caps = gst_pad_proxy_getcaps (pad);
> >
> > Hi,
>
> Hello,
>
> >
> > this is completely breaking backwards compatibility with the old
> > baseparse subclasses. Let's take two examples:
> >
> > 1) h264parse allows to convert between different stream-formats. If
> > downstream now requires a specific stream-format,
> > gst_pad_proxy_getcaps() will propagate this singla stream-format to
> > upstream and if upstream wants to provide something different
> > negotiation will fail. But it shouldn't, h264parse can convert between
> > the stream-formats. f3f9e4b9786d00299e0c6909180c4cd326489bdf fixes this
> > in h264parse but it's nonetheless API/ABI breakage.
>
> There was a patch for that in the h264parse in
> https://bugzilla.gnome.org/show_bug.cgi?id=661874 waiting for review.
> Was waiting it to be accepted before using the same approach on other
> parsers that can do conversions.

Oh, sorry that I forgot about this patch. Is yours better/different than
mine?

> >
> > 2) Consider a parser and downstream requests parsed=true on the caps.
> > gst_pad_proxy_getcaps() will propagate the parsed=true field to the
> > sinkpad caps and if upstream provides unparsed input negotiation will
> > fail again.
>
> I guess removing parsed/framed entries on caps can be added as a default
> handling on baseparse.
>
>
> The other option is to revert that patch and add a getcaps to all
> parsers as the default getcaps gets the templatecaps. Template caps
> won't have any other downstream restrictions on width/height/framerate
> propagated.

I think to be really backwards compatible we have to always return the
template caps if the subclass didn't implement a getcaps function
(instead of using gst_pad_proxy_getcaps()). In 0.11 we could change that
to do something more clever and requiring subclasses to implement the
getcaps function though.

Nonetheless it makes sense to implement the getcaps function in 0.10 for
all existing parsers to propagate the downstream caps.

_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

[Thiago Santos]

On Wed, 2011-11-16 at 14:06 -0800, Sebastian Dröge wrote:

> On Mi, 2011-11-16 at 17:21 -0300, Thiago Sousa Santos wrote:
> > On Wed, 2011-11-16 at 10:51 -0800, Sebastian Dröge wrote:
> > > On Di, 2011-10-18 at 04:29 -0700, Thiago Sousa Santos wrote:
> > > > Module: gstreamer
> > > > Branch: master
> > > > Commit: e3f2d7db71dfb14441648033eb6cf3324ccc98b3
> > > > URL:    http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=e3f2d7db71dfb14441648033eb6cf3324ccc98b3
> > > >
> > > > Author: Thiago Santos <[hidden email]>
> > > > Date:   Mon Oct 17 14:42:08 2011 -0300
> > > >
> > > > baseparse: add getcaps function
> > > >
> > > > [...]
> > > >
> > > > +  if (klass->get_sink_caps)
> > > > +    caps = klass->get_sink_caps (parse);
> > > > +  else
> > > > +    caps = gst_pad_proxy_getcaps (pad);
> > >
> > > Hi,
> >
> > Hello,
> >
> > >
> > > this is completely breaking backwards compatibility with the old
> > > baseparse subclasses. Let's take two examples:
> > >
> > > 1) h264parse allows to convert between different stream-formats. If
> > > downstream now requires a specific stream-format,
> > > gst_pad_proxy_getcaps() will propagate this singla stream-format to
> > > upstream and if upstream wants to provide something different
> > > negotiation will fail. But it shouldn't, h264parse can convert between
> > > the stream-formats. f3f9e4b9786d00299e0c6909180c4cd326489bdf fixes this
> > > in h264parse but it's nonetheless API/ABI breakage.
> >
> > There was a patch for that in the h264parse in
> > https://bugzilla.gnome.org/show_bug.cgi?id=661874 waiting for review.
> > Was waiting it to be accepted before using the same approach on other
> > parsers that can do conversions.
>
> Oh, sorry that I forgot about this patch. Is yours better/different than
> mine?
>

Your implementation is simpler, I'd go with that.

> > >
> > > 2) Consider a parser and downstream requests parsed=true on the caps.
> > > gst_pad_proxy_getcaps() will propagate the parsed=true field to the
> > > sinkpad caps and if upstream provides unparsed input negotiation will
> > > fail again.
> >
> > I guess removing parsed/framed entries on caps can be added as a default
> > handling on baseparse.
> >
> >
> > The other option is to revert that patch and add a getcaps to all
> > parsers as the default getcaps gets the templatecaps. Template caps
> > won't have any other downstream restrictions on width/height/framerate
> > propagated.
>
> I think to be really backwards compatible we have to always return the
> template caps if the subclass didn't implement a getcaps function
> (instead of using gst_pad_proxy_getcaps()). In 0.11 we could change that
> to do something more clever and requiring subclasses to implement the
> getcaps function though.
>
> Nonetheless it makes sense to implement the getcaps function in 0.10 for
> all existing parsers to propagate the downstream caps.

I agree, let's revert that patch and add getcaps manually to our
parsers. Maybe we could add a generic getcaps convenience function on
baseparse that parsers could point to for a default implementation to
avoid replicating code. Might be enough for the ones that won't do
conversion.

--
Thiago

> _______________________________________________
> gstreamer-devel mailing list
> [hidden email]
> http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel


_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

[Sebastian Dröge-7]

On Mi, 2011-11-16 at 19:28 -0300, Thiago Sousa Santos wrote:

> > > >
> > > > 2) Consider a parser and downstream requests parsed=true on the caps.
> > > > gst_pad_proxy_getcaps() will propagate the parsed=true field to the
> > > > sinkpad caps and if upstream provides unparsed input negotiation will
> > > > fail again.
> > >
> > > I guess removing parsed/framed entries on caps can be added as a default
> > > handling on baseparse.
> > >
> > >
> > > The other option is to revert that patch and add a getcaps to all
> > > parsers as the default getcaps gets the templatecaps. Template caps
> > > won't have any other downstream restrictions on width/height/framerate
> > > propagated.
> >
> > I think to be really backwards compatible we have to always return the
> > template caps if the subclass didn't implement a getcaps function
> > (instead of using gst_pad_proxy_getcaps()). In 0.11 we could change that
> > to do something more clever and requiring subclasses to implement the
> > getcaps function though.
> >
> > Nonetheless it makes sense to implement the getcaps function in 0.10 for
> > all existing parsers to propagate the downstream caps.
>
> I agree, let's revert that patch and add getcaps manually to our
> parsers. Maybe we could add a generic getcaps convenience function on
> baseparse that parsers could point to for a default implementation to
> avoid replicating code. Might be enough for the ones that won't do
> conversion.

I'd leave the ::get_sink_caps() vfunc in baseparse but change the code
to not call gst_pad_proxy_getcaps() if it isn't implemented but instead
just return the template caps.

No need to revert anything anywhere... I hope :)

_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel