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 720345 - videobalance: handle unsupported caps features in passthrough mode
videobalance: handle unsupported caps features in passthrough mode
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-12-12 19:26 UTC by Matthieu Bouron
Modified: 2018-11-03 14:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
videobalance: handle unsupported caps features in passthrough mode (4.41 KB, patch)
2013-12-12 19:27 UTC, Matthieu Bouron
needs-work Details | Review
videobalance: handle unsupported caps features in passthrough mode (4.43 KB, patch)
2014-01-06 11:36 UTC, Matthieu Bouron
needs-work Details | Review
videobalance: add best-effort mode (7.06 KB, patch)
2014-01-09 18:44 UTC, Matthieu Bouron
needs-work Details | Review
videobalance: add fallback-to-passthrough property (7.56 KB, patch)
2014-01-10 14:20 UTC, Matthieu Bouron
none Details | Review
videobalance: add fallback-to-passthrough property (7.56 KB, patch)
2014-01-10 14:22 UTC, Matthieu Bouron
none Details | Review
videobalance: add fallback-to-passthrough property (7.57 KB, patch)
2014-01-10 14:43 UTC, Matthieu Bouron
reviewed Details | Review

Description Matthieu Bouron 2013-12-12 19:26:15 UTC
This patch allows videobalance to work with unsupported caps features in passthrough mode.
This is especially usefull when an hardware decoder is plugged by playbin.
Comment 1 Matthieu Bouron 2013-12-12 19:27:54 UTC
Created attachment 264098 [details] [review]
videobalance: handle unsupported caps features in passthrough mode
Comment 2 Sebastian Dröge (slomo) 2013-12-14 17:21:55 UTC
Review of attachment 264098 [details] [review]:

::: gst/videofilter/gstvideobalance.c
@@ +77,3 @@
+
+#define VIDEOBALANCE_ALL_CAPS VIDEOBALANCE_CAPS ";" \
+    GST_VIDEO_CAPS_MAKE_WITH_FEATURES ("ANY", VIDEOBALANCE_VIDEO_FORMATS)

For this to all make any sense it would be good to implement support for every video format, with the help of the pack/unpack functions from GstVideoFormatInfo. Because why allow NV12/VASurface, but disallow NV16/VASurface?

And just doing passthrough on any unsupported video format will cause unexpected behaviour. Maybe we also need a property to select this auto-passthrough-if-unsupported mode?

@@ +427,3 @@
 
+static gboolean
+gst_video_balance_can_handle_caps (GstCaps * srccaps)

Also should happen on otherwise unsupported caps (video format!)

@@ +435,3 @@
+  caps = gst_caps_new_empty_simple ("video/x-raw");
+  features =
+      gst_caps_features_new (GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY, NULL);

