-----Original Message-----
From: Nicolas Dufresne [mailto:nicolas@ndufresne.ca]
Sent: Friday, September 10, 2021 3:54 AM
To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
shawnguo@kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de
Cc: hverkuil-cisco@xs4all.nl; kernel@pengutronix.de; festevam@gmail.com;
dl-linux-imx [off-list ref]; Aisheng Dong [off-list ref];
linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
Subject: Re: [EXT] Re: [PATCH v8 04/15] media:Add v4l2 event codec_error and
skip

Caution: EXT Email

Le jeudi 09 septembre 2021 à 03:13 +0000, Ming Qian a écrit :

quoted

quoted

-----Original Message-----
From: Nicolas Dufresne [mailto:nicolas@ndufresne.ca]
Sent: Wednesday, September 8, 2021 9:33 PM
To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
shawnguo@kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de
Cc: hverkuil-cisco@xs4all.nl; kernel@pengutronix.de;
festevam@gmail.com; dl-linux-imx [off-list ref]; Aisheng Dong
[off-list ref]; linux-media@vger.kernel.org;
linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
linux-arm-kernel@lists.infradead.org
Subject: [EXT] Re: [PATCH v8 04/15] media:Add v4l2 event codec_error
and skip

Caution: EXT Email

Hi Ming,

more API only review.

Le mardi 07 septembre 2021 à 17:49 +0800, Ming Qian a écrit :

quoted

The codec_error event can tell client that there are some error
occurs in the decoder engine.

The skip event can tell the client that there are a frame has been
decoded, but it won't be outputed.

Signed-off-by: Ming Qian <ming.qian@nxp.com>
Signed-off-by: Shijie Qin <redacted>
Signed-off-by: Zhou Peng <eagle.zhou@nxp.com>
---
 .../userspace-api/media/v4l/vidioc-dqevent.rst       | 12

++++++++++++

quoted

quoted

quoted

 include/uapi/linux/videodev2.h                       |  2 ++
 2 files changed, 14 insertions(+)

diff --git
a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
index 6eb40073c906..87d40ad25604 100644

--- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst

@@ -182,6 +182,18 @@ call.
      the regions changes. This event has a struct
      :c:type:`v4l2_event_motion_det`
      associated with it.
+    * - ``V4L2_EVENT_CODEC_ERROR``
+      - 7
+      - This event is triggered when some error occurs inside the
+ codec

engine,

quoted

+     usually it can be replaced by a POLLERR event, but in some
+ cases,
the

POLLERR

quoted

+     may cause the application to exit, but this event can allow
+ the

application to

quoted

+     handle the codec error without exiting.

Events are sent to userspace in a separate queue from the VB2 queue.
Which means it's impossible for userspace to know where this error
actually took place.
Userspace may endup discarding valid frames from the VB queue, as it
does not know which one are good, and which one are bad.

There is likely a bit of spec work to be done here for non-fatal
decode errors, but I think the right approach is to use
V4L2_BUF_FLAG_ERROR. What we expect from decoders is that for each
frame, a CAPTURE buffer is assigned.
If
decoding that frame was not possible but the error is recoverable
(corrupted bitstream, missing reference, etc.), then the failing
frame get marked with FLAG_ERROR and decoding continues as usual.

What isn't documented is that you can set bytesused to 0, meaning
there is nothing useful in that frame, or a valid bytesused when you
know only some blocks are broken (e.g. missing 1 ref). Though,
GStreamer might be the only implementation of that, and byteused 0 may

confuse some existing userspace.

quoted

quoted

Hi Nicolas,
    We don't use this event to tell userspace which frame is broken.
Actually it tries to tell userspace that the decoder is abnormal and
there will be no more frames output. The usersapce shouldn't wait, it
can reset the decoder instance if it wants to continue decoding more
frames, or it can exit directly.
    Usually there will no capture buffer can be dequeued, so we can't
set a V4L2_BUF_FLAG_ERROR flag to a capture buffer.

That is not logical, if userspace asked to decode a buffer, but didn't queue back
any CAPTURE buffer, you are expected to just sit there and wait.

quoted

    In my opinion, setting bytesused to 0 means eos, and as you say,
it may confuse some existing userspace.

Byteused 0 only mean EOS for one specific driver, MFC. That behaviour was
kept to avoid breaking existing userspace. In fact, you have to opt in, the
framework will prevent you from using it for that purpose.

quoted

    I think it can be replaced by POLLERR in most of case, but we meet
some applications who prefer to use this event instead of pollerr

In general, recoverable errors should be handled without the need for
userspace to reset. This looks more like you have a bug in your error handling
and deffer it to userspace. Most userspace will just abort and report to users, I
doubt this is really what you expect.

What matters for recoverable errors is that you keep consuming OUTPUT
buffers.
And userspace should be happy with never getting anything from the CAPTURE
till the propblem was recovered by the driver. Of course, userspace should
probably garbage collect the metadata it might be holding, chromium does
that with a leaky queue of 16 metadata buffer notably

My recommandation would be to drop this for now, and just try to not stall on
errors (or make it a hard failure for now, pollerr, or ioctl errors).

