diff mbox series

[5/7] media: coda: fix default JPEG colorimetry

Message ID 20220404163533.707508-5-p.zabel@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series [1/7] media: coda: set output buffer bytesused to appease v4l2-compliance | expand

Commit Message

Philipp Zabel April 4, 2022, 4:35 p.m. UTC
Set default colorspace to SRGB for JPEG encoder and decoder devices,
to fix the following v4l2-compliance test failure:

	test VIDIOC_TRY_FMT: OK
		fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB

Also explicitly set transfer function, YCbCr encoding and quantization
range, as required by v4l2-compliance for the JPEG encoded side.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 .../media/platform/chips-media/coda-common.c  | 36 +++++++++++++------
 1 file changed, 25 insertions(+), 11 deletions(-)

Comments

Nicolas Dufresne April 5, 2022, 2:15 p.m. UTC | #1
Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit :
> Set default colorspace to SRGB for JPEG encoder and decoder devices,
> to fix the following v4l2-compliance test failure:
> 
> 	test VIDIOC_TRY_FMT: OK
> 		fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB
> 
> Also explicitly set transfer function, YCbCr encoding and quantization
> range, as required by v4l2-compliance for the JPEG encoded side.

Note that this will perhaps hit some GStreamer bugs that are being discussed.
Documenting for the users:

https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1118
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1406

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  .../media/platform/chips-media/coda-common.c  | 36 +++++++++++++------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> index 4a7346ed771e..c068c16d1eb4 100644
> --- a/drivers/media/platform/chips-media/coda-common.c
> +++ b/drivers/media/platform/chips-media/coda-common.c
> @@ -732,13 +732,22 @@ static int coda_try_fmt_vid_cap(struct file *file, void *priv,
>  	return 0;
>  }
>  
> -static void coda_set_default_colorspace(struct v4l2_pix_format *fmt)
> +static void coda_set_default_colorspace(struct coda_ctx *ctx,
> +					struct v4l2_pix_format *fmt)
>  {
>  	enum v4l2_colorspace colorspace;
>  
> -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> -		colorspace = V4L2_COLORSPACE_JPEG;
> -	else if (fmt->width <= 720 && fmt->height <= 576)
> +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG ||
> +	    fmt->pixelformat == V4L2_PIX_FMT_JPEG) {
> +		fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +		fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
> +		fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +		fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +		return;
> +	}
> +
> +	if (fmt->width <= 720 && fmt->height <= 576)
>  		colorspace = V4L2_COLORSPACE_SMPTE170M;
>  	else
>  		colorspace = V4L2_COLORSPACE_REC709;
> @@ -763,7 +772,7 @@ static int coda_try_fmt_vid_out(struct file *file, void *priv,
>  		return ret;
>  
>  	if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT)
> -		coda_set_default_colorspace(&f->fmt.pix);
> +		coda_set_default_colorspace(ctx, &f->fmt.pix);
>  
>  	q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>  	codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc);
> @@ -1640,13 +1649,18 @@ static void set_default_params(struct coda_ctx *ctx)
>  	csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h);
>  
>  	ctx->params.codec_mode = ctx->codec->mode;
> -	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG)
> -		ctx->colorspace = V4L2_COLORSPACE_JPEG;
> -	else
> +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
> +		ctx->colorspace = V4L2_COLORSPACE_SRGB;
> +		ctx->xfer_func = V4L2_XFER_FUNC_SRGB;
> +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +		ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	} else {
>  		ctx->colorspace = V4L2_COLORSPACE_REC709;
> -	ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> -	ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> -	ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> +		ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +		ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> +	}
>  	ctx->params.framerate = 30;
>  
>  	/* Default formats for output and input queues */
Hans Verkuil April 21, 2022, 10:02 a.m. UTC | #2
Hi Philipp,

On 04/04/2022 18:35, Philipp Zabel wrote:
> Set default colorspace to SRGB for JPEG encoder and decoder devices,
> to fix the following v4l2-compliance test failure:
> 
> 	test VIDIOC_TRY_FMT: OK
> 		fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB
> 
> Also explicitly set transfer function, YCbCr encoding and quantization
> range, as required by v4l2-compliance for the JPEG encoded side.

I'm not quite sure if this patch addresses the correct issue.

> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  .../media/platform/chips-media/coda-common.c  | 36 +++++++++++++------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> index 4a7346ed771e..c068c16d1eb4 100644
> --- a/drivers/media/platform/chips-media/coda-common.c
> +++ b/drivers/media/platform/chips-media/coda-common.c
> @@ -732,13 +732,22 @@ static int coda_try_fmt_vid_cap(struct file *file, void *priv,
>  	return 0;
>  }
>  
> -static void coda_set_default_colorspace(struct v4l2_pix_format *fmt)
> +static void coda_set_default_colorspace(struct coda_ctx *ctx,
> +					struct v4l2_pix_format *fmt)
>  {
>  	enum v4l2_colorspace colorspace;
>  
> -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> -		colorspace = V4L2_COLORSPACE_JPEG;

It's perfectly fine to keep this, the problem occurs with the raw image side
(capture for the decoder, output for the encoder).

There the colorspace must be SRGB, the xfer_func may be 0 or SRGB, and the
ycbcr_enc is 0 (if not a YUV pixelformat) or ENC_601 (if it is a YUV format).
Actually, if the hardware can convert from other YUV encodings such as 709,
then other YUV encodings are valid, but I assume that's not the case.

> -	else if (fmt->width <= 720 && fmt->height <= 576)
> +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG ||
> +	    fmt->pixelformat == V4L2_PIX_FMT_JPEG) {
> +		fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +		fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
> +		fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +		fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +		return;
> +	}
> +
> +	if (fmt->width <= 720 && fmt->height <= 576)
>  		colorspace = V4L2_COLORSPACE_SMPTE170M;
>  	else
>  		colorspace = V4L2_COLORSPACE_REC709;
> @@ -763,7 +772,7 @@ static int coda_try_fmt_vid_out(struct file *file, void *priv,
>  		return ret;
>  
>  	if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT)
> -		coda_set_default_colorspace(&f->fmt.pix);
> +		coda_set_default_colorspace(ctx, &f->fmt.pix);
>  
>  	q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>  	codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc);
> @@ -1640,13 +1649,18 @@ static void set_default_params(struct coda_ctx *ctx)
>  	csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h);
>  
>  	ctx->params.codec_mode = ctx->codec->mode;
> -	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG)
> -		ctx->colorspace = V4L2_COLORSPACE_JPEG;
> -	else
> +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
> +		ctx->colorspace = V4L2_COLORSPACE_SRGB;
> +		ctx->xfer_func = V4L2_XFER_FUNC_SRGB;
> +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +		ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	} else {
>  		ctx->colorspace = V4L2_COLORSPACE_REC709;

My guess is that the raw format colorspace is set to REC709, which is definitely
wrong for a JPEG codec. For a JPEG codec that must be set to SRGB.

I suspect that's the real bug here.

I'm skipping this patch for now.

Regards,

	Hans

> -	ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> -	ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> -	ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> +		ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +		ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> +	}
>  	ctx->params.framerate = 30;
>  
>  	/* Default formats for output and input queues */
Philipp Zabel April 21, 2022, 10:38 a.m. UTC | #3
On Do, 2022-04-21 at 12:02 +0200, Hans Verkuil wrote:
> Hi Philipp,
> 
> On 04/04/2022 18:35, Philipp Zabel wrote:
> > Set default colorspace to SRGB for JPEG encoder and decoder devices,
> > to fix the following v4l2-compliance test failure:
> > 
> > 	test VIDIOC_TRY_FMT: OK
> > 		fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB
> > 
> > Also explicitly set transfer function, YCbCr encoding and quantization
> > range, as required by v4l2-compliance for the JPEG encoded side.
> 
> I'm not quite sure if this patch addresses the correct issue.
> 
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  .../media/platform/chips-media/coda-common.c  | 36 +++++++++++++------
> >  1 file changed, 25 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> > index 4a7346ed771e..c068c16d1eb4 100644
> > --- a/drivers/media/platform/chips-media/coda-common.c
> > +++ b/drivers/media/platform/chips-media/coda-common.c
> > @@ -732,13 +732,22 @@ static int coda_try_fmt_vid_cap(struct file *file, void *priv,
> >  	return 0;
> >  }
> >  
> > 
> > 
> > 
> > -static void coda_set_default_colorspace(struct v4l2_pix_format *fmt)
> > +static void coda_set_default_colorspace(struct coda_ctx *ctx,
> > +					struct v4l2_pix_format *fmt)
> >  {
> >  	enum v4l2_colorspace colorspace;
> >  
> > 
> > 
> > 
> > -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> > -		colorspace = V4L2_COLORSPACE_JPEG;
> 
> It's perfectly fine to keep this, the problem occurs with the raw image side
> (capture for the decoder, output for the encoder).
> 
> There the colorspace must be SRGB, the xfer_func may be 0 or SRGB, and the
> ycbcr_enc is 0 (if not a YUV pixelformat) or ENC_601 (if it is a YUV format).
> Actually, if the hardware can convert from other YUV encodings such as 709,
> then other YUV encodings are valid, but I assume that's not the case.

So the driver has to support different colorspace on output and capture
queues?

> > -	else if (fmt->width <= 720 && fmt->height <= 576)
> > +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> > +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG ||
> > +	    fmt->pixelformat == V4L2_PIX_FMT_JPEG) {
> > +		fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > +		fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
> > +		fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > +		fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +		return;
> > +	}
> > +
> > +	if (fmt->width <= 720 && fmt->height <= 576)
> >  		colorspace = V4L2_COLORSPACE_SMPTE170M;
> >  	else
> >  		colorspace = V4L2_COLORSPACE_REC709;
> > @@ -763,7 +772,7 @@ static int coda_try_fmt_vid_out(struct file *file, void *priv,
> >  		return ret;
> >  
> > 
> > 
> > 
> >  	if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT)
> > -		coda_set_default_colorspace(&f->fmt.pix);
> > +		coda_set_default_colorspace(ctx, &f->fmt.pix);
> >  
> > 
> > 
> > 
> >  	q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> >  	codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc);
> > @@ -1640,13 +1649,18 @@ static void set_default_params(struct coda_ctx *ctx)
> >  	csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h);
> >  
> > 
> > 
> > 
> >  	ctx->params.codec_mode = ctx->codec->mode;
> > -	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG)
> > -		ctx->colorspace = V4L2_COLORSPACE_JPEG;
> > -	else
> > +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> > +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
> > +		ctx->colorspace = V4L2_COLORSPACE_SRGB;
> > +		ctx->xfer_func = V4L2_XFER_FUNC_SRGB;
> > +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > +		ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +	} else {
> >  		ctx->colorspace = V4L2_COLORSPACE_REC709;
> 
> My guess is that the raw format colorspace is set to REC709, which is definitely
> wrong for a JPEG codec. For a JPEG codec that must be set to SRGB.
> 
> I suspect that's the real bug here.
> 
> I'm skipping this patch for now.

Thank you, I think at least for the decoder the issue was that the
driver defaulted to V4L2_COLORSPACE_JPEG, but since ctx->colorspace is
used for both sides, that would also be used as colorspace for the raw
image side. For the encoder it looks like you are right.

I'll double check.

regards
Philipp
Hans Verkuil April 21, 2022, 11:06 a.m. UTC | #4
On 21/04/2022 12:38, Philipp Zabel wrote:
> On Do, 2022-04-21 at 12:02 +0200, Hans Verkuil wrote:
>> Hi Philipp,
>>
>> On 04/04/2022 18:35, Philipp Zabel wrote:
>>> Set default colorspace to SRGB for JPEG encoder and decoder devices,
>>> to fix the following v4l2-compliance test failure:
>>>
>>> 	test VIDIOC_TRY_FMT: OK
>>> 		fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB
>>>
>>> Also explicitly set transfer function, YCbCr encoding and quantization
>>> range, as required by v4l2-compliance for the JPEG encoded side.
>>
>> I'm not quite sure if this patch addresses the correct issue.
>>
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>  .../media/platform/chips-media/coda-common.c  | 36 +++++++++++++------
>>>  1 file changed, 25 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
>>> index 4a7346ed771e..c068c16d1eb4 100644
>>> --- a/drivers/media/platform/chips-media/coda-common.c
>>> +++ b/drivers/media/platform/chips-media/coda-common.c
>>> @@ -732,13 +732,22 @@ static int coda_try_fmt_vid_cap(struct file *file, void *priv,
>>>  	return 0;
>>>  }
>>>  
>>>
>>>
>>>
>>> -static void coda_set_default_colorspace(struct v4l2_pix_format *fmt)
>>> +static void coda_set_default_colorspace(struct coda_ctx *ctx,
>>> +					struct v4l2_pix_format *fmt)
>>>  {
>>>  	enum v4l2_colorspace colorspace;
>>>  
>>>
>>>
>>>
>>> -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
>>> -		colorspace = V4L2_COLORSPACE_JPEG;
>>
>> It's perfectly fine to keep this, the problem occurs with the raw image side
>> (capture for the decoder, output for the encoder).
>>
>> There the colorspace must be SRGB, the xfer_func may be 0 or SRGB, and the
>> ycbcr_enc is 0 (if not a YUV pixelformat) or ENC_601 (if it is a YUV format).
>> Actually, if the hardware can convert from other YUV encodings such as 709,
>> then other YUV encodings are valid, but I assume that's not the case.
> 
> So the driver has to support different colorspace on output and capture
> queues?

Correct. Note that it is OK to replace COLORSPACE_JPEG by explicit colorspace,
xfer_func, ycbcr_enc and quantization values, but in reality (almost?) all drivers
use COLORSPACE_JPEG, and that won't go away. Keeping it will certainly reduce
the patch size.

Regards,

	Hans

> 
>>> -	else if (fmt->width <= 720 && fmt->height <= 576)
>>> +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
>>> +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG ||
>>> +	    fmt->pixelformat == V4L2_PIX_FMT_JPEG) {
>>> +		fmt->colorspace = V4L2_COLORSPACE_SRGB;
>>> +		fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
>>> +		fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>> +		fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>>> +		return;
>>> +	}
>>> +
>>> +	if (fmt->width <= 720 && fmt->height <= 576)
>>>  		colorspace = V4L2_COLORSPACE_SMPTE170M;
>>>  	else
>>>  		colorspace = V4L2_COLORSPACE_REC709;
>>> @@ -763,7 +772,7 @@ static int coda_try_fmt_vid_out(struct file *file, void *priv,
>>>  		return ret;
>>>  
>>>
>>>
>>>
>>>  	if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT)
>>> -		coda_set_default_colorspace(&f->fmt.pix);
>>> +		coda_set_default_colorspace(ctx, &f->fmt.pix);
>>>  
>>>
>>>
>>>
>>>  	q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>>>  	codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc);
>>> @@ -1640,13 +1649,18 @@ static void set_default_params(struct coda_ctx *ctx)
>>>  	csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h);
>>>  
>>>
>>>
>>>
>>>  	ctx->params.codec_mode = ctx->codec->mode;
>>> -	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG)
>>> -		ctx->colorspace = V4L2_COLORSPACE_JPEG;
>>> -	else
>>> +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
>>> +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
>>> +		ctx->colorspace = V4L2_COLORSPACE_SRGB;
>>> +		ctx->xfer_func = V4L2_XFER_FUNC_SRGB;
>>> +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>> +		ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>>> +	} else {
>>>  		ctx->colorspace = V4L2_COLORSPACE_REC709;
>>
>> My guess is that the raw format colorspace is set to REC709, which is definitely
>> wrong for a JPEG codec. For a JPEG codec that must be set to SRGB.
>>
>> I suspect that's the real bug here.
>>
>> I'm skipping this patch for now.
> 
> Thank you, I think at least for the decoder the issue was that the
> driver defaulted to V4L2_COLORSPACE_JPEG, but since ctx->colorspace is
> used for both sides, that would also be used as colorspace for the raw
> image side. For the encoder it looks like you are right.
> 
> I'll double check.
> 
> regards
> Philipp
Philipp Zabel April 21, 2022, 2:54 p.m. UTC | #5
On Do, 2022-04-21 at 12:02 +0200, Hans Verkuil wrote:
[...]
> > -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> > -		colorspace = V4L2_COLORSPACE_JPEG;
> 
> It's perfectly fine to keep this, the problem occurs with the raw image side
> (capture for the decoder, output for the encoder).
> 
> There the colorspace must be SRGB, the xfer_func may be 0 or SRGB,

v4l2-compliance doesn't allow xfer_func 0:

		fail: v4l2-test-formats.cpp(835): fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_SRGB
	test VIDIOC_S_FMT: FAIL

Is this too strict?

> and the
> ycbcr_enc is 0 (if not a YUV pixelformat) or ENC_601 (if it is a YUV format).

Why does it have to be ENC_601 for YUV formats? Both
V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_JPEG) and
V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SRGB) map to
V4L2_YCBCR_ENC_601 regardless of the pixel format, so there should be
no difference if 0 was allowed as well.

