GNOME Bugzilla – Bug 762529
v4l2src produces wrong colorimetry info for NV12
Last modified: 2016-03-17 13:14:01 UTC
When video is captured in NV12 and later converted by videoconvert into YV12 black turns gray. To reproduce the issue run: sudo modprobe vivid gst-launch-1.0 v4l2src ! video/x-raw, format=NV12 ! videoconvert ! video/x-raw, format=YV12 ! xvimagesink Also reproducible with real hardware. gst_v4l2_object_get_colorspace[1] is called with V4L2_COLORSPACE_SRGB, V4L2_QUANTIZATION_DEFAULT, V4L2_YCBCR_ENC_DEFAULT, V4L2_XFER_FUNC_DEFAULT which leads to full range colorimetry when it should be limited range to get proper colors. Seems to be a discrepancy at the documentation, the following is the info I've got at #v4l channel. {{{ ad-n770: 13:16 I'm looking into one issue in the gstreamer element v4l2src that seems to lead to a discrepancy in the v4l documentation 13:17 the issue is reproduced with vivid and a uvcvideo compatible hardware 13:18 when NV12 format is selected for capture the GStreamer element produces wrong colorimetry information that leads black turn to gray 13:18 we tracked down to the following facts 13:20 1) for NV12 is reported V4L2_COLORSPACE_SRGB and V4L2_QUANTIZATION_DEFAULT 13:21 2) at https://linuxtv.org/downloads/v4l-dvb-apis/ch02s06.html#col-srgb it's said "The default Y'CbCr quantization is full range." 13:22 3) at https://linuxtv.org/downloads/v4l-dvb-apis/ch02s05.html bottom note for V4L2_QUANTIZATION_DEFAULT it's said "V4L2_QUANTIZATION_DEFAULT Use the default quantization encoding as defined by the colorspace. This is always full range for R'G'B' (except for the BT.2020 colorspace) and usually limited range for Y'CbCr." 13:24 seems that the issue in the colors is due the fact that we followed the rule of full range because V4L2_COLORSPACE_SRGB 13:25 but we are not sure if it's either correct for NV12 reporting as SRGB or if we should follow the note in V4L2_QUANTIZATION_DEFAULT "usually limited range for Y'CbCr." 13:26 please could you give us some light on this? hverkuil: 13:27 ad-n770: I believe that the ENC_SYCC documentation is wrong. I think it is actually the same as ENC_601 and that it is limited range instead of full range. 13:29 What is confusing is that SYCC is the same as BT601 within the limited range, but allows values above and below that range. 13:31 It is best to follow the V4L2_MAP_QUANTIZATION_DEFAULT() macro from videodev2.h: that does the right thing to the best of my knowledge. 13:32 The spec regarding SYCC is confusing. I plan on testing this with a signal generator to double check this. 13:32 In the meantime I'll prepare a patch to bring the spec in line with the V4L2_MAP_QUANTIZATION_DEFAULT macro. }}} [1] https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/sys/v4l2/gstv4l2object.c#n1879
I've been pointed at #v4l that we should use the macros like in the following example. http://git.linuxtv.org/v4l-utils.git/tree/utils/qv4l2/capture-win-gl.cpp#n218
Created attachment 321973 [details] [review] Fixes an issue in our outdated copy of this file Maybe we should update the whole file instead
Created attachment 321976 [details] [review] Use V4L2 macros to deal with default values on colorimetry related attributes This is the actual fix for the NV12 issue.
Review of attachment 321973 [details] [review]: This is an upstream bug, please make a full update to a known kernel commit/release (documented in the comment). We don't maintain any custom changes to this file, other then the one requires to make it work standalone.
Review of attachment 321976 [details] [review]: ::: sys/v4l2/gstv4l2object.c @@ +1888,3 @@ + matrix = V4L2_MAP_YCBCR_ENC_DEFAULT (colorspace); + if (range == V4L2_QUANTIZATION_DEFAULT) + range = V4L2_MAP_QUANTIZATION_DEFAULT (is_rgb, colorspace, matrix); This makes the following switch completely use-less. Please remove the uneeded code. Though, keep the _RAW case, since we have in GStreamer an explicit value for "unknown", while those macro would pick random value which would then cause confusion and errors (the macro are wrong for COLORSPACE_RAW).
Hope that Hans will fix the doc properly, as what the macro do is pretty far from what the doc says.
Created attachment 322904 [details] [review] Import external headers from latest stable kernel As per review comments this new patch syncs with latest stable kernel 4.4.3.
Created attachment 322908 [details] [review] Use V4L2 macros to deal with default values on colorimetry related attributes Cleaned switch as per review request. But I'm not sure if for V4L2_COLORSPACE_RAW we should directly return in the case. Also the value V4L2_COLORSPACE_DEFAULT isn't handled in the switch case explicitly which might be wrong. In the header there's the following macro and I'm not sure if we should use it or replicate the behaviour. /* * Determine how COLORSPACE_DEFAULT should map to a proper colorspace. * This depends on whether this is a SDTV image (use SMPTE 170M), an * HDTV image (use Rec. 709), or something else (use sRGB). */ #define V4L2_MAP_COLORSPACE_DEFAULT(is_sdtv, is_hdtv) \ ((is_sdtv) ? V4L2_COLORSPACE_SMPTE170M : \ ((is_hdtv) ? V4L2_COLORSPACE_REC709 : V4L2_COLORSPACE_SRGB)) This logic seems a bit wrong to me, what happens with UDTV ?
Nicolas, what should we do about this?
(In reply to Josep Torra Valles from comment #8) > Created attachment 322908 [details] [review] [review] > Use V4L2 macros to deal with default values on colorimetry related attributes > > Cleaned switch as per review request. > > But I'm not sure if for V4L2_COLORSPACE_RAW we should directly return in the > case. RAW is special case, as we have an explicit representation for that in GStreamer. While in V4L2 macros, it simply guesses something. > > Also the value V4L2_COLORSPACE_DEFAULT isn't handled in the switch case > explicitly which might be wrong. In the header there's the following macro > and I'm not sure if we should use it or replicate the behaviour. The behaviour is to not set anything in the caps, so GStreamer will do a guess base on the color format and resolution. This guess is well tested. > > /* > * Determine how COLORSPACE_DEFAULT should map to a proper colorspace. > * This depends on whether this is a SDTV image (use SMPTE 170M), an > * HDTV image (use Rec. 709), or something else (use sRGB). > */ > #define V4L2_MAP_COLORSPACE_DEFAULT(is_sdtv, is_hdtv) \ > ((is_sdtv) ? V4L2_COLORSPACE_SMPTE170M : \ > ((is_hdtv) ? V4L2_COLORSPACE_REC709 : V4L2_COLORSPACE_SRGB)) > > This logic seems a bit wrong to me, what happens with UDTV ? I do believe that UDTV should result with is_hdtv to be set, but you'd have to ask Hans. Colorwhise, I doubt there is a difference with HD and UD. Maybe we should have just updated the switch cases to match the macro behaviour in the end, with the missing hints. At least it made it simple to understand what will be the result. The macros makes things a bit obscure in my opinion (sorry for changing my mind). Sebastian, I'd like to get this one right, so I'm not in a rush, if we get that for 1.8.1, I'm fine.
(In reply to Nicolas Dufresne (stormer) from comment #10) > Colorwhise, I doubt there is a difference with HD and UD The color matrix is the same, the only difference is the gamma curve which is also only minimally different.
Let's consider it a blocker for now and see what we can do here.
> > But I'm not sure if for V4L2_COLORSPACE_RAW we should directly return in the > > case. > > RAW is special case, as we have an explicit representation for that in > GStreamer. While in V4L2 macros, it simply guesses something. Sorry probably I didn't explain my concern properly. At https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/sys/v4l2/gstv4l2object.c#n1942 I've the feeling that it should just bail out with return TRUE instead of continue with the other switches.
(In reply to Josep Torra Valles from comment #13) > > > But I'm not sure if for V4L2_COLORSPACE_RAW we should directly return in the > > > case. > > > > RAW is special case, as we have an explicit representation for that in > > GStreamer. While in V4L2 macros, it simply guesses something. > > Sorry probably I didn't explain my concern properly. > > At > https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/sys/v4l2/ > gstv4l2object.c#n1942 > > I've the feeling that it should just bail out with return TRUE instead of > continue with the other switches. I believe the spec is empty around this one. If you set _RAW and default everywhere else, that's exactly what you will get. The question is if a driver can set colorspace RAW (to avoid randomly choosen defaults) and set few of the parameters still. That was at least my interpretation. We yet to have to find such a HW, most of them provides an explicit colorspace.
Created attachment 323093 [details] [review] Fixes colorimetry for NV12 New version of the fix as per review comments.
Review of attachment 323093 [details] [review]: Yep, I like this one, this way we keep control.
Btw, if someone want to test, open an app with a fully back blackground. Using any USB webcam, before the patch, if you block all lights the window is slightly gray using the following pipeline, and is fully black with this patch: gst-lauch-1.0 v4l2src ! videoconvert ! ximagesink Note that with glimagesink it makes no difference, as there is absolutly no colorimetry support yet in libgstgl.
commit c65b66432e8ee8480a84a9d4a0fd6cb6447c335d Author: Josep Torra <n770galaxy@gmail.com> Date: Fri Mar 4 15:09:45 2016 +0100 v4l2: fix colorimetry for NV12 Replicate V4L2_MAP_QUANTIZATION_DEFAULT macro behavior. At #v4l it was described that documentation might be wrong and that we should trust this macro instead. https://bugzilla.gnome.org/show_bug.cgi?id=762529