GNOME Bugzilla – Bug 720345
videobalance: handle unsupported caps features in passthrough mode
Last modified: 2018-11-03 14:50:46 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.
Created attachment 264098 [details] [review] videobalance: handle unsupported caps features in passthrough mode
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
Matthieu?
Created attachment 265428 [details] [review] videobalance: handle unsupported caps features in passthrough mode
(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
(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.
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)
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.
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 ?
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? :)
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
(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.
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).
Created attachment 265925 [details] [review] videobalance: add fallback-to-passthrough property Fixes a typo.
(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.
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.
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? ;)
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?
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
-- 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.