GNOME Bugzilla – Bug 721953
pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API
Last modified: 2014-03-05 19:40:39 UTC
This patch makes the pango basetextoverlay element handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API since in this case it only places the correct meta on outgoing buffers. This is useful when an hardware decoder is used and a downstream element supports the overlay composition meta.
Created attachment 265963 [details] [review] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API
Created attachment 265964 [details] [review] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API
This doesn't look right to me. You can't just claim to support everything on your template caps and error out if you get something you don't like. Defeats negotiation.
Review of attachment 265964 [details] [review]: What Tim said. This should also be negotiated and you only allow passthrough if downstream supports the videooverlay meta. Requires doing an allocation query from the CAPS query handler unfortunately.
(In reply to comment #4) > Review of attachment 265964 [details] [review]: > > What Tim said. This should also be negotiated and you only allow passthrough if > downstream supports the videooverlay meta. Requires doing an allocation query > from the CAPS query handler unfortunately. Also same as for videobalance and deinterlace applies here.
(In reply to comment #4) > Review of attachment 265964 [details] [review]: > > What Tim said. This should also be negotiated and you only allow passthrough if > downstream supports the videooverlay meta. Requires doing an allocation query > from the CAPS query handler unfortunately. Is that even doable ? How can we finish building the pipeline if we don't have caps yet ?
Got two solution for this, plan A would be as describe by slomo: -> CAPS QUERY ALLOCATION QUERY -> if comp <- ANY else <- what we support in SW What I'm not sure is if in this pattern the allocation query may fail (e.g. too soon) ? Plan B would be: -> CAPS QUERY if no comp <- what we support in SW else <- ANY -> CAPS EVENT: if no comp state yet QUERY ALLOCATION save comp state if no comp && !is_subset(incaps, SW caps)) <- RECONFIGURE EVENT ... Ideas ?
Created attachment 266726 [details] [review] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API Patch updated using Nicolas' second solution. What do you think ?
Created attachment 267038 [details] [review] [PATCH 1/1] video-overlay-composition: add GST_CAPS_FEATURE_META_GST_VIDEO_OVERLY_COMPOSITION
Created attachment 267039 [details] [review] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API Here is a work in progress using the caps query with caps features. It seems to work fine in the upstream -> downstream direction however i'm wondering how we should handle the downstream -> upstream case since at some point we don't know what we really support. My thoughts about it would be to somehow keep track of what we support during the caps query in the downstream -> upstream direction. Just filtering using the caps template feels wrong to me. Comments welcome.
Created attachment 267140 [details] [review] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API Patch updated, upstream caps are filtered against what downstream supports in the src caps query handler. However, querying downstream in the src caps query handler seems a bit risky (loop ?) or incorrect in terms of design, is it ? To avoid querying downstream at this stage, I was thinking about storing storying downstream caps when it is queried in the sink caps handler.
Review of attachment 267140 [details] [review]: ::: ext/pango/gstbasetextoverlay.c @@ +153,3 @@ + +#define BASE_TEXT_OVERLAY_ALL_CAPS BASE_TEXT_OVERLAY_CAPS ";" \ + GST_VIDEO_CAPS_MAKE_WITH_FEATURES ("ANY", GST_VIDEO_FORMATS_ALL) I think this is right for SINK caps, but for SRC caps you already know that for this case you'll add (and require) the composition meta support. So I'm wondering why you don't do that ? @@ +1109,1 @@ + peer_caps = gst_pad_peer_query_caps (otherpad, overlay_filter); With the proposed template modification, I don't see why you would need a special implementation of the caps query.
Created attachment 267145 [details] [review] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API Patch updated. Logic in src query handler is simplified.
Created attachment 267151 [details] [review] 267145: pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API Patch updated, getcaps splitted into get_videosink_caps and get_src_caps. Logic of both query handler should be ok now.
Created attachment 267314 [details] [review] 267145: pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API Patch updated, processing functions have been factorized. Since they will be reused in other elements I wonder If they should be shipped by gst-plugins-base libs. What do you think ?
Created attachment 267571 [details] [review] [PATCH 1/3] video-overlay-composition: add GST_CAPS_FEATURE_META_GST_VIDEO_OVERLY_COMPOSITION
Created attachment 267572 [details] [review] [PATCH 2/3] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API
Created attachment 267573 [details] [review] [PATCH 3/3] tests: add textoverlay passthrough with composition feature unit test
Review of attachment 267572 [details] [review]: My only concert is in the videosink_query, we need not to hide that the sink has the feature support to compose in order to allow having multiple textoverlay one after the other. Would it be possible to check if that is handled correctly ? ::: ext/pango/gstbasetextoverlay.c @@ +552,3 @@ + gst_caps_unref (overlay->sw_template_caps); + overlay->sw_template_caps = NULL; + } Should be a static global caps instead of per-instance allocated. @@ +796,3 @@ + GST_DEBUG_OBJECT (overlay, "unsupported caps %" GST_PTR_FORMAT, caps); + gst_pad_push_event (overlay->video_sinkpad, + gst_event_new_custom (GST_EVENT_RECONFIGURE, NULL)); I think this is the the right use of RECONFIGURE, and this should be removed. @@ +1083,3 @@ static GstCaps * +gst_base_text_overlay_add_caps_feature_support (GstCaps * caps, + const gchar * feature, GstCaps * filter) This is very confusing, as the filter does not match the query filter, but it the sw caps supported by the renderer, while the caps match the query filter. I would better name that helper and it's parameter to reflect what it's used for. @@ +1103,3 @@ +static GstCaps * +gst_base_text_overlay_filter_caps_by_feature (GstCaps * caps, + const gchar * feature, GstCaps * filter) Again the use of filter is a bit confusing here @@ +1145,3 @@ + GstCaps *peer_caps = NULL, *caps = NULL, *overlay_filter = NULL; + + if (G_UNLIKELY (!overlay)) Is this really possible ? @@ +1175,3 @@ + GST_DEBUG_OBJECT (pad, "our template %" GST_PTR_FORMAT, templ); + caps = + gst_caps_intersect_full (peer_caps, templ, GST_CAPS_INTERSECT_FIRST); If peer_caps is ANY, this will return a copy of the template, why doing an intersection ? We know thought that this is no filter or the filter is ANY, so no need to intersect with the filter. @@ +1242,3 @@ + GST_DEBUG_OBJECT (pad, "our template %" GST_PTR_FORMAT, templ); + caps = + gst_caps_intersect_full (peer_caps, templ, GST_CAPS_INTERSECT_FIRST); Same.
Created attachment 267658 [details] [review] [PATCH 2/3] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API
Review of attachment 267571 [details] [review]: ++
Review of attachment 267658 [details] [review]: Very close now. ::: ext/pango/gstbasetextoverlay.c @@ +555,3 @@ + gst_caps_unref (overlay->sw_template_caps); + overlay->sw_template_caps = NULL; + } Not that important, but gst_caps_replace() is could be used. ::: ext/pango/gstbasetextoverlay.h @@ +159,3 @@ GstVideoOverlayComposition *composition; + + GstCaps *sw_template_caps; I would suggest moving that into the class instance, a bit more difficult to read, but I think this is where it belongs.
Review of attachment 267573 [details] [review]: I'm ok with that one, when the other is ready.
(In reply to comment #22) > Review of attachment 267658 [details] [review]: > > Very close now. > > ::: ext/pango/gstbasetextoverlay.c > @@ +555,3 @@ > + gst_caps_unref (overlay->sw_template_caps); > + overlay->sw_template_caps = NULL; > + } > > Not that important, but gst_caps_replace() is could be used. Not needed anymore. sw_template_caps is now a class parameter. > > ::: ext/pango/gstbasetextoverlay.h > @@ +159,3 @@ > GstVideoOverlayComposition *composition; > + > + GstCaps *sw_template_caps; > > I would suggest moving that into the class instance, a bit more difficult to > read, but I think this is where it belongs. Done in the upcoming patch. Thanks for the review.
Created attachment 267669 [details] [review] [PATCH 2/3] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API
Review of attachment 267669 [details] [review]: I think it all make sense now
Review of attachment 267669 [details] [review]: Basically same comments as for assrender. Almost ready to be merged :) ::: ext/pango/gstbasetextoverlay.c @@ +394,3 @@ klass->get_text = gst_base_text_overlay_get_text; + klass->sw_template_caps = gst_static_caps_get (&sw_template_caps); No need to store this in the class, just get the caps whenever needed @@ +1149,3 @@ + + gst_caps_append (new_caps, filtered_caps); + gst_caps_unref (simple_caps); If you first unref and then append it is more efficient as the simple caps can then just be taken directly instead of copying them @@ +1188,3 @@ + + /* if peer returns ANY caps, return src pad template caps */ + caps = gst_caps_copy (gst_pad_get_pad_template_caps (srcpad)); Apply filter here @@ +1251,3 @@ + + /* if peer returns ANY caps, return sink pad template caps */ + caps = gst_caps_copy (gst_pad_get_pad_template_caps (sinkpad)); Apply filter here
Review of attachment 267573 [details] [review]: Almost ready too ::: tests/check/elements/textoverlay.c @@ +54,3 @@ +#define VIDEO_CAPS_TEMPLATE_WITH_FEATURE_STRING \ + "video/x-raw(meta:GstVideoOverlayComposition), " \ You have a #define for that :) @@ +111,3 @@ + gboolean need_pool = FALSE; + + gst_query_parse_allocation (query, &caps, &need_pool); You don't need to call this here if you don't use the results anyway @@ +476,3 @@ GST_END_TEST; +GST_START_TEST (test_video_passthrough_with_feature) What this should test if *unsupported* caps are working if the comp meta is supported. E.g. put video/x-raw(memory:EGLImage) there or some arbitrary video/x-raw,format=ABCD However that test here as is, is also a bit useful. So keep it around and just add a new one :)
Created attachment 270790 [details] [review] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API Patch updated, taking into account latest comments.
Created attachment 270791 [details] [review] tests: add textoverlay passthrough with composition feature unit test Patch updated, taking into account latest comments. A new test still needs to be added to test with unsupported caps.
Comment on attachment 270790 [details] [review] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API Ok, can be pushed after the new test is there :)
Created attachment 271012 [details] [review] tests: add textoverlay passthrough with composition feature unit tests Patch updated, adding a new test with unsupported caps (I420+GLTextureUpload). I kept the video-format to I420 to keep consistency with what's inside the generated video buffer plus choosing an arbitrary format like ABCD won't work since the textoverlay template caos are SW CAPS + GST_VIDEO_FORMATS_ALL/ANY.
commit c904661dc320cd7233b1707aa7fc7c3f7a948d4e Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Thu Jan 30 15:41:49 2014 +0000 tests: add textoverlay passthrough with composition feature unit tests https://bugzilla.gnome.org/show_bug.cgi?id=721953 commit ed8e7d42752c4e67ca5392b6c3ffe76abbe061ae Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Thu Jan 23 12:20:05 2014 +0000 pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the G https://bugzilla.gnome.org/show_bug.cgi?id=721953 commit a8951c16dadf2ec6d28ad7c938b6fe674d97bbc2 Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Thu Jan 23 12:19:13 2014 +0000 video-overlay-composition: add GST_CAPS_FEATURE_META_GST_VIDEO_OVERLAY_COMPO