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@xxxxxxxxxxxxxx>
>>> ---
>>> .../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
