Re: [PATCH v9 11/11] media: i2c: ov7670: Fully set mbus frame fmt
Hi Laurent,
On Thu, Feb 22, 2018 at 02:47:06PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Thursday, 22 February 2018 14:36:00 EET jacopo mondi wrote:
> > On Thu, Feb 22, 2018 at 02:14:53PM +0200, Laurent Pinchart wrote:
> > > On Thursday, 22 February 2018 14:04:12 EET jacopo mondi wrote:
> > >> On Wed, Feb 21, 2018 at 10:28:06PM +0200, Laurent Pinchart wrote:
> > >>> On Tuesday, 20 February 2018 10:58:57 EET jacopo mondi wrote:
>
> [snip]
>
> > This actually makes me wonder if those informations (ycbcb_enc,
> > quantization and xfer_func) shouldn't actually be part of the
> > supported format list... I blindly added those default fields in the
> > try_fmt function, but I doubt they applies to all supported formats.
> >
> > Eg. the sensor supports YUYV as well as 2 RGB encodings (RGB444 and
> > RGB 565) and 1 raw format (BGGR). I now have a question here:
> >
> > 1) ycbcr_enc transforms non-linear R'G'B' to Y'CbCr: does this
> > applies to RGB and raw formats? I don't think so, and what value is
> > the correct one for the ycbcr_enc field in this case? I assume
> > xfer_func and quantization applies to all formats instead..
> > >>>
> > >>> There's no encoding for RGB formats if I understand things correctly,
> > >>> so I'd set ycbcr_enc to V4L2_YCBCR_ENC_DEFAULT. The transfer function
> > >>> and the quantization apply to all formats, but I'd be surprised to find
> > >>> a sensor outputting limited range for RGB.
> > >>
> > >> Ack, we got the same understanding for RGB formats. I wonder if for
> > >> those formats we wouldn't need a V4L2_YCBCR_ENC_NONE or similar...
> > >
> > > That, or explicitly documenting that when the format is not YUV the field
> > > should be set by both drivers and applications to V4L2_YCBCR_ENC_DEFAULT
> > > when written and ignored when read.
> >
> > Well, if no encoding is performed because the color encoding scheme is
> > RGB, the colorspace does anyway define an encoding method, so it seems
> > to me the latter is more appropriate (use DEFAULT and ignore when read).
> >
> > That's anyway just my opinion, but I could send a patch for
> > documentation and see what feedback it gets.
> >
> > >>> Have you been able to check whether the sensor outputs limited range
> > >>> of full range YUV ? If it outputs full range you can hardcode
> > >>> quantization to V4L2_QUANTIZATION_FULL_RANGE for all formats.
> > >>
> > >> In YUYV mode, I see values > 0xf0 ( > 240, which is the max value for
> > >> CbCr samples in limited quantization range), so I assume quantization
> > >> is full range.
> > >
> > > It should be, yes. What's the minimum and maximum values you get ?
> >
> > From a white surface:
> > min = 0x39
> > max = 0xfc
> >
> > From a black surface:
> > min = 0x00 (with 62 occurrences)
> > max = 0x8b
> >
> > I guess that's indeed full range quantization
>
> Could you check Y and UV separately ?
>
> #!/usr/bin/python
>
> import sys
>
> def main(argv):
> if len(argv) != 2:
> print('Usage: %s ' % argv[0])
> return 1
>
> data = open(argv[1], 'rb').read()
>
> y_min = 255
> y_max = 0
> uv_min = 255
> uv_max = 0
>
> for i in range(len(data) // 2):
> y = data[2*i]
> uv = data[2*i]
uv = data[2*i+1]
>
> y_min = min(y_min, y)
> y_max = max(y_max, y)
> uv_min = min(uv_min, uv)
> uv_max = max(uv_max, uv)
>
> print('Y [%u, %u] UV [%u, %u]' % (y_min, y_max, uv_min, uv_max))
> return 0
>
> if __name__ == '__main__':
> sys.exit(main(sys.argv))
>
White image:
Y [57, 252] UV [105, 145]
Black image:
Y [0, 32] UV [116, 139]
Re: [PATCH v9 11/11] media: i2c: ov7670: Fully set mbus frame fmt
Hi Jacopo,
On Thursday, 22 February 2018 14:36:00 EET jacopo mondi wrote:
> On Thu, Feb 22, 2018 at 02:14:53PM +0200, Laurent Pinchart wrote:
> > On Thursday, 22 February 2018 14:04:12 EET jacopo mondi wrote:
> >> On Wed, Feb 21, 2018 at 10:28:06PM +0200, Laurent Pinchart wrote:
> >>> On Tuesday, 20 February 2018 10:58:57 EET jacopo mondi wrote:
[snip]
> This actually makes me wonder if those informations (ycbcb_enc,
> quantization and xfer_func) shouldn't actually be part of the
> supported format list... I blindly added those default fields in the
> try_fmt function, but I doubt they applies to all supported formats.
>
> Eg. the sensor supports YUYV as well as 2 RGB encodings (RGB444 and
> RGB 565) and 1 raw format (BGGR). I now have a question here:
>
> 1) ycbcr_enc transforms non-linear R'G'B' to Y'CbCr: does this
> applies to RGB and raw formats? I don't think so, and what value is
> the correct one for the ycbcr_enc field in this case? I assume
> xfer_func and quantization applies to all formats instead..
> >>>
> >>> There's no encoding for RGB formats if I understand things correctly,
> >>> so I'd set ycbcr_enc to V4L2_YCBCR_ENC_DEFAULT. The transfer function
> >>> and the quantization apply to all formats, but I'd be surprised to find
> >>> a sensor outputting limited range for RGB.
> >>
> >> Ack, we got the same understanding for RGB formats. I wonder if for
> >> those formats we wouldn't need a V4L2_YCBCR_ENC_NONE or similar...
> >
> > That, or explicitly documenting that when the format is not YUV the field
> > should be set by both drivers and applications to V4L2_YCBCR_ENC_DEFAULT
> > when written and ignored when read.
>
> Well, if no encoding is performed because the color encoding scheme is
> RGB, the colorspace does anyway define an encoding method, so it seems
> to me the latter is more appropriate (use DEFAULT and ignore when read).
>
> That's anyway just my opinion, but I could send a patch for
> documentation and see what feedback it gets.
>
> >>> Have you been able to check whether the sensor outputs limited range
> >>> of full range YUV ? If it outputs full range you can hardcode
> >>> quantization to V4L2_QUANTIZATION_FULL_RANGE for all formats.
> >>
> >> In YUYV mode, I see values > 0xf0 ( > 240, which is the max value for
> >> CbCr samples in limited quantization range), so I assume quantization
> >> is full range.
> >
> > It should be, yes. What's the minimum and maximum values you get ?
>
> From a white surface:
> min = 0x39
> max = 0xfc
>
> From a black surface:
> min = 0x00 (with 62 occurrences)
> max = 0x8b
>
> I guess that's indeed full range quantization
Could you check Y and UV separately ?
#!/usr/bin/python
import sys
def main(argv):
if len(argv) != 2:
print('Usage: %s ' % argv[0])
return 1
data = open(argv[1], 'rb').read()
y_min = 255
y_max = 0
uv_min = 255
uv_max = 0
for i in range(len(data) // 2):
y = data[2*i]
uv = data[2*i]
y_min = min(y_min, y)
y_max = max(y_max, y)
uv_min = min(uv_min, uv)
uv_max = max(uv_max, uv)
print('Y [%u, %u] UV [%u, %u]' % (y_min, y_max, uv_min, uv_max))
return 0
if __name__ == '__main__':
sys.exit(main(sys.argv))
> > > Actually the hardest part here was having a white enough surface to
> > > point the sensor to :)
> >
> > Pointing a flashlight to the sensor usually does the trick.
>
> I had a yellowish light that didn't work that well, I ended up putting
> a white paper sheet in front of it and then took the picture.
Time to get a white flashlight ? :-)
--
Regards,
Laurent Pinchart
Re: [PATCH v9 11/11] media: i2c: ov7670: Fully set mbus frame fmt
Hi Laurent, On Thu, Feb 22, 2018 at 02:14:53PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Thursday, 22 February 2018 14:04:12 EET jacopo mondi wrote: > > On Wed, Feb 21, 2018 at 10:28:06PM +0200, Laurent Pinchart wrote: > > > On Tuesday, 20 February 2018 10:58:57 EET jacopo mondi wrote: > > > > On Mon, Feb 19, 2018 at 09:19:32PM +0200, Laurent Pinchart wrote: > > > > > On Monday, 19 February 2018 18:59:44 EET Jacopo Mondi wrote: > > > > >> The sensor driver sets mbus format colorspace information and sizes, > > > > >> but not ycbcr encoding, quantization and xfer function. When supplied > > > > >> with an badly initialized mbus frame format structure, those fields > > > > >> need to be set explicitly not to leave them uninitialized. This is > > > > >> tested by v4l2-compliance, which supplies a mbus format description > > > > >> structure and checks for all fields to be properly set. > > > > >> > > > > >> Without this commit, v4l2-compliance fails when testing formats with: > > > > >> fail: v4l2-test-formats.cpp(335): ycbcr_enc >= 0xff > > > > >> > > > > >> Signed-off-by: Jacopo Mondi > > > > >> --- > > > > >> > > > > >> drivers/media/i2c/ov7670.c | 4 > > > > >> 1 file changed, 4 insertions(+) > > > > >> > > > > >> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > > > > >> index 25b26d4..61c472e 100644 > > > > >> --- a/drivers/media/i2c/ov7670.c > > > > >> +++ b/drivers/media/i2c/ov7670.c > > > > >> @@ -996,6 +996,10 @@ static int ov7670_try_fmt_internal(struct > > > > >> v4l2_subdev > > > > >> *sd, fmt->height = wsize->height; > > > > >> > > > > >> fmt->colorspace = ov7670_formats[index].colorspace; > > > > > > > > > > On a side note, if I'm not mistaken the colorspace field is set to > > > > > SRGB > > > > > for all entries. Shouldn't you hardcode it here and remove the field ? > > > > > > > > > >> +fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > > > >> +fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > > > > >> +fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT; > > > > > > > > > > How about setting the values explicitly instead of relying on defaults > > > > > ? > > > > > That would be V4L2_YCBCR_ENC_601, V4L2_QUANTIZATION_LIM_RANGE and > > > > > V4L2_XFER_FUNC_SRGB. And could you then check a captured frame to see > > > > > if > > > > > the sensor outputs limited or full range ? > > > > > > > > This actually makes me wonder if those informations (ycbcb_enc, > > > > quantization and xfer_func) shouldn't actually be part of the > > > > supported format list... I blindly added those default fields in the > > > > try_fmt function, but I doubt they applies to all supported formats. > > > > > > > > Eg. the sensor supports YUYV as well as 2 RGB encodings (RGB444 and > > > > RGB 565) and 1 raw format (BGGR). I now have a question here: > > > > > > > > 1) ycbcr_enc transforms non-linear R'G'B' to Y'CbCr: does this > > > > applies to RGB and raw formats? I don't think so, and what value is > > > > the correct one for the ycbcr_enc field in this case? I assume > > > > xfer_func and quantization applies to all formats instead.. > > > > > > There's no encoding for RGB formats if I understand things correctly, so > > > I'd set ycbcr_enc to V4L2_YCBCR_ENC_DEFAULT. The transfer function and > > > the quantization apply to all formats, but I'd be surprised to find a > > > sensor outputting limited range for RGB. > > > > Ack, we got the same understanding for RGB formats. I wonder if for > > those formats we wouldn't need a V4L2_YCBCR_ENC_NONE or similar... > > That, or explicitly documenting that when the format is not YUV the field > should be set by both drivers and applications to V4L2_YCBCR_ENC_DEFAULT when > written and ignored when read. > Well, if no encoding is performed because the color encoding scheme is RGB, the colorspace does anyway define an encoding method, so it seems to me the latter is more appropriate (use DEFAULT and ignore when read). That's anyway just my opinion, but I could send a patch for documentation and see what feedback it gets. > > > Have you been able to check whether the sensor outputs limited range of > > > full range YUV ? If it outputs full range you can hardcode quantization > > > to V4L2_QUANTIZATION_FULL_RANGE for all formats. > > > > In YUYV mode, I see values > 0xf0 ( > 240, which is the max value for > > CbCr samples in limited quantization range), so I assume quantization > > is full range. > > It should be, yes. What's the minimum and maximum values you get ? >From a white surface: min = 0x39 max = 0xfc >From a black surface: min = 0x00 (with 62 occurrences) max = 0x8b I guess that's indeed full range quantization > > > Actually the hardest part here was having a white enough surface to > > point the sensor to :) > > Pointing a flashlight to the sensor usually does the trick. I had a yellowish light that didn't work that well, I ended up putting a white paper sheet in front of it and then took the picture. Thanks j > > >
Re: [PATCH v9 11/11] media: i2c: ov7670: Fully set mbus frame fmt
Hi Jacopo, On Thursday, 22 February 2018 14:04:12 EET jacopo mondi wrote: > On Wed, Feb 21, 2018 at 10:28:06PM +0200, Laurent Pinchart wrote: > > On Tuesday, 20 February 2018 10:58:57 EET jacopo mondi wrote: > > > On Mon, Feb 19, 2018 at 09:19:32PM +0200, Laurent Pinchart wrote: > > > > On Monday, 19 February 2018 18:59:44 EET Jacopo Mondi wrote: > > > >> The sensor driver sets mbus format colorspace information and sizes, > > > >> but not ycbcr encoding, quantization and xfer function. When supplied > > > >> with an badly initialized mbus frame format structure, those fields > > > >> need to be set explicitly not to leave them uninitialized. This is > > > >> tested by v4l2-compliance, which supplies a mbus format description > > > >> structure and checks for all fields to be properly set. > > > >> > > > >> Without this commit, v4l2-compliance fails when testing formats with: > > > >> fail: v4l2-test-formats.cpp(335): ycbcr_enc >= 0xff > > > >> > > > >> Signed-off-by: Jacopo Mondi > > > >> --- > > > >> > > > >> drivers/media/i2c/ov7670.c | 4 > > > >> 1 file changed, 4 insertions(+) > > > >> > > > >> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > > > >> index 25b26d4..61c472e 100644 > > > >> --- a/drivers/media/i2c/ov7670.c > > > >> +++ b/drivers/media/i2c/ov7670.c > > > >> @@ -996,6 +996,10 @@ static int ov7670_try_fmt_internal(struct > > > >> v4l2_subdev > > > >> *sd, fmt->height = wsize->height; > > > >> > > > >>fmt->colorspace = ov7670_formats[index].colorspace; > > > > > > > > On a side note, if I'm not mistaken the colorspace field is set to > > > > SRGB > > > > for all entries. Shouldn't you hardcode it here and remove the field ? > > > > > > > >> + fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > > >> + fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > > > >> + fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT; > > > > > > > > How about setting the values explicitly instead of relying on defaults > > > > ? > > > > That would be V4L2_YCBCR_ENC_601, V4L2_QUANTIZATION_LIM_RANGE and > > > > V4L2_XFER_FUNC_SRGB. And could you then check a captured frame to see > > > > if > > > > the sensor outputs limited or full range ? > > > > > > This actually makes me wonder if those informations (ycbcb_enc, > > > quantization and xfer_func) shouldn't actually be part of the > > > supported format list... I blindly added those default fields in the > > > try_fmt function, but I doubt they applies to all supported formats. > > > > > > Eg. the sensor supports YUYV as well as 2 RGB encodings (RGB444 and > > > RGB 565) and 1 raw format (BGGR). I now have a question here: > > > > > > 1) ycbcr_enc transforms non-linear R'G'B' to Y'CbCr: does this > > > applies to RGB and raw formats? I don't think so, and what value is > > > the correct one for the ycbcr_enc field in this case? I assume > > > xfer_func and quantization applies to all formats instead.. > > > > There's no encoding for RGB formats if I understand things correctly, so > > I'd set ycbcr_enc to V4L2_YCBCR_ENC_DEFAULT. The transfer function and > > the quantization apply to all formats, but I'd be surprised to find a > > sensor outputting limited range for RGB. > > Ack, we got the same understanding for RGB formats. I wonder if for > those formats we wouldn't need a V4L2_YCBCR_ENC_NONE or similar... That, or explicitly documenting that when the format is not YUV the field should be set by both drivers and applications to V4L2_YCBCR_ENC_DEFAULT when written and ignored when read. > > Have you been able to check whether the sensor outputs limited range of > > full range YUV ? If it outputs full range you can hardcode quantization > > to V4L2_QUANTIZATION_FULL_RANGE for all formats. > > In YUYV mode, I see values > 0xf0 ( > 240, which is the max value for > CbCr samples in limited quantization range), so I assume quantization > is full range. It should be, yes. What's the minimum and maximum values you get ? > Actually the hardest part here was having a white enough surface to > point the sensor to :) Pointing a flashlight to the sensor usually does the trick. > info->format = *fmt; > > return 0; -- Regards, Laurent Pinchart
Re: [PATCH v9 11/11] media: i2c: ov7670: Fully set mbus frame fmt
Hi Laurent, On Wed, Feb 21, 2018 at 10:28:06PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Tuesday, 20 February 2018 10:58:57 EET jacopo mondi wrote: > > On Mon, Feb 19, 2018 at 09:19:32PM +0200, Laurent Pinchart wrote: > > > On Monday, 19 February 2018 18:59:44 EET Jacopo Mondi wrote: > > >> The sensor driver sets mbus format colorspace information and sizes, > > >> but not ycbcr encoding, quantization and xfer function. When supplied > > >> with an badly initialized mbus frame format structure, those fields > > >> need to be set explicitly not to leave them uninitialized. This is > > >> tested by v4l2-compliance, which supplies a mbus format description > > >> structure and checks for all fields to be properly set. > > >> > > >> Without this commit, v4l2-compliance fails when testing formats with: > > >> fail: v4l2-test-formats.cpp(335): ycbcr_enc >= 0xff > > >> > > >> Signed-off-by: Jacopo Mondi > > >> --- > > >> > > >> drivers/media/i2c/ov7670.c | 4 > > >> 1 file changed, 4 insertions(+) > > >> > > >> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > > >> index 25b26d4..61c472e 100644 > > >> --- a/drivers/media/i2c/ov7670.c > > >> +++ b/drivers/media/i2c/ov7670.c > > >> @@ -996,6 +996,10 @@ static int ov7670_try_fmt_internal(struct > > >> v4l2_subdev > > >> *sd, fmt->height = wsize->height; > > >> > > >> fmt->colorspace = ov7670_formats[index].colorspace; > > > > > > On a side note, if I'm not mistaken the colorspace field is set to SRGB > > > for all entries. Shouldn't you hardcode it here and remove the field ? > > > > > >> +fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > >> +fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > > >> +fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT; > > > > > > How about setting the values explicitly instead of relying on defaults ? > > > That would be V4L2_YCBCR_ENC_601, V4L2_QUANTIZATION_LIM_RANGE and > > > V4L2_XFER_FUNC_SRGB. And could you then check a captured frame to see if > > > the sensor outputs limited or full range ? > > > > This actually makes me wonder if those informations (ycbcb_enc, > > quantization and xfer_func) shouldn't actually be part of the > > supported format list... I blindly added those default fields in the > > try_fmt function, but I doubt they applies to all supported formats. > > > > Eg. the sensor supports YUYV as well as 2 RGB encodings (RGB444 and > > RGB 565) and 1 raw format (BGGR). I now have a question here: > > > > 1) ycbcr_enc transforms non-linear R'G'B' to Y'CbCr: does this > > applies to RGB and raw formats? I don't think so, and what value is > > the correct one for the ycbcr_enc field in this case? I assume > > xfer_func and quantization applies to all formats instead.. > > There's no encoding for RGB formats if I understand things correctly, so I'd > set ycbcr_enc to V4L2_YCBCR_ENC_DEFAULT. The transfer function and the > quantization apply to all formats, but I'd be surprised to find a sensor > outputting limited range for RGB. Ack, we got the same understanding for RGB formats. I wonder if for those formats we wouldn't need a V4L2_YCBCR_ENC_NONE or similar... > > Have you been able to check whether the sensor outputs limited range of full > range YUV ? If it outputs full range you can hardcode quantization to > V4L2_QUANTIZATION_FULL_RANGE for all formats. In YUYV mode, I see values > 0xf0 ( > 240, which is the max value for CbCr samples in limited quantization range), so I assume quantization is full range. Actually the hardest part here was having a white enough surface to point the sensor to :) Thanks j > > > >> info->format = *fmt; > > >> > > >> return 0; > > -- > Regards, > > Laurent Pinchart >
Re: [PATCH v9 11/11] media: i2c: ov7670: Fully set mbus frame fmt
Hi Jacopo, On Tuesday, 20 February 2018 10:58:57 EET jacopo mondi wrote: > On Mon, Feb 19, 2018 at 09:19:32PM +0200, Laurent Pinchart wrote: > > On Monday, 19 February 2018 18:59:44 EET Jacopo Mondi wrote: > >> The sensor driver sets mbus format colorspace information and sizes, > >> but not ycbcr encoding, quantization and xfer function. When supplied > >> with an badly initialized mbus frame format structure, those fields > >> need to be set explicitly not to leave them uninitialized. This is > >> tested by v4l2-compliance, which supplies a mbus format description > >> structure and checks for all fields to be properly set. > >> > >> Without this commit, v4l2-compliance fails when testing formats with: > >> fail: v4l2-test-formats.cpp(335): ycbcr_enc >= 0xff > >> > >> Signed-off-by: Jacopo Mondi > >> --- > >> > >> drivers/media/i2c/ov7670.c | 4 > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > >> index 25b26d4..61c472e 100644 > >> --- a/drivers/media/i2c/ov7670.c > >> +++ b/drivers/media/i2c/ov7670.c > >> @@ -996,6 +996,10 @@ static int ov7670_try_fmt_internal(struct > >> v4l2_subdev > >> *sd, fmt->height = wsize->height; > >> > >>fmt->colorspace = ov7670_formats[index].colorspace; > > > > On a side note, if I'm not mistaken the colorspace field is set to SRGB > > for all entries. Shouldn't you hardcode it here and remove the field ? > > > >> + fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > >> + fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > >> + fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT; > > > > How about setting the values explicitly instead of relying on defaults ? > > That would be V4L2_YCBCR_ENC_601, V4L2_QUANTIZATION_LIM_RANGE and > > V4L2_XFER_FUNC_SRGB. And could you then check a captured frame to see if > > the sensor outputs limited or full range ? > > This actually makes me wonder if those informations (ycbcb_enc, > quantization and xfer_func) shouldn't actually be part of the > supported format list... I blindly added those default fields in the > try_fmt function, but I doubt they applies to all supported formats. > > Eg. the sensor supports YUYV as well as 2 RGB encodings (RGB444 and > RGB 565) and 1 raw format (BGGR). I now have a question here: > > 1) ycbcr_enc transforms non-linear R'G'B' to Y'CbCr: does this > applies to RGB and raw formats? I don't think so, and what value is > the correct one for the ycbcr_enc field in this case? I assume > xfer_func and quantization applies to all formats instead.. There's no encoding for RGB formats if I understand things correctly, so I'd set ycbcr_enc to V4L2_YCBCR_ENC_DEFAULT. The transfer function and the quantization apply to all formats, but I'd be surprised to find a sensor outputting limited range for RGB. Have you been able to check whether the sensor outputs limited range of full range YUV ? If it outputs full range you can hardcode quantization to V4L2_QUANTIZATION_FULL_RANGE for all formats. > >>info->format = *fmt; > >> > >>return 0; -- Regards, Laurent Pinchart
Re: [PATCH v9 11/11] media: i2c: ov7670: Fully set mbus frame fmt
On 02/21/2018 04:47 PM, jacopo mondi wrote: > Hello again, > > On Tue, Feb 20, 2018 at 09:58:57AM +0100, jacopo mondi wrote: >> Hi Laurent, >> >> On Mon, Feb 19, 2018 at 09:19:32PM +0200, Laurent Pinchart wrote: >>> Hi Jacopo, >>> >>> Thank you for the patch. >>> >>> On Monday, 19 February 2018 18:59:44 EET Jacopo Mondi wrote: The sensor driver sets mbus format colorspace information and sizes, but not ycbcr encoding, quantization and xfer function. When supplied with an badly initialized mbus frame format structure, those fields need to be set explicitly not to leave them uninitialized. This is tested by v4l2-compliance, which supplies a mbus format description structure and checks for all fields to be properly set. Without this commit, v4l2-compliance fails when testing formats with: fail: v4l2-test-formats.cpp(335): ycbcr_enc >= 0xff Signed-off-by: Jacopo Mondi --- drivers/media/i2c/ov7670.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c index 25b26d4..61c472e 100644 --- a/drivers/media/i2c/ov7670.c +++ b/drivers/media/i2c/ov7670.c @@ -996,6 +996,10 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd, fmt->height = wsize->height; fmt->colorspace = ov7670_formats[index].colorspace; >>> >>> On a side note, if I'm not mistaken the colorspace field is set to SRGB for >>> all entries. Shouldn't you hardcode it here and remove the field ? >>> + fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; + fmt->quantization = V4L2_QUANTIZATION_DEFAULT; + fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT; >>> >>> How about setting the values explicitly instead of relying on defaults ? >>> That >>> would be V4L2_YCBCR_ENC_601, V4L2_QUANTIZATION_LIM_RANGE and >>> V4L2_XFER_FUNC_SRGB. And could you then check a captured frame to see if the >>> sensor outputs limited or full range ? >>> >> >> This actually makes me wonder if those informations (ycbcb_enc, >> quantization and xfer_func) shouldn't actually be part of the >> supported format list... I blindly added those default fields in the >> try_fmt function, but I doubt they applies to all supported formats. >> >> Eg. the sensor supports YUYV as well as 2 RGB encodings (RGB444 and >> RGB 565) and 1 raw format (BGGR). I now have a question here: >> >> 1) ycbcr_enc transforms non-linear R'G'B' to Y'CbCr: does this >> applies to RGB and raw formats? I don't think so, and what value is >> the correct one for the ycbcr_enc field in this case? I assume >> xfer_func and quantization applies to all formats instead.. >> > > What if I repropose this as separate patch not part of the CEU series > in order not to hold back v10 (which I hope will be the last CEU > iteration)? I would definitely be fine with that. We first need to define what exactly should be done by drivers. See also this thread: https://www.spinics.net/lists/linux-renesas-soc/msg23888.html I'll work on that going forward as part of the compliance tests. Regards, Hans > >> Thanks >>j >> info->format = *fmt; return 0; >>> >>> -- >>> Regards, >>> >>> Laurent Pinchart >>>
Re: [PATCH v9 11/11] media: i2c: ov7670: Fully set mbus frame fmt
Hello again, On Tue, Feb 20, 2018 at 09:58:57AM +0100, jacopo mondi wrote: > Hi Laurent, > > On Mon, Feb 19, 2018 at 09:19:32PM +0200, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Monday, 19 February 2018 18:59:44 EET Jacopo Mondi wrote: > > > The sensor driver sets mbus format colorspace information and sizes, > > > but not ycbcr encoding, quantization and xfer function. When supplied > > > with an badly initialized mbus frame format structure, those fields > > > need to be set explicitly not to leave them uninitialized. This is > > > tested by v4l2-compliance, which supplies a mbus format description > > > structure and checks for all fields to be properly set. > > > > > > Without this commit, v4l2-compliance fails when testing formats with: > > > fail: v4l2-test-formats.cpp(335): ycbcr_enc >= 0xff > > > > > > Signed-off-by: Jacopo Mondi > > > --- > > > drivers/media/i2c/ov7670.c | 4 > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > > > index 25b26d4..61c472e 100644 > > > --- a/drivers/media/i2c/ov7670.c > > > +++ b/drivers/media/i2c/ov7670.c > > > @@ -996,6 +996,10 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev > > > *sd, fmt->height = wsize->height; > > > fmt->colorspace = ov7670_formats[index].colorspace; > > > > On a side note, if I'm not mistaken the colorspace field is set to SRGB for > > all entries. Shouldn't you hardcode it here and remove the field ? > > > > > + fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > > + fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > > > + fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT; > > > > How about setting the values explicitly instead of relying on defaults ? > > That > > would be V4L2_YCBCR_ENC_601, V4L2_QUANTIZATION_LIM_RANGE and > > V4L2_XFER_FUNC_SRGB. And could you then check a captured frame to see if the > > sensor outputs limited or full range ? > > > > This actually makes me wonder if those informations (ycbcb_enc, > quantization and xfer_func) shouldn't actually be part of the > supported format list... I blindly added those default fields in the > try_fmt function, but I doubt they applies to all supported formats. > > Eg. the sensor supports YUYV as well as 2 RGB encodings (RGB444 and > RGB 565) and 1 raw format (BGGR). I now have a question here: > > 1) ycbcr_enc transforms non-linear R'G'B' to Y'CbCr: does this > applies to RGB and raw formats? I don't think so, and what value is > the correct one for the ycbcr_enc field in this case? I assume > xfer_func and quantization applies to all formats instead.. > What if I repropose this as separate patch not part of the CEU series in order not to hold back v10 (which I hope will be the last CEU iteration)? > Thanks >j > > > > info->format = *fmt; > > > > > > return 0; > > > > -- > > Regards, > > > > Laurent Pinchart > >
Re: [PATCH v9 11/11] media: i2c: ov7670: Fully set mbus frame fmt
Hi Laurent, On Mon, Feb 19, 2018 at 09:19:32PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Monday, 19 February 2018 18:59:44 EET Jacopo Mondi wrote: > > The sensor driver sets mbus format colorspace information and sizes, > > but not ycbcr encoding, quantization and xfer function. When supplied > > with an badly initialized mbus frame format structure, those fields > > need to be set explicitly not to leave them uninitialized. This is > > tested by v4l2-compliance, which supplies a mbus format description > > structure and checks for all fields to be properly set. > > > > Without this commit, v4l2-compliance fails when testing formats with: > > fail: v4l2-test-formats.cpp(335): ycbcr_enc >= 0xff > > > > Signed-off-by: Jacopo Mondi > > --- > > drivers/media/i2c/ov7670.c | 4 > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > > index 25b26d4..61c472e 100644 > > --- a/drivers/media/i2c/ov7670.c > > +++ b/drivers/media/i2c/ov7670.c > > @@ -996,6 +996,10 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev > > *sd, fmt->height = wsize->height; > > fmt->colorspace = ov7670_formats[index].colorspace; > > On a side note, if I'm not mistaken the colorspace field is set to SRGB for > all entries. Shouldn't you hardcode it here and remove the field ? > > > + fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > + fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > > + fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT; > > How about setting the values explicitly instead of relying on defaults ? That > would be V4L2_YCBCR_ENC_601, V4L2_QUANTIZATION_LIM_RANGE and > V4L2_XFER_FUNC_SRGB. And could you then check a captured frame to see if the > sensor outputs limited or full range ? > This actually makes me wonder if those informations (ycbcb_enc, quantization and xfer_func) shouldn't actually be part of the supported format list... I blindly added those default fields in the try_fmt function, but I doubt they applies to all supported formats. Eg. the sensor supports YUYV as well as 2 RGB encodings (RGB444 and RGB 565) and 1 raw format (BGGR). I now have a question here: 1) ycbcr_enc transforms non-linear R'G'B' to Y'CbCr: does this applies to RGB and raw formats? I don't think so, and what value is the correct one for the ycbcr_enc field in this case? I assume xfer_func and quantization applies to all formats instead.. Thanks j > > info->format = *fmt; > > > > return 0; > > -- > Regards, > > Laurent Pinchart >
Re: [PATCH v9 11/11] media: i2c: ov7670: Fully set mbus frame fmt
Hi Jacopo, Thank you for the patch. On Monday, 19 February 2018 18:59:44 EET Jacopo Mondi wrote: > The sensor driver sets mbus format colorspace information and sizes, > but not ycbcr encoding, quantization and xfer function. When supplied > with an badly initialized mbus frame format structure, those fields > need to be set explicitly not to leave them uninitialized. This is > tested by v4l2-compliance, which supplies a mbus format description > structure and checks for all fields to be properly set. > > Without this commit, v4l2-compliance fails when testing formats with: > fail: v4l2-test-formats.cpp(335): ycbcr_enc >= 0xff > > Signed-off-by: Jacopo Mondi > --- > drivers/media/i2c/ov7670.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > index 25b26d4..61c472e 100644 > --- a/drivers/media/i2c/ov7670.c > +++ b/drivers/media/i2c/ov7670.c > @@ -996,6 +996,10 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev > *sd, fmt->height = wsize->height; > fmt->colorspace = ov7670_formats[index].colorspace; On a side note, if I'm not mistaken the colorspace field is set to SRGB for all entries. Shouldn't you hardcode it here and remove the field ? > + fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > + fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > + fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT; How about setting the values explicitly instead of relying on defaults ? That would be V4L2_YCBCR_ENC_601, V4L2_QUANTIZATION_LIM_RANGE and V4L2_XFER_FUNC_SRGB. And could you then check a captured frame to see if the sensor outputs limited or full range ? > info->format = *fmt; > > return 0; -- Regards, Laurent Pinchart