On the other hand, quantization should be set to
V4L2_QUANTIZATION_FULL_RANGE for YUV formats on the raw image side, as
that defaults to LIM_RANGE for raw YUV images with SRGB colorspace.

Basically, I wonder whether or not it would be a good idea to apply the
following change to v4l2-compliance:

diff --git a/utils/v4l2-compliance/v4l2-test-formats.cpp b/utils/v4l2-compliance/v4l2-test-formats.cpp
index 269a3832fcdf..902c66367ff7 100644
--- a/utils/v4l2-compliance/v4l2-test-formats.cpp
+++ b/utils/v4l2-compliance/v4l2-test-formats.cpp
@@ -832,8 +832,10 @@ static int testJPEGColorspace(const cv4l_fmt &fmt_raw, const cv4l_fmt &fmt_jpeg,
        }
        /* V4L2_COLORSPACE_JPEG is shorthand for these values: */
        fail_on_test(fmt_jpeg.g_colorspace() != V4L2_COLORSPACE_SRGB);
-       fail_on_test(fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_SRGB);
-       fail_on_test(fmt_jpeg.g_ycbcr_enc() != V4L2_YCBCR_ENC_601);
+       fail_on_test(fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_DEFAULT &&
+                    fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_SRGB);
+       fail_on_test(fmt_jpeg.g_ycbcr_enc() != V4L2_YCBCR_ENC_DEFAULT &&
+                    fmt_jpeg.g_ycbcr_enc() != V4L2_YCBCR_ENC_601);
        fail_on_test(fmt_jpeg.g_quantization() != V4L2_QUANTIZATION_FULL_RANGE);
        return 0;
 }

