GNOME Bugzilla – Bug 725248
videobox fails to negotiate in an environment with possible RGB input.
Last modified: 2014-06-17 12:47:59 UTC
I have an element that only outputs a small range of RGB formats. I want to videoconvert to a YUV format, clip the results and output the final video to an xvimagesink. However, I find that in this scenario, videobox fails to negotiate a format it can work with, and ends up rejecting all caps, causing the pipeline to fail. Here is the smallest reproduction of the error I've been able to create: gst-launch-1.0 videotestsrc ! "video/x-raw,format={BGRx,BGRA,RGB,RGB16,RGB15}" ! videoconvert ! videobox ! xvimagesink display=:0 I have this issue both with the 1.2 version of GStreamer I'm developing on, and the latest 1.3.x version in git.
Could reproduce that.
Created attachment 278262 [details] [review] Fix for the capnego problem Looks like the issue is in gst_video_box_transform_caps(). "AYUV" is treated as rgb. if (strstr (str, "RGB") || strstr (str, "BGR") || strcmp (str, "AYUV") == 0) /* <== problem could here */ seen_rgb = TRUE; else if (strcmp (str, "I420") == 0 || strcmp (str, "YV12") == 0 || strcmp (str, "AYUV") == 0) Pipeline worked with attached patch
Review of attachment 278262 [details] [review]: Thanks for spotting the issue, it is indeed related to the AYUV format as videobox can convert from both AYUV to other YUV and also to RGB formats. Unfortunately your patch introduces a regression, the pipeline: "gst-launch-1.0 videotestsrc ! "video/x-raw,format={AYUV}" ! videoconvert ! videobox ! "video/x- raw,format={RGBA}" ! videoconvert ! xvimagesink" won't work anymore as videobox now claims it can't convert AYUV -> RGBA while it can (and could before your patch). I guess we need to handle AYUV differently as it means that videobox can produce both RGB and YUV formats. We need to handle AYUV separately on that if to set both seen_rgb and seen_yuv to true. (In the loop case you can even break the loop after this case as both seen_yuv/rgb are set anyway) ::: gst/videobox/gstvideobox.c @@ -2877,1 @@ seen_rgb = TRUE; This same issue appears another time when iterating the list just above this occurence
Created attachment 278525 [details] [review] Corrected patch Thanks for pointing out the issue. I have corrected the patch. Please review.
Review of attachment 278525 [details] [review]: This still doesn't fix it. To test you should make sure that all the following pipelines still work: gst-launch-1.0 videotestsrc ! "video/x-raw,format={X}" ! videobox ! "video/x-raw,format={Y}" ! videoconvert ! xvimagesink replace X and Y with: RGBA : AYUV AYUV : RGBA I420 : AYUV AYUV : I420 AYUV : AYUV ::: gst/videobox/gstvideobox.c @@ +2863,3 @@ lval = gst_value_list_get_value (fval, j); if ((str = g_value_get_string (lval))) { + if (strstr (str, "RGB") || strstr (str, "BGR")) for AYUV you need to set both "seen_rgb" and "seen_yuv" to true as with AYUV input videobox can convert to both RGB and YUV formats. The variable names "seen_yuv" and "seen_rgb" are a bit misleading as it is not representing what it has 'seen' anymore but what it can produce. So when you set seen_rgb to true it actually means that videobox has seen a format that it can use to produce RGB output, the same applies for seen_yuv. As AYUV can be used for producing both rgb and yuv, it needs to set both variables to true. Something like: if (ayuv) { seen_rgb=true, seen_yuv=true } else if (other rgb) { seen_rgb=true } else if (other yuv) { seen_yuv=true } @@ +2872,3 @@ } else if (fval && G_VALUE_HOLDS_STRING (fval)) { if ((str = g_value_get_string (fval))) { + if (strstr (str, "RGB") || strstr (str, "BGR")) Same here. @@ -2888,3 @@ - g_value_set_string (&val, "AYUV"); - gst_value_list_append_value (&list, &val); - g_value_unset (&val); This is not correct. AYUV should be kept being set for both seen_rgb and seen_yuv as it can be converted from both format sets. If you look at the comment a bit above in the code you will see: /* Supported conversions: * I420->AYUV * I420->YV12 * YV12->AYUV * YV12->I420 * AYUV->I420 * AYUV->YV12 * AYUV->xRGB (24bpp, 32bpp, incl. alpha) * xRGB->xRGB (24bpp, 32bpp, from/to all variants, incl. alpha) * xRGB->AYUV (24bpp, 32bpp, incl. alpha) * * Passthrough only for everything else. */ As you can see, AYUV can be converted from the RGB formats and to RGB formats as well as for another YUV formats.
Created attachment 278573 [details] [review] Capsnego fix Corrected per your suggestion. It works for all supported conversions now.
Comment on attachment 278573 [details] [review] Capsnego fix Commited this patch with a style fix for using {} on all of those if/else if expressions as using on some and not on others made it harder to read IMHO. Also added a better commit message.
commit 3c4c130c5e11524b0ea7a849ec1dd213741e93cf Author: Ravi Kiran K N <ravi.kiran@samsung.com> Date: Tue Jun 17 13:16:27 2014 +0530 videobox: Fix caps negotiation issue Make sure that if AYUV is received it will detect that it can produce both RGB and YUV formats Signed-off-by: Ravi Kiran K N <ravi.kiran@samsung.com> https://bugzilla.gnome.org/show_bug.cgi?id=725248 and pushed for 1.2 branch: commit daf888a6d80af3d64c5618470e2b12111b7657e3 Author: Ravi Kiran K N <ravi.kiran@samsung.com> Date: Tue Jun 17 13:16:27 2014 +0530 videobox: Fix caps negotiation issue Make sure that if AYUV is received it will detect that it can produce both RGB and YUV formats Signed-off-by: Ravi Kiran K N <ravi.kiran@samsung.com> https://bugzilla.gnome.org/show_bug.cgi?id=725248