OK, I agree with you, I will drop it

quoted

quoted

quoted

+    * - ``V4L2_EVENT_SKIP``
+      - 8
+      - This event is triggered when one frame is decoded, but it
+ won't
be

outputed

quoted

+     to the display. So the application can't get this frame, and
+ the
input

frame count

quoted

+     is dismatch with the output frame count. And this evevt is
+ telling
the

client to

quoted

+     handle this case.

Similar to my previous comment, this event is flawed, since
userspace cannot know were the skip is located in the queued
buffers. Currently, all decoders are mandated to support
V4L2_BUF_FLAG_TIMESTAMP_COPY. The timestamp must NOT be

interpreted

quoted

quoted

by the driver and must be reproduce as-is in the associated CAPTURE
buffer. It is possible to "garbage" collect skipped frames with this
method, though tedious.

An alternative, and I think it would be much nicer then this, would
be to use the v4l2_buffer.sequence counter, and just make it skip 1
on skips. Though, the down side is that userspace must also know how
to reorder frames (a driver job for stateless codecs) in order to
identify which frame was skipped. So this is perhaps not that
useful, other then knowing something was skipped in the past.

A third option would be to introduce V4L2_BUF_FLAG_SKIPPED. This way
the driver could return an empty payload (bytesused = 0) buffer with
this flag set, and the proper timestamp properly copied. This would
let the driver communicate skipped frames in real-time. Note that
this could break with existing userspace, so it would need to be
opted-in somehow (a control or some flags).

Hi Nicolas,
   The problem we meet is that userspace doesn't care which frame is
skipped, it just need to know that there are a frame is skipped, the
driver should promise the input frame count is equals to the output frame

count.

quoted

    Your first method is possible in theory, but we find the timestamp
may be unreliable, we meet many timestamp issues that userspace may
enqueue invalid timestamp or repeated timestamp and so on, so we can't

accept this solution.

The driver should not interpret the provided timestamp, so it should not be
able to say if the timestamp is valid or not, this is not the driver's task.

The driver task is to match the timestamp to the CAPTURE buffer (if that buffer
was produced), and reproduce it exactly.

quoted

    I think your second option is better. And there are only 1
question, we find some application prefer to use the V4L2_EVENT_EOS to
check the eos, not checking the empty buffer, if we use this method to
check skipped frame, the

Checking the empty buffer is a legacy method, only available in Samsung MFC
driver. The spec says that the last buffer should be flagged with _LAST, and any
further attempt to poll should unblock and DQBUF return EPIPE.

quoted

application should check empty buffer instead of V4L2_EVENT_EOS,
otherwise if the last frame is skipped, the application will miss it.
Of course this is not a problem, it just increases the complexity of
the userspace implementation

The EPIPE mechanism covers this issue, which we initially had with the LAST
flag.

quoted

    I don't think your third method is feasible, the reasons are as below
              1. usually the empty payload means eos, and as you say,
it may introduce confusion.
      2. The driver may not have the opportunity to return an empty
payload during decoding, in our driver, driver will pass the capture
buffer to firmware, and when some frame is skipped, the firmware won't
return the buffer, driver may not find an available capture buffer to
return to userspace.

   The requirement is that userspace need to match the input frame
count and output frame count. It doesn't care which frame is skipped,
so the V4L2_EVENT_SKIP is the easiest way for driver and userspace.
   If you think this event is really inappropriate, I prefer to adopt
your second option

Please, drop SKIP from you driver and this patchset and fix your draining
process handling to follow the spec. The Samsung OMX component is
irrelevant to mainline submission, the OMX code should be updated to follow
the spec.

OK, I'll drop it, and follow your second option that use the v4l2_buffer.sequence counter.

My previous statement about empty buffer was not appropriate.
I know the last buffer should be flagged with _LAST, and in most of case,
driver will append a empty buffer with _LAST flag.
But in userspace, I find some application will check the bytesused, if it's 0, the application will think it's eos
I checked the gstreamer v4l2 decoder, I found the following code:
    /* Legacy M2M devices return empty buffer when drained */
    if (size == 0 && GST_V4L2_IS_M2M (obj->device_caps))
        goto eos;
and I found the ffmpeg v4l2 decoder does the similar thing.

So I don't want to use empty buffer except the last buffer in the driver.

quoted

quoted

quoted

     * - ``V4L2_EVENT_PRIVATE_START``
       - 0x08000000
       - Base event number for driver-private events.

diff --git a/include/uapi/linux/videodev2.h

b/include/uapi/linux/videodev2.h index 5bb0682b4a23..c56640d42dc5
100644

--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h

@@ -2369,6 +2369,8 @@ struct v4l2_streamparm {
 #define V4L2_EVENT_FRAME_SYNC                        4
 #define V4L2_EVENT_SOURCE_CHANGE             5
 #define V4L2_EVENT_MOTION_DET                        6
+#define V4L2_EVENT_CODEC_ERROR                       7
+#define V4L2_EVENT_SKIP                              8
 #define V4L2_EVENT_PRIVATE_START             0x08000000

 /* Payload for V4L2_EVENT_VSYNC */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel