| Message ID | 20220404163533.707508-5-p.zabel@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] media: coda: set output buffer bytesused to appease v4l2-compliance | expand |
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 */
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
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
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 --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 */
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(-)