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

[Hans Verkuil <hverkuil@xs4all.nl>]

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