After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 721953 - pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API
pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the Gs...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal enhancement
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 723236
Blocks:
 
 
Reported: 2014-01-10 18:28 UTC by Matthieu Bouron
Modified: 2014-03-05 19:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API (2.76 KB, patch)
2014-01-10 18:29 UTC, Matthieu Bouron
none Details | Review
pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API (2.81 KB, patch)
2014-01-10 18:29 UTC, Matthieu Bouron
needs-work Details | Review
pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API (5.46 KB, patch)
2014-01-20 13:05 UTC, Matthieu Bouron
none Details | Review
[PATCH 1/1] video-overlay-composition: add GST_CAPS_FEATURE_META_GST_VIDEO_OVERLY_COMPOSITION (892 bytes, patch)
2014-01-23 12:22 UTC, Matthieu Bouron
none Details | Review
pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API (7.39 KB, patch)
2014-01-23 12:28 UTC, Matthieu Bouron
none Details | Review
pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API (10.04 KB, patch)
2014-01-24 16:14 UTC, Matthieu Bouron
reviewed Details | Review
pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API (8.86 KB, patch)
2014-01-24 17:56 UTC, Matthieu Bouron
none Details | Review
267145: pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API (11.79 KB, patch)
2014-01-24 19:41 UTC, Matthieu Bouron
none Details | Review
267145: pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API (11.63 KB, patch)
2014-01-27 14:34 UTC, Matthieu Bouron
none Details | Review
[PATCH 1/3] video-overlay-composition: add GST_CAPS_FEATURE_META_GST_VIDEO_OVERLY_COMPOSITION (892 bytes, patch)
2014-01-29 19:26 UTC, Matthieu Bouron
committed Details | Review
[PATCH 2/3] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API (11.73 KB, patch)
2014-01-29 19:26 UTC, Matthieu Bouron
needs-work Details | Review
[PATCH 3/3] tests: add textoverlay passthrough with composition feature unit test (8.05 KB, patch)
2014-01-29 19:27 UTC, Matthieu Bouron
needs-work Details | Review
[PATCH 2/3] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API (11.79 KB, patch)
2014-01-30 16:08 UTC, Matthieu Bouron
needs-work Details | Review
[PATCH 2/3] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API (11.96 KB, patch)
2014-01-30 18:31 UTC, Matthieu Bouron
needs-work Details | Review
pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API (11.66 KB, patch)
2014-03-03 13:14 UTC, Matthieu Bouron
committed Details | Review
tests: add textoverlay passthrough with composition feature unit test (7.99 KB, patch)
2014-03-03 13:16 UTC, Matthieu Bouron
accepted-commit_now Details | Review
tests: add textoverlay passthrough with composition feature unit tests (11.97 KB, patch)
2014-03-05 17:15 UTC, Matthieu Bouron
committed Details | Review

Description Matthieu Bouron 2014-01-10 18:28:13 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.
Comment 1 Matthieu Bouron 2014-01-10 18:29:07 UTC
Created attachment 265963 [details] [review]
pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API
Comment 2 Matthieu Bouron 2014-01-10 18:29:57 UTC
Created attachment 265964 [details] [review]
pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API
Comment 3 Tim-Philipp Müller 2014-01-10 19:08:12 UTC
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.
Comment 4 Sebastian Dröge (slomo) 2014-01-14 09:10:32 UTC
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.
Comment 5 Sebastian Dröge (slomo) 2014-01-14 09:11:36 UTC
(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.
Comment 6 Nicolas Dufresne (ndufresne) 2014-01-16 18:28:23 UTC
(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 ?
Comment 7 Nicolas Dufresne (ndufresne) 2014-01-16 19:25:08 UTC
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 ?
Comment 8 Matthieu Bouron 2014-01-20 13:05:20 UTC
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 ?
Comment 9 Matthieu Bouron 2014-01-23 12:22:15 UTC
Created attachment 267038 [details] [review]
[PATCH 1/1] video-overlay-composition: add GST_CAPS_FEATURE_META_GST_VIDEO_OVERLY_COMPOSITION
Comment 10 Matthieu Bouron 2014-01-23 12:28:00 UTC
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.
Comment 11 Matthieu Bouron 2014-01-24 16:14:20 UTC
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.
Comment 12 Nicolas Dufresne (ndufresne) 2014-01-24 16:44:18 UTC
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.
Comment 13 Matthieu Bouron 2014-01-24 17:56:21 UTC
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.
Comment 14 Matthieu Bouron 2014-01-24 19:41:47 UTC
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.
Comment 15 Matthieu Bouron 2014-01-27 14:34:36 UTC
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 ?
Comment 16 Matthieu Bouron 2014-01-29 19:26:20 UTC
Created attachment 267571 [details] [review]
[PATCH 1/3] video-overlay-composition: add GST_CAPS_FEATURE_META_GST_VIDEO_OVERLY_COMPOSITION
Comment 17 Matthieu Bouron 2014-01-29 19:26:48 UTC
Created attachment 267572 [details] [review]
[PATCH 2/3] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API
Comment 18 Matthieu Bouron 2014-01-29 19:27:21 UTC
Created attachment 267573 [details] [review]
[PATCH 3/3] tests: add textoverlay passthrough with composition feature unit test
Comment 19 Nicolas Dufresne (ndufresne) 2014-01-29 21:36:45 UTC
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.
Comment 20 Matthieu Bouron 2014-01-30 16:08:40 UTC
Created attachment 267658 [details] [review]
[PATCH 2/3] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API
Comment 21 Nicolas Dufresne (ndufresne) 2014-01-30 17:59:35 UTC
Review of attachment 267571 [details] [review]:

++
Comment 22 Nicolas Dufresne (ndufresne) 2014-01-30 18:03:09 UTC
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.
Comment 23 Nicolas Dufresne (ndufresne) 2014-01-30 18:04:25 UTC
Review of attachment 267573 [details] [review]:

I'm ok with that one, when the other is ready.
Comment 24 Matthieu Bouron 2014-01-30 18:30:46 UTC
(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.
Comment 25 Matthieu Bouron 2014-01-30 18:31:09 UTC
Created attachment 267669 [details] [review]
[PATCH 2/3] pango: basetextoverlay: handle video/x-raw(ANY) if downstream supports the GstVideoOverlayCompositionMeta API
Comment 26 Nicolas Dufresne (ndufresne) 2014-01-30 18:46:35 UTC
Review of attachment 267669 [details] [review]:

I think it all make sense now
Comment 27 Sebastian Dröge (slomo) 2014-03-02 20:07:09 UTC
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
Comment 28 Sebastian Dröge (slomo) 2014-03-02 20:08:42 UTC
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 :)
Comment 29 Matthieu Bouron 2014-03-03 13:14:40 UTC
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.
Comment 30 Matthieu Bouron 2014-03-03 13:16:20 UTC
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 31 Sebastian Dröge (slomo) 2014-03-03 19:30:10 UTC
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 :)
Comment 32 Matthieu Bouron 2014-03-05 17:15:16 UTC
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.
Comment 33 Sebastian Dröge (slomo) 2014-03-05 19:40:22 UTC
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