After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762529 - v4l2src produces wrong colorimetry info for NV12
v4l2src produces wrong colorimetry info for NV12
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 1.7.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-02-23 12:52 UTC by Josep Torra Valles
Modified: 2016-03-17 13:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fixes an issue in our outdated copy of this file (1.08 KB, patch)
2016-02-23 14:21 UTC, Josep Torra Valles
none Details | Review
Use V4L2 macros to deal with default values on colorimetry related attributes (2.53 KB, patch)
2016-02-23 14:23 UTC, Josep Torra Valles
none Details | Review
Import external headers from latest stable kernel (6.53 KB, patch)
2016-03-02 18:47 UTC, Josep Torra Valles
none Details | Review
Use V4L2 macros to deal with default values on colorimetry related attributes (4.68 KB, patch)
2016-03-02 18:58 UTC, Josep Torra Valles
none Details | Review
Fixes colorimetry for NV12 (2.83 KB, patch)
2016-03-04 14:21 UTC, Josep Torra Valles
committed Details | Review

Description Josep Torra Valles 2016-02-23 12:52:07 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
Comment 1 Josep Torra Valles 2016-02-23 14:18:43 UTC
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
Comment 2 Josep Torra Valles 2016-02-23 14:21:41 UTC
Created attachment 321973 [details] [review]
Fixes an issue in our outdated copy of this file

Maybe we should update the whole file instead
Comment 3 Josep Torra Valles 2016-02-23 14:23:30 UTC
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.
Comment 4 Nicolas Dufresne (ndufresne) 2016-02-24 01:03:55 UTC
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.
Comment 5 Nicolas Dufresne (ndufresne) 2016-02-24 01:18:32 UTC
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).
Comment 6 Nicolas Dufresne (ndufresne) 2016-02-24 01:19:51 UTC
Hope that Hans will fix the doc properly, as what the macro do is pretty far from what the doc says.
Comment 7 Josep Torra Valles 2016-03-02 18:47:43 UTC
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.
Comment 8 Josep Torra Valles 2016-03-02 18:58:01 UTC
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 ?
Comment 9 Sebastian Dröge (slomo) 2016-03-02 22:33:05 UTC
Nicolas, what should we do about this?
Comment 10 Nicolas Dufresne (ndufresne) 2016-03-03 02:28:25 UTC
(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.
Comment 11 Sebastian Dröge (slomo) 2016-03-03 07:44:39 UTC
(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.
Comment 12 Sebastian Dröge (slomo) 2016-03-03 08:18:12 UTC
Let's consider it a blocker for now and see what we can do here.
Comment 13 Josep Torra Valles 2016-03-03 11:49:20 UTC
> > 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.
Comment 14 Nicolas Dufresne (ndufresne) 2016-03-03 13:24:08 UTC
(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.
Comment 15 Josep Torra Valles 2016-03-04 14:21:42 UTC
Created attachment 323093 [details] [review]
Fixes colorimetry for NV12

New version of the fix as per review comments.
Comment 16 Nicolas Dufresne (ndufresne) 2016-03-07 16:34:06 UTC
Review of attachment 323093 [details] [review]:

Yep, I like this one, this way we keep control.
Comment 17 Nicolas Dufresne (ndufresne) 2016-03-07 16:47:21 UTC
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.
Comment 18 Josep Torra Valles 2016-03-17 13:14:01 UTC
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