Not needed, sysmem is there by default
Comment 3 Sebastian Dröge (slomo) 2013-12-30 10:27:25 UTC
Matthieu?
Comment 4 Matthieu Bouron 2014-01-06 11:36:09 UTC
Created attachment 265428 [details] [review]
videobalance: handle unsupported caps features in passthrough mode
Comment 5 Matthieu Bouron 2014-01-06 11:41:41 UTC
(In reply to comment #2)
> Review of attachment 264098 [details] [review]:
> 
> ::: gst/videofilter/gstvideobalance.c
> @@ +77,3 @@
> +
> +#define VIDEOBALANCE_ALL_CAPS VIDEOBALANCE_CAPS ";" \
> +    GST_VIDEO_CAPS_MAKE_WITH_FEATURES ("ANY", VIDEOBALANCE_VIDEO_FORMATS)
> 
> For this to all make any sense it would be good to implement support for every
> video format, with the help of the pack/unpack functions from
> GstVideoFormatInfo. Because why allow NV12/VASurface, but disallow
> NV16/VASurface?

If you agree with it, i'll do that in separate.

> 
> And just doing passthrough on any unsupported video format will cause
> unexpected behaviour. Maybe we also need a property to select this
> auto-passthrough-if-unsupported mode?

The purpose is to make videobalance disable itself by default if it does not support the caps (and in particular the caps feature) to make hardware decoders work with playbin in the current situation, so I don't think a new property would be useful in this case.

> 
> @@ +427,3 @@
> 
> +static gboolean
> +gst_video_balance_can_handle_caps (GstCaps * srccaps)
> 
> Also should happen on otherwise unsupported caps (video format!)
> 
> @@ +435,3 @@
> +  caps = gst_caps_new_empty_simple ("video/x-raw");
> +  features =
> +      gst_caps_features_new (GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY, NULL);
> 
> Not needed, sysmem is there by default
Comment 6 Sebastian Dröge (slomo) 2014-01-08 09:38:40 UTC
(In reply to comment #5)
> (In reply to comment #2)
> > Review of attachment 264098 [details] [review] [details]:
> > 
> > ::: gst/videofilter/gstvideobalance.c
> > @@ +77,3 @@
> > +
> > +#define VIDEOBALANCE_ALL_CAPS VIDEOBALANCE_CAPS ";" \
> > +    GST_VIDEO_CAPS_MAKE_WITH_FEATURES ("ANY", VIDEOBALANCE_VIDEO_FORMATS)
> > 
> > For this to all make any sense it would be good to implement support for every
> > video format, with the help of the pack/unpack functions from
> > GstVideoFormatInfo. Because why allow NV12/VASurface, but disallow
> > NV16/VASurface?
> 
> If you agree with it, i'll do that in separate.

Yes

> > 
> > And just doing passthrough on any unsupported video format will cause
> > unexpected behaviour. Maybe we also need a property to select this
> > auto-passthrough-if-unsupported mode?
> 
> The purpose is to make videobalance disable itself by default if it does not
> support the caps (and in particular the caps feature) to make hardware decoders
> work with playbin in the current situation, so I don't think a new property
> would be useful in this case.

It should be a new property IMHO, where the default is the current behaviour. And playbin would set it to the behaviour your patch adds.
Comment 7 Nicolas Dufresne (ndufresne) 2014-01-08 15:08:10 UTC
What would we do if a color balance parameter is set but the current pass-through caps is unsupported ? Doing nothing would be persived as a buggy behaviour and failing the pipeline would be unwanted.

Unless there is a way to reconfigure an upstream converter, but that might be racy. I think supporting the new pixel format, or implement a slow but generic path using pack/unpack is better. (same apply for videoscale I guess)
Comment 8 Sebastian Dröge (slomo) 2014-01-09 15:30:32 UTC
There are two things here: custom color formats and custom caps features. The former should be fixed by just implementing the support for this color format or a generic path with pack/unpack. That should get a new bug (same for videoscale).


For custom caps features, well. There's nothing you can do really. I think what would make sense in the context of playbin is to have a property on videobalance that lets it do nothing if it can't do anything. Like we did for deinterlace already.
Comment 9 Matthieu Bouron 2014-01-09 18:44:07 UTC
Created attachment 265868 [details] [review]
videobalance: add best-effort mode

Best-effort mode name is open to discussion I was not really inspired :).

More importantly, it feels like returning FALSE if the caps are unsupported in the set_info is not enough to make the pipeline fail. What do you think about it ?
Comment 10 Sebastian Dröge (slomo) 2014-01-10 08:32:15 UTC
According to the code, returning FALSE from set_info() will a) let the CAPS event fail, b) will let transform/transform_ip fail immediately. How does it not fail for you? :)
Comment 11 Sebastian Dröge (slomo) 2014-01-10 08:47:51 UTC
Review of attachment 265868 [details] [review]:

No idea about the name, I don't like this one :) Maybe instead of such things we should somehow check with the allocation query or something if downstream can do deinterlace or colorbalance and otherwise fail. Like what textoverlay does.

