by Hans Verkuil

[permalink] [raw]

Subject: Re: [EXT] Re: [PATCH v4 00/11] Add V4L2 driver for i.MX8 JPEG Encoder/Decoder

On 04/11/2020 14:27, Mirela Rabulea wrote:
> Hi Hans,
>
> On Wed, 2020-11-04 at 12:55 +0100, Hans Verkuil wrote:
>>
>> So please recompile v4l2-compliance from the git repo and retest. If
>> you find new failures, then you probably need to post a v5, otherwise
>> it is enough to reply to this with the output of the updated v4l2-
>> compliance.
>>
>>
>
> I was unsure if the latest of v4l2-compliance is stable.

It's always kept in sync with the media_tree master branch.

The version that distros have is always much too old and should never
be used when testing drivers.

> This commit (which is not part of stable 1.20), got me a little
> confused:
>
> commit e50041186be9f69dd94b64fb924115201726e72a
> Author: Hans Verkuil <[email protected]>
> Date: Thu Jul 16 13:41:37 2020 +0200
>
> v4l2-compliance: fix colorspace checks for JPEG codecs
>
> The colorspace handling for JPEG encoders/decoders is quite
> different from other m2m devices since the colorspace of a
> compressed JPEG file is effectively fixed to sRGB. So the
> typical passthrough of colorspace information from output
> to capture is not valid, instead the colorspace information is
> (mostly) fixed.
>
> And it somehow contradicts(?) with Philipp's latest commits on coda
> jpeg driver:
>
> commit 1e3e2a9ac40ad4d11699a49a1bbbf40cd8d4c8bd
> Author: Philipp Zabel <[email protected]>
> Date: Fri Jun 29 08:46:46 2018 -0400
>
> media: coda: jpeg: allow non-JPEG colorspace
>
> The hardware codec is not colorspace aware. We should trust
> userspace to
> set the correct colorimetry information on the OUTPUT queue and
> mirror
> the exact same setting on the CAPTURE queue.
>
> There is no reason to restrict colorspace to JPEG for JPEG images,
> if
> userspace injects the correct colorspace information into the JPEG
> headers after encoding.
>
> Fixes: b14ac545688d ("[media] coda: improve colorimetry handling")
>
> I agree on the statement "The hardware codec is not colorspace aware",
> by that I mean this colorspace will not translate into any setting sent
> to the hardware. The question is, how the driver is expected to bahave?

This was never really well defined. Basically the JPEG standard doesn't
store colorimetry as metadata, instead it is understood to be sRGB colorimetry.

So if you take what a HW JPEG encoder creates and want to show it on another
device, then it will be interpreted as sRGB. Now if userspace adds some JPEG
extension where it declares the colorimetry to be something else, then that
is fine, but that's out of scope of a HW JPEG encoder driver, IMHO.

I suspect that the coda patch was actually trying to make coda behave with
an older version of v4l2-compliance where a JPEG codec was tested in the
same way as a H264 codec. Later we realized that that didn't make sense for
JPEG codecs and the test was changed. But now coda fails on that test.

> Once I clarify on this, I'll include a fix in v5.

I hope this helps.

Hans

>
> I'll post below, for reference, the full log on latest from master.
>
> Thanks,
> Mirela
>
> root@imx8qxpmek:/unit_tests/JPEG# ./v4l2-compliance-master -d
> /dev/video0 -s
> v4l2-compliance 1.21.0-4675, 64 bits, 64-bit time_t
> v4l2-compliance SHA: b84569db756a 2020-10-23 12:30:38
>
> Compliance test for mxc-jpeg decode device /dev/video0:
>
> Driver Info:
> Driver name : mxc-jpeg decode
> Card type : mxc-jpeg decoder
> Bus info : platform:58400000.jpegdec
> Driver version : 5.10.0
> Capabilities : 0x84204000
> Video Memory-to-Memory Multiplanar
> Streaming
> Extended Pix Format
> Device Capabilities
> Device Caps : 0x04204000
> Video Memory-to-Memory Multiplanar
> Streaming
> Extended Pix Format
> Detected JPEG Decoder
>
> Required ioctls:
> test VIDIOC_QUERYCAP: OK
>
> Allow for multiple opens:
> test second /dev/video0 open: OK
> test VIDIOC_QUERYCAP: OK
> test VIDIOC_G/S_PRIORITY: OK
> test for unlimited opens: OK
>
> test invalid ioctls: OK
> Debug ioctls:
> test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
> test VIDIOC_LOG_STATUS: OK (Not Supported)
>
> Input ioctls:
> test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> test VIDIOC_ENUMAUDIO: OK (Not Supported)
> test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
> test VIDIOC_G/S_AUDIO: OK (Not Supported)
> Inputs: 0 Audio Inputs: 0 Tuners: 0
>
> Output ioctls:
> test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> Outputs: 0 Audio Outputs: 0 Modulators: 0
>
> Input/Output configuration ioctls:
> test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
> test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> test VIDIOC_G/S_EDID: OK (Not Supported)
>
> Control ioctls:
> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
> test VIDIOC_QUERYCTRL: OK (Not Supported)
> test VIDIOC_G/S_CTRL: OK (Not Supported)
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> Standard Controls: 0 Private Controls: 0
>
> Format ioctls:
> test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> test VIDIOC_G/S_PARM: OK (Not Supported)
> test VIDIOC_G_FBUF: OK (Not Supported)
> test VIDIOC_G_FMT: OK
> test VIDIOC_TRY_FMT: OK
> fail: v4l2-test-formats.cpp(810):
> fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB
> test VIDIOC_S_FMT: FAIL
> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> test Cropping: OK (Not Supported)
> test Composing: OK (Not Supported)
> test Scaling: OK
>
> Codec ioctls:
> test VIDIOC_(TRY_)ENCODER_CMD: OK
> test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> test VIDIOC_(TRY_)DECODER_CMD: OK
>
> Buffer ioctls:
> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> test VIDIOC_EXPBUF: OK
> test Requests: OK (Not Supported)
>
> Test input 0:
>
> Streaming ioctls:
> test read/write: OK (Not Supported)
> test blocking wait: OK
> Video Capture Multiplanar: Captured 58 buffers
> test MMAP (no poll): OK
> Video Capture Multiplanar: Captured 58 buffers
> test MMAP (select): OK
> Video Capture Multiplanar: Captured 58 buffers
> test MMAP (epoll): OK
> test USERPTR (no poll): OK (Not Supported)
> test USERPTR (select): OK (Not Supported)
> test DMABUF: Cannot test, specify --expbuf-device
>
> Total for mxc-jpeg decode device /dev/video0: 52, Succeeded: 51,
> Failed: 1, Warnings: 0
>
>
> root@imx8qxpmek:/unit_tests/JPEG# ./v4l2-compliance-master -d
> /dev/video1 -s
> v4l2-compliance 1.21.0-4675, 64 bits, 64-bit time_t
> v4l2-compliance SHA: b84569db756a 2020-10-23 12:30:38
>
> Compliance test for mxc-jpeg decode device /dev/video1:
>
> Driver Info:
> Driver name : mxc-jpeg decode
> Card type : mxc-jpeg decoder
> Bus info : platform:58450000.jpegenc
> Driver version : 5.10.0
> Capabilities : 0x84204000
> Video Memory-to-Memory Multiplanar
> Streaming
> Extended Pix Format
> Device Capabilities
> Device Caps : 0x04204000
> Video Memory-to-Memory Multiplanar
> Streaming
> Extended Pix Format
> Detected JPEG Encoder
>
> Required ioctls:
> test VIDIOC_QUERYCAP: OK
>
> Allow for multiple opens:
> test second /dev/video1 open: OK
> test VIDIOC_QUERYCAP: OK
> test VIDIOC_G/S_PRIORITY: OK
> test for unlimited opens: OK
>
> test invalid ioctls: OK
> Debug ioctls:
> test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
> test VIDIOC_LOG_STATUS: OK (Not Supported)
>
> Input ioctls:
> test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> test VIDIOC_ENUMAUDIO: OK (Not Supported)
> test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
> test VIDIOC_G/S_AUDIO: OK (Not Supported)
> Inputs: 0 Audio Inputs: 0 Tuners: 0
>
> Output ioctls:
> test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> Outputs: 0 Audio Outputs: 0 Modulators: 0
>
> Input/Output configuration ioctls:
> test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
> test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> test VIDIOC_G/S_EDID: OK (Not Supported)
>
> Control ioctls:
> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
> test VIDIOC_QUERYCTRL: OK (Not Supported)
> test VIDIOC_G/S_CTRL: OK (Not Supported)
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> Standard Controls: 0 Private Controls: 0
>
> Format ioctls:
> test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> test VIDIOC_G/S_PARM: OK (Not Supported)
> test VIDIOC_G_FBUF: OK (Not Supported)
> test VIDIOC_G_FMT: OK
> test VIDIOC_TRY_FMT: OK
> fail: v4l2-test-formats.cpp(810):
> fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB
> test VIDIOC_S_FMT: FAIL
> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> test Cropping: OK (Not Supported)
> test Composing: OK (Not Supported)
> test Scaling: OK (Not Supported)
>
> Codec ioctls:
> test VIDIOC_(TRY_)ENCODER_CMD: OK
> test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> test VIDIOC_(TRY_)DECODER_CMD: OK
>
> Buffer ioctls:
> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> test VIDIOC_EXPBUF: OK
> test Requests: OK (Not Supported)
>
> Test input 0:
>
> Streaming ioctls:
> test read/write: OK (Not Supported)
> test blocking wait: OK
> Video Capture Multiplanar: Captured 58 buffers
> test MMAP (no poll): OK
> Video Capture Multiplanar: Captured 58 buffers
> test MMAP (select): OK
> Video Capture Multiplanar: Captured 58 buffers
> test MMAP (epoll): OK
> test USERPTR (no poll): OK (Not Supported)
> test USERPTR (select): OK (Not Supported)
> test DMABUF: Cannot test, specify --expbuf-device
>
> Total for mxc-jpeg decode device /dev/video1: 52, Succeeded: 51,
> Failed: 1, Warnings: 0
>

