[lkml]   [2017]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 1/6] staging: Import the BCM2835 MMAL-based V4L2 camera driver.
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
>>>> 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_,
>>>> 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.
>> "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:

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!



 \ /
  Last update: 2017-02-10 10:48    [W:0.097 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site