Actually, if the hardware can convert from other YUV encodings such as 709,
then other YUV encodings are valid, but I assume that's not the case.

True, the hardware is completely oblivious to colorimetry.

regards
Philipp
Philipp Zabel April 21, 2022, 3:28 p.m. UTC | #6
On Do, 2022-04-21 at 16:54 +0200, Philipp Zabel wrote:
> On Do, 2022-04-21 at 12:02 +0200, Hans Verkuil wrote:
> [...]
> > > -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> > > -		colorspace = V4L2_COLORSPACE_JPEG;
> > 
> > It's perfectly fine to keep this, the problem occurs with the raw image side
> > (capture for the decoder, output for the encoder).
> > 
> > There the colorspace must be SRGB, the xfer_func may be 0 or SRGB,
> 
> v4l2-compliance doesn't allow xfer_func 0:
> 
> 		fail: v4l2-test-formats.cpp(835): fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_SRGB
> 	test VIDIOC_S_FMT: FAIL
> 
> Is this too strict?
> 
> > and the
> > ycbcr_enc is 0 (if not a YUV pixelformat) or ENC_601 (if it is a YUV format).
> 
> Why does it have to be ENC_601 for YUV formats? Both
> V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_JPEG) and
> V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SRGB) map to
> V4L2_YCBCR_ENC_601 regardless of the pixel format, so there should be
> no difference if 0 was allowed as well.
> 
> On the other hand, quantization should be set to
> V4L2_QUANTIZATION_FULL_RANGE for YUV formats on the raw image side, as
> that defaults to LIM_RANGE for raw YUV images with SRGB colorspace.
> 
> Basically, I wonder whether or not it would be a good idea to apply the
> following change to v4l2-compliance:
> 
> diff --git a/utils/v4l2-compliance/v4l2-test-formats.cpp b/utils/v4l2-compliance/v4l2-test-formats.cpp
> index 269a3832fcdf..902c66367ff7 100644
> --- a/utils/v4l2-compliance/v4l2-test-formats.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-formats.cpp
> @@ -832,8 +832,10 @@ static int testJPEGColorspace(const cv4l_fmt &fmt_raw, const cv4l_fmt &fmt_jpeg,
>         }
>         /* V4L2_COLORSPACE_JPEG is shorthand for these values: */
>         fail_on_test(fmt_jpeg.g_colorspace() != V4L2_COLORSPACE_SRGB);
> -       fail_on_test(fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_SRGB);
> -       fail_on_test(fmt_jpeg.g_ycbcr_enc() != V4L2_YCBCR_ENC_601);
> +       fail_on_test(fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_DEFAULT &&
> +                    fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_SRGB);
> +       fail_on_test(fmt_jpeg.g_ycbcr_enc() != V4L2_YCBCR_ENC_DEFAULT &&
> +                    fmt_jpeg.g_ycbcr_enc() != V4L2_YCBCR_ENC_601);
>         fail_on_test(fmt_jpeg.g_quantization() != V4L2_QUANTIZATION_FULL_RANGE);
>         return 0;
>  }

