On 10/03/2021 13:33, Mirela Rabulea wrote: > 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@xxxxxxx> >>>>> >>>>> 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@xxxxxxx> >>>>> --- >>>>> 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. Yes, just add a comment that the JPEG codec always uses full range YUV for the uncompressed frames. Regards, Hans > > Thanks, > Mirela > >> >> In the latter case you want to support the V4L2_PIX_FMT_FLAG_SET_CSC >> flag: >> >> >