2020-11-04 16:06:08

Hi Laurentiu,

On Mon, 2020-11-02 at 18:20 +0200, Laurentiu Palcu wrote:
> Hi Mirela,
>
> I wanted to give it a more in-depth look but I then saw that patch 11
> deletes a lot of code from this file. So, the review on the deleted
> parts would be pointless... :/
>
> I suggest you squash 4 and 11 together.

Sorry about that, I refrained from squashing 4 & 11 for 2 reasons:

1. On patch 4 I did not make significant changes since previous version
(just enough to keep it compiling) I hoped it will be easier to review
like this, tried to explain in cover letter.

2. After using the jpeg helpers, the mxc_jpeg_parse() is somewhere
between 2-8% slower, depending on the measuring method, so I was
thinking it would be nice to have the previous method in the history,
as a reference. Now, I have done more measurements on the overall
impact, an it is insignificant, ~0.01% for a streaming case (1 1080p
RGB24 buffer enqueued 1000 times).

I can definitely squash patch 7 into 4, together with other changes
from this version review. I'm waiting for more opinions about squashing
11 into 4.

>
> > v4l2-compliance tests are passing, including the
> > streaming tests, "v4l2-compliance -s"
> >
> > V3 Replaced GRABBER
>
> What does this mean?

Removed, my bad, that info was added in cover letter.

