GNOME Bugzilla – Bug 741263
videodecoder: implement caps query
Last modified: 2014-12-17 23:08:29 UTC
The videodecoder base class should implement the caps query to allow proxying downstream restrictions to upstream of the common video fields: width, height and framerate.
Created attachment 292322 [details] [review] videodecoder: implement caps query To allow the decoder to proxy downstream caps restrictions to upstream. This patch makes it proxy width, height and framerate.
Audiodecoder also needs this, will provide a patch once this one gets reviewed
Review of attachment 292322 [details] [review]: Makes sense ::: gst-libs/gst/video/gstvideodecoder.c @@ +1580,3 @@ +static GstCaps * +gst_video_decoder_create_other_caps (GstVideoDecoder * decoder, GstPad * pad, create_other_caps() seems like a weird name. Maybe check audioencoder/videoencoder for better naming, I remember they have also code like this (and you might want to refactor that into a utility function if possible). @@ +1590,3 @@ + const GValue *width, *height, *framerate; + + tmpl_caps = gst_pad_get_pad_template_caps (pad); These have to be unreffed after usage @@ +1609,3 @@ + gst_structure_set_value (result_structure, "height", height); + if (framerate) + gst_structure_set_value (result_structure, "framerate", framerate); pixel-aspect-ratio too maybe
Created attachment 292395 [details] [review] videodecoder: implement caps query Refactor the encoder's caps query proxying function to a common place and use it in the videodecoder to proxy downstream restrictions. The new function is private to the gstvideo lib.
Review of attachment 292395 [details] [review]: Looks good :)
Created attachment 292407 [details] [review] videoutils: return empty if the element has no possible allowed caps Instead of returning the template caps and having a failure happen later because there are no possible caps
Created attachment 292408 [details] [review] videoutils: proxy filter when doing a caps query downstream Allows downstream to use the filter and possibly reduce caps complexity to speed up negotiation
Review of attachment 292408 [details] [review]: You probably want to do all the same for audio btw :) ::: gst-libs/gst/video/gstvideoutilsprivate.c @@ +37,3 @@ + gint i, j; + + for (i = 0; i < gst_caps_get_size (templ_caps); i++) { Put gst_caps_get_size() out of the loop @@ +41,3 @@ + gst_structure_get_name_id (gst_caps_get_structure (templ_caps, i)); + + for (j = 0; j < gst_caps_get_size (caps); j++) { And here @@ +91,3 @@ gst_pad_get_pad_template_caps (sinkpad); + if (filter) { + GstCaps *our_src_caps = gst_pad_query_caps (srcpad, NULL); This potentially reverses the query direction. E.g. querying the srcpad can cause a query on the peer of the sinkpad... while a few lines below you also query the peer of the srcpad. If you're unlucky this can lead to an infinite loop of queries. Do you mean gst_pad_get_current_caps() here? Or just use the template caps as below? Why do you need to intersect with the srcpad caps at all? :) @@ +100,3 @@ + allowed = + gst_caps_intersect_full (peer_caps, our_src_caps, + GST_CAPS_INTERSECT_FIRST); peer_query_caps() can return GST_CAPS_ANY if there is no peer and filter, so you need to intersect with the template caps here at least.
(In reply to comment #8) > Review of attachment 292408 [details] [review]: > > You probably want to do all the same for audio btw :) > > ::: gst-libs/gst/video/gstvideoutilsprivate.c > @@ +37,3 @@ > + gint i, j; > + > + for (i = 0; i < gst_caps_get_size (templ_caps); i++) { > > Put gst_caps_get_size() out of the loop > > @@ +41,3 @@ > + gst_structure_get_name_id (gst_caps_get_structure (templ_caps, i)); > + > + for (j = 0; j < gst_caps_get_size (caps); j++) { > > And here > > @@ +91,3 @@ > gst_pad_get_pad_template_caps (sinkpad); > + if (filter) { > + GstCaps *our_src_caps = gst_pad_query_caps (srcpad, NULL); > > This potentially reverses the query direction. E.g. querying the srcpad can > cause a query on the peer of the sinkpad... while a few lines below you also > query the peer of the srcpad. If you're unlucky this can lead to an infinite > loop of queries. > > Do you mean gst_pad_get_current_caps() here? Or just use the template caps as > below? Why do you need to intersect with the srcpad caps at all? :) I guess I'll just default to using template caps. I added the query because I got get_allowed_caps and modified it to use a filter. So I guess using the get_allowed_caps a bit below is also a bad idea as it will query the source pad caps, too. Will update both branches. Thanks for the review. > > @@ +100,3 @@ > + allowed = > + gst_caps_intersect_full (peer_caps, our_src_caps, > + GST_CAPS_INTERSECT_FIRST); > > peer_query_caps() can return GST_CAPS_ANY if there is no peer and filter, so > you need to intersect with the template caps here at least.
Created attachment 292775 [details] [review] videoutils: proxy filter when doing a caps query downstream Allows downstream to use the filter and possibly reduce caps complexity to speed up negotiation
Created attachment 292776 [details] [review] videodecoder: implement accept caps query to handle image caps without framerate decoders that use image/* caps shouldn't care about framerate and would be able to accept a caps even when no framerate is specified. Due to the accept caps query use of subset caps comparison a caps missing a framerate would not be accepted. This patch adds special handling to ignore the framerate field in accept caps subset checks
Comment on attachment 292776 [details] [review] videodecoder: implement accept caps query to handle image caps without framerate This seems a bit bogus to me, there is nothing particularly special about image/* caps, they should be treated just like video/* caps IMHO. If no framerate is specified, 0/1 (variable framerate) should be assumed. The real problem here is of course that we otherwise rely on a parser being plugged in front, but I still don't think that we should treat image/* special.
(In reply to comment #12) > (From update of attachment 292776 [details] [review]) > This seems a bit bogus to me, there is nothing particularly special about > image/* caps, they should be treated just like video/* caps IMHO. If no > framerate is specified, 0/1 (variable framerate) should be assumed. The real > problem here is of course that we otherwise rely on a parser being plugged in > front, but I still don't think that we should treat image/* special. For doing that we would need to modify the typefind and it would break cases like: multifilesrc ! typefind ! jpegdec ! "image/x-raw, framerate=<something" ! videosink Unless we make the image decoders ignore the input framerate if it is 0/1 in favor of what downstream wants. But this would also be special casing.
The same issue should also happen with audio as some formats could be accepted even with missing information, like "audio/mpeg, mpegversion=1, layer=3" without channels and rate and the decoder should still be able to accept that.
Indeed. Parsers use a different acceptcaps for that, which is only checking for intersection instead of subset. But that will break if the decoder requires a specific codec profile, and upstream provides none in the caps. For example.
The problem is that right now the decoders always return the template caps and the subset check works ok with that. So some decoders that need all that have in the template caps not only the format 'name' but also the fields they required (width/height/rate/channels...) so the subset check works fine with the template. This patch fixes the caps query by adding the downstream restrictions but will break this subset check because it won't compare against the template caps anymore (where the required fields are put). So I'd propose to implement an accept caps query that does: 1) subset check the caps against the template caps 2) intersect with the query caps result. What do you think?
Created attachment 292860 [details] [review] videodecoder: accept-caps should only require fields from the template With the new caps query results the caps returned might have extra fields that are not required by the decoder (framerate for image decoders) and it causes a regression making, for example, jpegdec reject caps that don't have framerates. The accept-caps implementation will do 2 checks: 1) Do subset check with the template caps, making sure all the required fields that are present on the template are present on the received caps. 2) Do a intersection check with the result of a caps query, making sure that downstream can accept the fields in the received caps.
Sounds sensible, now this just needs to be considered once we add a getcaps vfunc to the audio/video decoder baseclass. In that case the result of that should be checked for subset instead of the template caps (and the template caps would be returned by the default implementation of getcaps).
(In reply to comment #18) > Sounds sensible, now this just needs to be considered once we add a getcaps > vfunc to the audio/video decoder baseclass. In that case the result of that > should be checked for subset instead of the template caps (and the template > caps would be returned by the default implementation of getcaps). I understand that once we have a getcaps function we should change the accept-caps implementation to do a caps query instead of short-circuiting the call to the internal function implementing it. Perhaps we want to do this now anyway in case any subclass overrides the query function on the pad directly. I didn't get the last part in parenthesis, isn't the default implementation what the patch adds here? That gets downstream caps and intersects with the pad template? Or do you want to change that to get downstream caps and intersect with the sublass getcaps. Anyway this is something that we can decide once we need this function.
Yeah, why not just add a getcaps vfunc now. I'm sure sooner or later we will need it anyway :) I only mentioned this because I wouldn't be surprised if we first added a getcaps function later without thinking about accept-caps, and then things are broken for a while ;) For that last part in the parenthesis, I think it didn't make any sense. Ignore it :) Sorry
Created attachment 292919 [details] [review] videodecoder: expose getcaps virtual function Allows subclasses to do custom caps query replies. Also exposes the standard caps query handler so subclasses can just extend on top of it instead of reimplementing the caps query proxying.
Review of attachment 292919 [details] [review]: ::: gst-libs/gst/video/gstvideodecoder.c @@ +1582,3 @@ + * gst_video_decoder_proxy_getcaps: + * @decoder: a #GstVideoDecoder + * @caps: initial caps @caps: (allow-none): initial caps @@ +1583,3 @@ + * @decoder: a #GstVideoDecoder + * @caps: initial caps + * @filter: filter caps Also (allow-none)? Can be NULL, right? @@ +1589,3 @@ + * elements. + * + * Returns: a #GstCaps owned by caller Returns: (transfer full): a #GstCaps owned by the caller @@ +1613,3 @@ + caps = klass->getcaps (decoder, filter); + else + caps = gst_video_decoder_proxy_getcaps (decoder, NULL, filter); I think here is a problem now with acceptcaps. klass->getcaps() can return a subset of the template caps, e.g. if the actual hardware currently available can do less than the element could do in general. In that case you would here return stricter caps, but acceptcaps would accept less strict caps (it only checks subset against template, and then intersection against gst_video_decoder_proxy_getcaps(), right?)
Thanks for the reviews. Pushed for video and also for audio. commit 8085352fb32c293c1128f5fcc6646f1f5e74663c Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Wed Dec 17 14:18:03 2014 -0300 videodecoder: expose getcaps virtual function Allows subclasses to do custom caps query replies. Also exposes the standard caps query handler so subclasses can just extend on top of it instead of reimplementing the caps query proxying. https://bugzilla.gnome.org/show_bug.cgi?id=741263 commit 49568005494bfdaf82734437ccaa0584eab22c9c Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Mon Dec 15 18:46:21 2014 -0300 videodecoder: accept-caps should only require fields from the template With the new caps query results the caps returned might have extra fields that are not required by the decoder (framerate for image decoders) and it causes a regression making, for example, jpegdec reject caps that don't have framerates. The accept-caps implementation will do 2 checks: 1) Do subset check with the template caps, making sure all the required fields that are present on the template are present on the received caps. 2) Do a intersection check with the result of a caps query, making sure that downstream can accept the fields in the received caps. https://bugzilla.gnome.org/show_bug.cgi?id=741263 commit 00c2ce60c34634008e533c96bcb5423cdbd32c41 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Tue Dec 9 16:08:12 2014 -0300 videoutils: proxy filter when doing a caps query downstream Allows downstream to use the filter and possibly reduce caps complexity to speed up negotiation https://bugzilla.gnome.org/show_bug.cgi?id=741263 commit f49208555226d3f65816ed143440a1777551620f Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Tue Dec 9 16:05:27 2014 -0300 videoutils: return empty if the element has no possible allowed caps Instead of returning the template caps and having a failure happen later because there are no possible caps https://bugzilla.gnome.org/show_bug.cgi?id=741263 commit f24075887f9d876af59f27a389f2e8c7d6c957ca Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Mon Dec 8 16:33:33 2014 -0300 videodecoder: implement caps query Refactor the encoder's caps query proxying function to a common place and use it in the videodecoder to proxy downstream restrictions. The new function is private to the gstvideo lib. https://bugzilla.gnome.org/show_bug.cgi?id=741263