::: gst/videofilter/gstvideobalance.c
@@ +176,3 @@
+    GstPad *srcpad = GST_BASE_TRANSFORM_SRC_PAD (base);
+    GstCaps *srccaps = gst_pad_get_current_caps (srcpad);
+    if (srccaps) {

Why srccaps? Why not sinkcaps?

@@ +436,3 @@
+  GstStaticCaps static_caps = GST_STATIC_CAPS (VIDEOBALANCE_CAPS);
+
+  caps = gst_caps_new_empty_simple ("video/x-raw");

Maybe make these static caps too so we don't have to create a new caps instance every time. Also isn't this caps a subset of VIDEOBALANCE_CAPS anyway? Why the two intersections?

@@ +611,3 @@
           DEFAULT_PROP_SATURATION,
           GST_PARAM_CONTROLLABLE | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
+  g_object_class_install_property (gobject_class, PROP_BEST_EFFORT_MODE,

Needs a gtk-doc comment, with Since marker and explanation why this is useful
Comment 12 Matthieu Bouron 2014-01-10 12:14:18 UTC
(In reply to comment #10)
> According to the code, returning FALSE from set_info() will a) let the CAPS
> event fail, b) will let transform/transform_ip fail immediately. How does it
> not fail for you? :)

After some investigation I found out that the pipeline (playbin, vaapidecode + cluttersink) happens most of the time to be stuck (gst-launch-1.0 does not exit and do not say that it frees the pipeline). So there is something fishy somewhere but anyway returning FALSE here is enough.
Comment 13 Matthieu Bouron 2014-01-10 14:20:00 UTC
Created attachment 265924 [details] [review]
videobalance: add fallback-to-passthrough property

Updated property name (I like it better but name still open to discussion), added gtk-doc for the new property (A separate patch should come up for gtk-doc of the other properties) and simplified caps intersection.

I still use the srccaps in the patch, they might not differ from the sinkcaps (Maybe i'm missing something there).
Comment 14 Matthieu Bouron 2014-01-10 14:22:15 UTC
Created attachment 265925 [details] [review]
videobalance: add fallback-to-passthrough property

Fixes a typo.
Comment 15 Sebastian Dröge (slomo) 2014-01-10 14:24:08 UTC
(In reply to comment #13)

> I still use the srccaps in the patch, they might not differ from the sinkcaps
> (Maybe i'm missing something there).

They might not differ (they do not) but I'm not 100% sure if you always have src caps here. You definitely always have sink caps here though.
Comment 16 Matthieu Bouron 2014-01-10 14:43:34 UTC
Created attachment 265929 [details] [review]
videobalance: add fallback-to-passthrough property

Uses sink caps instead of src caps in the gst_video_balance_update_properties method.
Comment 17 Sebastian Dröge (slomo) 2014-01-14 09:06:56 UTC
Review of attachment 265929 [details] [review]:

Looks generally good but I'm not sure I really like this implementation.

Maybe need a different approach, maybe something with metas that are negotiated similar to the subtitle stuff.
Or only allow passthrough if all the properties are also on passthrough, otherwise fail if the video format is not supported.

This all is rather weird and unexpected otherwise. Same goes for deinterlace IMHO.

::: gst/videofilter/gstvideobalance.c
@@ +614,3 @@
+  * when videobalance is auto-inserted by playbin after an hardware decoder.
+  * In this mode, changing any of the constrat, brightness and saturation
+  * properties won't have any effects.

Since: 1.4

@@ +830,3 @@
       break;
+    case PROP_FALLBACK_TO_PASSTHROUGH:
+      d = g_value_get_boolean (value);

Isn't d a double? ;)
Comment 18 Sebastian Dröge (slomo) 2014-03-02 13:55:35 UTC
What's the plan here now? Not doing this and instead relying on playbin to only plug this if the sink does not support colorbalance itself?
Comment 19 Sebastian Dröge (slomo) 2014-08-13 10:26:04 UTC
This here is better than nothing for now:

commit 0911307d7d1150e1d01bfc8d41707dcfdc1000ab
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Aug 13 13:23:10 2014 +0300

    videobalance: Allow any raw caps in passthrough mode, not just the ones we handle

commit a9eda81978f1f956c258bccde10df42c45075c0d
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Aug 13 13:04:21 2014 +0300

    videobalance: Allow ANY capsfeatures, but only in passthrough mode
    
    When changing the properties to not be in passthrough mode anymore,
    we will only accept caps we can process ourselves, potentially causing
    a not-negotiated error.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720345
Comment 20 GStreamer system administrator 2018-11-03 14:50:46 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/100.