> + *
> > + * A module parameter is available for debug purpose
> > (jpeg_tracing), to enable
> > + * it, enable dynamic debug for this module and:
> > + * echo 1 > /sys/module/mxc_jpeg_encdec/parameters/jpeg_tracing
>
> It looks like this it's trying to achieve the same thing that's
> already
> offered by v4l2_dbg() and the 'debug' module parameter. The advantage
> of
> the latter is that you don't need to recompile the kernel to enable
> dynamic debug... Maybe it's worth reusing it?

I replaced jpeg_tracing module parameter with debug module parameter in
the next version, so, it will be shared with what v4l2_dbg is using.

> > +0xB7, 0xB8, 0xB9, 0xBA, 0xC2, 0xC3, 0xC4,
> > +0xC5, 0xC6, 0xC7, 0xC8, 0xC9, 0xCA, 0xD2,
> > +0xD3, 0xD4, 0xD5, 0xD6, 0xD7, 0xD8, 0xD9,
> > +0xDA, 0xE2, 0xE3, 0xE4, 0xE5, 0xE6, 0xE7,
> > +0xE8, 0xE9, 0xEA, 0xF2, 0xF3, 0xF4, 0xF5,
> > +0xF6, 0xF7, 0xF8, 0xF9, 0xFA};
> > +static const unsigned char jpeg_dri[] = {0xFF, 0xDD,
> > +0x00, 0x04, 0x00, 0x20};
> > +static const unsigned char jpeg_sos_maximal[] = {0xFF, 0xDA,
> > +0x00, 0x0C, 0x04, 0x01, 0x00, 0x02, 0x11, 0x03,
> > +0x11, 0x04, 0x11, 0x00, 0x3F, 0x00,};
> > +static const unsigned char jpeg_eoi[] = {0xFF, 0xD9};
>
> These data blocks above should be properly indented, one tab to the
> right.

Done, for the next version.

> > +/* Print Four-character-code (FOURCC) */
> > +static char *fourcc_to_str(u32 format)
> > +{
> > + char *buf = kmalloc(32, GFP_KERNEL);
>
> I'm not sure this is worth it. Either you make it a static array:
>
> static char buf[5];
>
> And return a pointer to it, unless this is called from different
> contexts. Or you could make the caller pass a pointer to the buffer.
> Using kmalloc() every time you want to print 4 characters might
> introduce some unnecessary overhead if this is called too often.
>
> However, my sugestion is to get rid of this fourcc_to_str() helper
> completely and print the format directly where you need it. Here's a
> list of places where this is done, in case you have second thoughts:
>
> git grep "$v4l2_dbg\|pr_cont\|dev_$.*%c%c%c%c"
>

Removed fourcc_to_str(), used %c%c%c%c instead, the amount of lines of
code is similar, so, it's ok, no loss.

> > + struct mxc_jpeg_sof sof, *psof = 0;
> > + struct mxc_jpeg_sos *psos = 0;
> > + u8 byte, *next = 0;
>
> Don't use 0 for NULL pointers. Use NULL. :)

Done, it will be in the next version.

>
> > + enum mxc_jpeg_image_format img_fmt;
> > + u32 fourcc;
> > +
> > + memset(&sof, 0, sizeof(struct mxc_jpeg_sof));
> > + stream.addr = src_addr;
> > + stream.end = size;
> > + stream.loc = 0;
> > + *dht_needed = true;
> > + while (notfound) {
> > + byte = get_byte(&stream);
> > + if (byte == -1)
>
> byte is u8. The above doesn't make much sense.

This is handled differently now in the upstream jpeg helpers
(v4l2_jpeg_parse_header() and jpeg_next_marker()).
However, in the above, there is a problem, the end of the stream is not
detected properly. I'll modify get_byte to return int, not u8, because
-1 is for the end of stream, and 0xFF is
for markers (in u8 representation both were 0xFF). This passed
undetected because, for a valid jpeg the parsing was done only up to
SOS. So, a problematic case would be a corrupted jpeg, with SOI, but
without SOS. I'll try to add some corrupted jpegs in my testsuite.

Thanks,
Mirela