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?
> > - 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
