GNOME Bugzilla – Bug 754465
compositor: Negotiation failure with ARGB64
Last modified: 2015-09-11 21:15:24 UTC
When launching a stream that uses ARGB64, compositor tries to work with it though it does not support it. The problem is that in acceptcaps, the compositor conciders it supports all format (removing the format field from the template caps structure). With the following pipeline: $ gst-launch-1.0 uridecodebin uri=https://people.collabora.com/~tsaunier/samples/tmph72h4ynj.png ! imagefreeze ! compositor ! autovideosink We end up having the following error on the bus: Setting pipeline to PAUSED ... Pipeline is PREROLLING ... ERROR: from element /GstPipeline:pipeline0/GstCompositor:compositor0: At least one of the input pads contains alpha, but downstream can't support alpha. Additional debug info: gstvideoaggregator.c(559): gst_videoaggregator_update_converters (): /GstPipeline:pipeline0/GstCompositor:compositor0: Either convert your inputs to not contain alpha or add a videoconvert after the aggregator ERROR: pipeline doesn't want to preroll. Setting pipeline to NULL ... Freeing pipeline ...
Ah, and gst-launch-1.0 uridecodebin uri=https://people.collabora.com/~tsaunier/samples/tmph72h4ynj.png ! imagefreeze ! compositor ! videoconvert ! autovideosink failes the same way (which is what that bug is about)
I just bisected it and the culpright is: commit 1248b00c80aff2f24ce094a89ec58ce5c172afb8 Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Sat Jun 20 13:36:27 2015 +0200 videoaggregator: simplifies and improves sink_get_caps. The problem here was that after removing the formats and all the things we could convert, we then intersected these caps with the template caps. Hence if a subclass offered permissive sink templates (eg all the possible formats videoconvert handles), but only one output format, then at negotiation time getcaps returned caps with the format restricted to that format, even though we do handle conversion. https://bugzilla.gnome.org/show_bug.cgi?id=751255
Instead of just reverting this commit, we should understand why it fails first. Maybe it is exposing a bug that would otherwise also happen but less often.
There seems to be a collection of small things to fix here. Let's start with a simple one. The formats that compositor can handle: " { AYUV, BGRA, ARGB, RGBA, ABGR, Y444, Y42B, YUY2, UYVY, "\ " YVYU, I420, YV12, NV12, NV21, Y41B, RGB, BGR, xRGB, xBGR, "\ " RGBx, BGRx } " The input on this sample is ARGB64, so it should fail right away as it seems not to be supported. I guess we want to start by extending the compositor list to everything in the gstvideo lib? Or is there any reason for the smaller set of formats?
Created attachment 311026 [details] [review] videoaggregator: fix caps query to properly handle alpha formats Improves the situation by only reporting alpha in caps queries if downstream has it as well
(In reply to Thiago Sousa Santos from comment #4) > I guess we want to start by extending the compositor > list to everything in the gstvideo lib? Or is there any reason for the > smaller set of formats? We don't support blending for every format from the gstvideo lib, so some more negotiation work is needed here then. All the formats listed currently are supported for blending, and we chose any of the input formats for blending. So if we got an unsupported format, we would need to select a blending format somehow and convert with GstVideoConverter.
Review of attachment 311026 [details] [review]: Seems to make sense but ... ::: gst-libs/gst/video/gstvideoaggregator.c @@ +844,3 @@ + "GRAY16_LE, v308, RGB16, BGR16, RGB15, BGR15, UYVP, RGB8P, YUV9, YVU9, " + "IYU1, r210, I420_10LE, I420_10BE, I422_10LE, I422_10BE, " + " Y444_10LE, Y444_10BE, GBR, GBR_10LE, GBR_10BE, NV12_64Z32 }"); I don't like these two hardcoded lists... can't we just generate those in class_init() for GstVideoFormatInfos?
(In reply to Sebastian Dröge (slomo) from comment #7) > Review of attachment 311026 [details] [review] [review]: > > Seems to make sense but ... > > ::: gst-libs/gst/video/gstvideoaggregator.c > @@ +844,3 @@ > + "GRAY16_LE, v308, RGB16, BGR16, RGB15, BGR15, UYVP, RGB8P, YUV9, YVU9, " > + "IYU1, r210, I420_10LE, I420_10BE, I422_10LE, I422_10BE, " > + " Y444_10LE, Y444_10BE, GBR, GBR_10LE, GBR_10BE, NV12_64Z32 }"); > > I don't like these two hardcoded lists... can't we just generate those in > class_init() for GstVideoFormatInfos? Yes, I'm already working on it because videoaggregator actually needs to derive those from the subclass supported formats. So I'll keep a caps_alpha and caps_non_alpha around for the element.
Created attachment 311174 [details] [review] videoaggregator: fix caps query to properly handle alpha formats Only accept alpha if downstream has alpha as well. It could theoretically accept alpha unconditionally if blending is properly implemented for handle it but at the moment this is a missing feature. Improves the caps query by also comparing with the template caps to filter by what the subclass supports.
Review of attachment 311174 [details] [review]: Generally looks good :) ::: gst-libs/gst/video/gstvideoaggregator.c @@ +2075,3 @@ klass->get_output_buffer = gst_videoaggregator_get_output_buffer; + g_mutex_init (&klass->sink_caps_mutex); This is not necessarily a good idea... each subclass will have the mutex memcpy'd into their own class struct, g_mutex_lock() and friends might in theory do something with the memory location of the mutex. Better make this a global mutex, not one stored in the class struct @@ +2167,3 @@ + g_mutex_lock (&klass->sink_caps_mutex); + if (klass->sink_non_alpha_caps == NULL) { + klass->sink_non_alpha_caps = _get_non_alpha_caps_from_template (klass); Why not a GOnce in the class struct? But sure, this also works :)
(In reply to Sebastian Dröge (slomo) from comment #10) > Review of attachment 311174 [details] [review] [review]: > > Generally looks good :) > > ::: gst-libs/gst/video/gstvideoaggregator.c > @@ +2075,3 @@ > klass->get_output_buffer = gst_videoaggregator_get_output_buffer; > > + g_mutex_init (&klass->sink_caps_mutex); > > This is not necessarily a good idea... each subclass will have the mutex > memcpy'd into their own class struct, g_mutex_lock() and friends might in > theory do something with the memory location of the mutex. > > Better make this a global mutex, not one stored in the class struct > > @@ +2167,3 @@ > + g_mutex_lock (&klass->sink_caps_mutex); > + if (klass->sink_non_alpha_caps == NULL) { > + klass->sink_non_alpha_caps = _get_non_alpha_caps_from_template (klass); > > Why not a GOnce in the class struct? But sure, this also works :) Is a GOnce initialization possible inside a struct? I just saw there was some macro to initialize it, didn't check if it was 0 by default. I'll check and if that works I'll change it.
Created attachment 311181 [details] [review] videoaggregator: fix caps query to properly handle alpha formats Updated according to comments, now using a static global mutex to protect the sink caps generation
commit ba97ec74bedd3d8e910052d8345becc42ad75813 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Wed Sep 9 19:51:18 2015 -0300 videoaggregator: fix caps query to properly handle alpha formats Only accept alpha if downstream has alpha as well. It could theoretically accept alpha unconditionally if blending is properly implemented for handle it but at the moment this is a missing feature. Improves the caps query by also comparing with the template caps to filter by what the subclass supports. https://bugzilla.gnome.org/show_bug.cgi?id=754465