GNOME Bugzilla – Bug 748141
videoconvert, glcolorconvert: keep colorimetry/chroma-site fields if passthrough
Last modified: 2015-04-27 09:21:35 UTC
Created attachment 301927 [details] [review] glcolorconvert: Keep colorimetry/chroma-site fields if passthrough Hello, Here is a proposal to improve the way glcolorconvert and videoconvert deal with caps and caps negotiation. What currently happens is that both elements currently drop the colorimetry/chroma-site fields in all cases. It prevents them to operate in passthrough mode if upstream caps contains those fields (the basetransform class checks if incaps == outcaps). My proposal would be to: 1) the transform_caps method should not just drop those fields and replace the format field with the supported formats but keep the input caps which contains the colorimetry/chroma-site fields and which have compatible formats + input caps with the colorimetry/chroma-site fields dropped and format replaced by the supported format. Example: video/x-raw,format={RGBA, DUMMY},colorimetry=sRGB,width=1920 would become video/x-raw,format=RGBA,colorimetry=sRGB,width=1920;video/x-raw,format={ RGB, RGBx, RGBA, BGR, BGRx, ...},width=1920 2) As the caps can contains those fields on both side, we need to make sure that if a conversion happens the fields are removed. Example: We can't have incaps = video/x-raw,format=RGBA,colorimetry=sRGB and outcaps = video/x-raw,format=NV12,colorimetry=sRGB After the caps are fixed in fixate_caps and in the GST_PAD_SINK direction, if incaps != outcaps, we remove the fields. I've attached a patch that implement this behaviour in glcolorconvert (I think the "passthrough" caps computation might be improved or simplified somehow). If this improvements is judged correct I will also send a patch for the videoconvert element. Comments welcome :)
Just make sure that the fields are always there in GstBaseTransform::fixate_caps() and always remove the fields in transform_caps() :) Then it should already do the right thing
Created attachment 301971 [details] [review] glcolorconvert: Keep colorimetry/chroma-site fields if passthrough
Review of attachment 301971 [details] [review]: ::: ext/gl/gstglcolorconvertelement.c @@ +233,3 @@ + gst_structure_has_field (in_structure, "chroma-site")) && + gst_caps_is_subset (ret, caps)) { + gst_caps_replace (&ret, caps); Why only if it's a subset and those fields exist in the input? Why not always when it's a subset? Or alternatively, why not always copy over the fields whenever the input has them but the output doesn't?
(In reply to Sebastian Dröge (slomo) from comment #3) > Review of attachment 301971 [details] [review] [review]: > > ::: ext/gl/gstglcolorconvertelement.c > @@ +233,3 @@ > + gst_structure_has_field (in_structure, "chroma-site")) && > + gst_caps_is_subset (ret, caps)) { > + gst_caps_replace (&ret, caps); > > Why only if it's a subset and those fields exist in the input? Why not > always when it's a subset? My intent was to avoid the call to gst_caps_replace if the input does no have those fields. But I can remove the two checks, if you prefer. > > > Or alternatively, why not always copy over the fields whenever the input has > them but the output doesn't? I'm not sure if it's really correct to preserve those fields in all the cases. For example those fields will be "consumed" by the convert element when doing a yuv->rgb conversion (bt601, bt709 colorimetry setting will help to pick the right conversion path for example). Though it might be correct to preserve the fields when doing rgb*->rgb*, yuv*->yuv*, grey*->grey* conversions.
That's actually a good point. Maybe we should give the fixate method some actual knowledge about what these caps fields mean then, and let it do the right thing depending on the format :)
Created attachment 301978 [details] [review] glcolorconvert: Keep colorimetry/chroma-site fields if passthrough Correct usage of gst_caps_is_subset and remove checks for colorimetry and chroma-site fields.
commit 76b2cefd2d916aba1fb70a3ca221adec844aa7f6 Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Sun Apr 19 19:16:55 2015 +0200 glcolorconvert: Keep colorimetry and chroma-site fields if passthrough https://bugzilla.gnome.org/show_bug.cgi?id=748141 You said videoconvert is also affected? Want to provide a patch?
Created attachment 302417 [details] [review] videoconvert: Keep colorimetry/chroma-site fields if passthrough
Comment on attachment 302417 [details] [review] videoconvert: Keep colorimetry/chroma-site fields if passthrough commit 9dfe40d7402a08eb5bbc1b2568fa48dd9a7b1f9e Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Mon Apr 27 11:06:58 2015 +0200 videoconvert: Keep colorimetry and chroma-site fields if passthrough https://bugzilla.gnome.org/show_bug.cgi?id=748141