Re: [EXT] Re: [PATCH v7 3/9] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder

[Mirela Rabulea <mirela.rabulea@nxp.com>]

Hi Hans,

On Thu, 2021-03-04 at 14:03 +0100, Hans Verkuil wrote:
> Caution: EXT Email
> 
> On 22/02/2021 20:09, Mirela Rabulea wrote:
> > Hi Hans,
> > appologies for my late response, please see below 2 comments.
> 
> Replies below:
> 
> > 
> > On Tue, 2021-01-19 at 11:31 +0100, Hans Verkuil wrote:
> > > Caution: EXT Email
> > > 
> > > On 11/01/2021 20:28, Mirela Rabulea wrote:
> > > > From: Mirela Rabulea <mirela.rabulea@nxp.com>
> > > > 
> > > > V4L2 driver for the JPEG encoder/decoder from i.MX8QXP/i.MX8QM
> > > > application
> > > > processors.
> > > > The multi-planar buffers API is used.
> > > > 
> > > > Baseline and extended sequential jpeg decoding is supported.
> > > > Progressive jpeg decoding is not supported by the IP.
> > > > Supports encode and decode of various formats:
> > > >      YUV444, YUV422, YUV420, RGB, ARGB, Gray
> > > > YUV420 is the only multi-planar format supported.
> > > > Minimum resolution is 64 x 64, maximum 8192 x 8192.
> > > > The alignment requirements for the resolution depend on the
> > > > format,
> > > > multiple of 16 resolutions should work for all formats.
> > > > 
> > > > v4l2-compliance tests are passing, including the
> > > > streaming tests, "v4l2-compliance -s"
> > > > 
> > > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> > > > ---
> > > > Changes in v7:
> > > >   Add print_mxc_buf() to replace print_buf_preview() and
> > > > print_nbuf_to_eoi(),
> > > >   and inside, use the print_hex_dump() from printk.h, also,
> > > > print
> > > > all the planes.
> > > > 
> > > >  drivers/media/platform/Kconfig                |    2 +
> > > >  drivers/media/platform/Makefile               |    1 +
> > > >  drivers/media/platform/imx-jpeg/Kconfig       |   10 +
> > > >  drivers/media/platform/imx-jpeg/Makefile      |    3 +
> > > >  drivers/media/platform/imx-jpeg/mxc-jpeg-hw.c |  168 ++
> > > >  drivers/media/platform/imx-jpeg/mxc-jpeg-hw.h |  140 +
> > > >  drivers/media/platform/imx-jpeg/mxc-jpeg.c    | 2289
> > > > +++++++++++++++++
> > > >  drivers/media/platform/imx-jpeg/mxc-jpeg.h    |  184 ++
> > > >  8 files changed, 2797 insertions(+)
> > > >  create mode 100644 drivers/media/platform/imx-jpeg/Kconfig
> > > >  create mode 100644 drivers/media/platform/imx-jpeg/Makefile
> > > >  create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg-
> > > > hw.c
> > > >  create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg-
> > > > hw.h
> > > >  create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg.c
> > > >  create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg.h
> > > 
> > > One high-level comment: why introduce the driver in patch 3/9 and
> > > then
> > > change it again in 9/9? I would very much prefer to have just a
> > > single
> > > patch that adds this driver, i.e. merge patch 3 and 9 into a
> > > single
> > > patch.
> > 
> > I can squash patch 9 into patch 3, but please note that patch 9
> > depends
> > on jpeg helper patches 6,7,8, so these patches will also have to
> > move
> > before patch 3. Let me know yout thought and I'll do this in v9, in
> > v8
> > version I only addressed the rest of your feedback and some more
> > from
> > Philipp.
> 
> Yes, just move the jpeg helper to earlier in the patch series.
> 
> > 
> > > 
> 
> <snip>
> 
> > > > +     /* fix colorspace information to sRGB for both output &
> > > > capture */
> > > > +     pix_mp->colorspace = V4L2_COLORSPACE_SRGB;
> > > > +     pix_mp->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > > > +     pix_mp->xfer_func = V4L2_XFER_FUNC_SRGB;
> > > > +     pix_mp->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > 
> > > So YUV formats are expected to be full range as well? Both when
> > > encoding
> > > and decoding?
> > 
> > I set the colorspace information like that based on this comment:
> >       /*
> >        * Effectively shorthand for V4L2_COLORSPACE_SRGB,
> > V4L2_YCBCR_ENC_601
> >        * and V4L2_QUANTIZATION_FULL_RANGE. To be used for (Motion-
> > )JPEG.
> >        */
> >       V4L2_COLORSPACE_JPEG          = 7,
> > 
> 
> Inside a JPEG the YUV quantization is using full range. But when you
> *decode* a JPEG the YUV quantization in the raw decoded image is
> normally limited range again (the default for YUV).
> 
> It depends on what this decoder does: most will decode to limited
> range YUV, some decode to full range YUV (in which case this code
> would be
> correct), and some support both.

Experimentally, I saw the decoder outputs full-range values, but I was
not sure about limited-range support, so I asked our IP owner, and I
got this answer:
"The decoder does not alter the range of the data in any way. 
It outputs the same full or limited range data that was given to the
encoder."

So, surely our encoder/decoder cannot change the range of the samples,
but it could process both full or half range.
I was thinking about a possibility to allow a half-range streams
scenario (half range raw format -> encoder -> jpeg with half range???
-> decoder -> half range raw format).
That could be achieved by allowing user to choose (specify) the
quantization for the output stream, and the driver would set the same
for the capture stream, but this would result in a JPEG with half range
for the capture stream.
So, you mentioned inside JPEG the YUV quantization is using full range,
experience confirms it (I decoded a jpeg produced with gimp and one
with ms-paint, and I saw full range values), and v4l2-compliance fails
if the pixel format is JPEG and quantization is not full.
I'm not sure if the standard allows half-range JPEG (it would be a
subset of full-range, so theoretically possible).

So, I will just add a comment to clarify the mxc-jpeg driver will
always use full-range.

The mxc-jpeg driver will not support CSC, therefor it is not setting
V4L2_FMT_FLAG_CSC_COLORSPACE in v4l2_fmtdesc during enumeration.
So, the application cannot request a specific colorspace for the
capture stream. No change needed here.

Let me know if this sounds ok, and I'll send v9 with the added coment
and the squash.

Thanks,
Mirela

> 
> In the latter case you want to support the V4L2_PIX_FMT_FLAG_SET_CSC
> flag:
> 
>