Please disregard, I overlooked that this part is checking the encoded
image side. The raw image side is allowed 0 ycbcr_enc and xfer_func
already, and raw image quantization is not checked at all.

regards
Philipp
Philipp Zabel April 26, 2022, 9:21 a.m. UTC | #7
Hi Hans,

On Do, 2022-04-21 at 13:06 +0200, Hans Verkuil wrote:
> On 21/04/2022 12:38, Philipp Zabel wrote:
> > On Do, 2022-04-21 at 12:02 +0200, Hans Verkuil wrote:
> > > Hi Philipp,
> > > 
> > > On 04/04/2022 18:35, Philipp Zabel wrote:
> > > > Set default colorspace to SRGB for JPEG encoder and decoder devices,
> > > > to fix the following v4l2-compliance test failure:
> > > > 
> > > > 	test VIDIOC_TRY_FMT: OK
> > > > 		fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB
> > > > 
> > > > Also explicitly set transfer function, YCbCr encoding and quantization
> > > > range, as required by v4l2-compliance for the JPEG encoded side.
> > > 
> > > I'm not quite sure if this patch addresses the correct issue.
> > > 
> > > > 
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > ---
> > > >  .../media/platform/chips-media/coda-common.c  | 36 +++++++++++++------
> > > >  1 file changed, 25 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> > > > index 4a7346ed771e..c068c16d1eb4 100644
> > > > --- a/drivers/media/platform/chips-media/coda-common.c
> > > > +++ b/drivers/media/platform/chips-media/coda-common.c
> > > > @@ -732,13 +732,22 @@ static int coda_try_fmt_vid_cap(struct file *file, void *priv,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > 
> > > > 
> > > > 
> > > > -static void coda_set_default_colorspace(struct v4l2_pix_format *fmt)
> > > > +static void coda_set_default_colorspace(struct coda_ctx *ctx,
> > > > +					struct v4l2_pix_format *fmt)
> > > >  {
> > > >  	enum v4l2_colorspace colorspace;
> > > >  
> > > > 
> > > > 
> > > > 
> > > > -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> > > > -		colorspace = V4L2_COLORSPACE_JPEG;
> > > 
> > > It's perfectly fine to keep this, the problem occurs with the raw image side
> > > (capture for the decoder, output for the encoder).
> > > 
> > > There the colorspace must be SRGB, the xfer_func may be 0 or SRGB, and the
> > > ycbcr_enc is 0 (if not a YUV pixelformat) or ENC_601 (if it is a YUV format).
> > > Actually, if the hardware can convert from other YUV encodings such as 709,
> > > then other YUV encodings are valid, but I assume that's not the case.
> > 
> > So the driver has to support different colorspace on output and capture
> > queues?
> 
> Correct. Note that it is OK to replace COLORSPACE_JPEG by explicit colorspace,
> xfer_func, ycbcr_enc and quantization values, but in reality (almost?) all drivers
> use COLORSPACE_JPEG, and that won't go away. Keeping it will certainly reduce
> the patch size.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > > > -	else if (fmt->width <= 720 && fmt->height <= 576)
> > > > +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> > > > +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG ||
> > > > +	    fmt->pixelformat == V4L2_PIX_FMT_JPEG) {
> > > > +		fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > > > +		fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
> > > > +		fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > > > +		fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	if (fmt->width <= 720 && fmt->height <= 576)
> > > >  		colorspace = V4L2_COLORSPACE_SMPTE170M;
> > > >  	else
> > > >  		colorspace = V4L2_COLORSPACE_REC709;

coda_set_default_colorspace() is only called when userspace calls S_FMT
with colorspace = V4L2_COLORSPACE_DEFAULT.

Since v4l2-compliance doesn't care about this, I've dropped this part.

> > > > @@ -763,7 +772,7 @@ static int coda_try_fmt_vid_out(struct file *file, void *priv,
> > > >  		return ret;
> > > >  
> > > > 
> > > > 
> > > > 
> > > >  	if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT)
> > > > -		coda_set_default_colorspace(&f->fmt.pix);
> > > > +		coda_set_default_colorspace(ctx, &f->fmt.pix);

And this.

> > > >  
> > > > 
> > > > 
> > > > 
> > > >  	q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > > >  	codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc);
> > > > @@ -1640,13 +1649,18 @@ static void set_default_params(struct coda_ctx *ctx)
> > > >  	csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h);
> > > >  
> > > > 
> > > > 
> > > > 
> > > >  	ctx->params.codec_mode = ctx->codec->mode;
> > > > -	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG)
> > > > -		ctx->colorspace = V4L2_COLORSPACE_JPEG;
> > > > -	else
> > > > +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> > > > +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
> > > > +		ctx->colorspace = V4L2_COLORSPACE_SRGB;
> > > > +		ctx->xfer_func = V4L2_XFER_FUNC_SRGB;
> > > > +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > > > +		ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > > +	} else {
> > > >  		ctx->colorspace = V4L2_COLORSPACE_REC709;
> > > 
> > > My guess is that the raw format colorspace is set to REC709, which is definitely
> > > wrong for a JPEG codec. For a JPEG codec that must be set to SRGB.
> > > 
> > > I suspect that's the real bug here.

Right, this part is enough to make v4l2-compliance happy. I've sent a
v2 reduced to this.

The driver still only supports identical colorimetry on both queues, so
when userspace sets colorspace = V4L2_COLORSPACE_JPEG on the output
queue, the capture queue will be set to the same.

regards
Philipp
diff mbox series

Patch

diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
index 4a7346ed771e..c068c16d1eb4 100644
--- a/drivers/media/platform/chips-media/coda-common.c
+++ b/drivers/media/platform/chips-media/coda-common.c
@@ -732,13 +732,22 @@  static int coda_try_fmt_vid_cap(struct file *file, void *priv,
 	return 0;
 }
 
-static void coda_set_default_colorspace(struct v4l2_pix_format *fmt)
+static void coda_set_default_colorspace(struct coda_ctx *ctx,
+					struct v4l2_pix_format *fmt)
 {
 	enum v4l2_colorspace colorspace;
 
-	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
-		colorspace = V4L2_COLORSPACE_JPEG;
-	else if (fmt->width <= 720 && fmt->height <= 576)
+	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
+	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG ||
+	    fmt->pixelformat == V4L2_PIX_FMT_JPEG) {
+		fmt->colorspace = V4L2_COLORSPACE_SRGB;
+		fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
+		fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
+		fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+		return;
+	}
+
+	if (fmt->width <= 720 && fmt->height <= 576)
 		colorspace = V4L2_COLORSPACE_SMPTE170M;
 	else
 		colorspace = V4L2_COLORSPACE_REC709;
@@ -763,7 +772,7 @@  static int coda_try_fmt_vid_out(struct file *file, void *priv,
 		return ret;
 
 	if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT)
-		coda_set_default_colorspace(&f->fmt.pix);
+		coda_set_default_colorspace(ctx, &f->fmt.pix);
 
 	q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
 	codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc);
@@ -1640,13 +1649,18 @@  static void set_default_params(struct coda_ctx *ctx)
 	csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h);
 
 	ctx->params.codec_mode = ctx->codec->mode;
-	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG)
-		ctx->colorspace = V4L2_COLORSPACE_JPEG;
-	else
+	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
+	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
+		ctx->colorspace = V4L2_COLORSPACE_SRGB;
+		ctx->xfer_func = V4L2_XFER_FUNC_SRGB;
+		ctx->ycbcr_enc = V4L2_YCBCR_ENC_601;
+		ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	} else {
 		ctx->colorspace = V4L2_COLORSPACE_REC709;
-	ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
-	ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
-	ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
+		ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
+		ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+		ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
+	}
 	ctx->params.framerate = 30;
 
 	/* Default formats for output and input queues */