On 02/06/2017 05:00 PM, Hans Verkuil wrote: > On 02/06/2017 04:21 PM, Dave Stevenson wrote: >> Hi Hans. >> >> On 06/02/17 12:58, Hans Verkuil wrote: >>> On 02/06/2017 12:37 PM, Dave Stevenson wrote: >>>> Hi Hans. >>>> >>>> On 06/02/17 09:08, Hans Verkuil wrote: >>>>> Hi Eric, >>>>> >>>>> Great to see this driver appearing for upstream merging! >>>>> >>>>> See below for my review comments, focusing mostly on V4L2 specifics. >>>>> >> >> <snip> >> >>>>>> + f->fmt.pix.pixelformat = dev->capture.fmt->fourcc; >>>>>> + f->fmt.pix.bytesperline = dev->capture.stride; >>>>>> + f->fmt.pix.sizeimage = dev->capture.buffersize; >>>>>> + >>>>>> + if (dev->capture.fmt->fourcc == V4L2_PIX_FMT_RGB24) >>>>>> + f->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; >>>>>> + else if (dev->capture.fmt->fourcc == V4L2_PIX_FMT_JPEG) >>>>>> + f->fmt.pix.colorspace = V4L2_COLORSPACE_JPEG; >>>>>> + else >>>>>> + f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; >>>>> >>>>> Colorspace has nothing to do with the pixel format. It should come from the >>>>> sensor/video receiver. >>>>> >>>>> If this information is not available, then COLORSPACE_SRGB is generally a >>>>> good fallback. >>>> >>>> I would if I could, but then I fail v4l2-compliance on V4L2_PIX_FMT_JPEG >>>> https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-formats.cpp#n329 >>>> The special case for JPEG therefore has to remain. >>> >>> Correct. Sorry, my fault, I forgot about that. >>> >>>> >>>> It looks like I tripped over the subtlety between V4L2_COLORSPACE_, >>>> V4L2_XFER_FUNC_, V4L2_YCBCR_ENC_, and V4L2_QUANTIZATION_, and Y'CbCr >>>> encoding vs colourspace. >>>> >>>> The ISP coefficients are set up for BT601 limited range, and any >>>> conversion back to RGB is done based on that. That seemed to fit >>>> SMPTE170M rather than SRGB. >>> >>> Colorspace refers to the primary colors + whitepoint that are used to >>> create the colors (basically this answers the question to which colors >>> R, G and B exactly refer to). The SMPTE170M has different primaries >>> compared to sRGB (and a different default transfer function as well). >>> >>> RGB vs Y'CbCr is just an encoding and it doesn't change the underlying >>> colorspace. Unfortunately, the term 'colorspace' is often abused to just >>> refer to RGB vs Y'CbCr. >>> >>> If the colorspace is SRGB, then when the pixelformat is a Y'CbCr encoding, >>> then the BT601 limited range encoding is implied, unless overridden via >>> the ycbcr_enc and/or quantization fields in struct v4l2_pix_format. >>> >>> In other words, this does already the right thing. >> >> https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/pixfmt-007.html#colorspace-srgb-v4l2-colorspace-srgb >> "The default transfer function is V4L2_XFER_FUNC_SRGB. The default >> Y’CbCr encoding is V4L2_YCBCR_ENC_601. The default Y’CbCr quantization >> is full range." >> So full range or limited? > > Ah, good catch. The default range for SRGB is full range, so the documentation > is correct. This is according to the sYCC standard. > > This means that you need to set the quantization field to limited range in this driver. > > Sorry for the confusion I caused. > > Interesting, I should take a look at other drivers since I suspect that this is > signaled wrong elsewhere as well. It used to be limited range but I changed it > to full range (as per the sYCC spec). But in practice it is limited range in most > cases. > > I'll take another look at this on Friday. > > I recommend that you leave the code as is for now. I posted a patch to correct the default quantization range mapping for sRGB and AdobeRGB: https://patchwork.linuxtv.org/patch/39306/ So even though the standards say full range, this would break backwards compatibility with all current kernel drivers since they all produce limited range when converting sRGB or AdobeRGB to a Y'CbCr format. Thanks for bringing this to my attention! Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
