RE: [EXT] Re: [PATCH v8 04/15] media:Add v4l2 event codec_error and skip

[Ming Qian <ming.qian@nxp.com>]

> -----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 <linux-imx@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> 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 :
> > 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 <shijie.qin@nxp.com>
> > Signed-off-by: Zhou Peng <eagle.zhou@nxp.com>
> > ---
> >  .../userspace-api/media/v4l/vidioc-dqevent.rst       | 12 ++++++++++++
> >  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,
> > +     usually it can be replaced by a POLLERR event, but in some cases, the
> POLLERR
> > +     may cause the application to exit, but this event can allow the
> application to
> > +     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.
> 

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.
    In my opinion, setting bytesused to 0 means eos, and as you say, it may confuse some existing userspace.
    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


> > +    * - ``V4L2_EVENT_SKIP``
> > +      - 8
> > +      - This event is triggered when one frame is decoded, but it won't be
> outputed
> > +     to the display. So the application can't get this frame, and the input
> frame count
> > +     is dismatch with the output frame count. And this evevt is telling the
> client to
> > +     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 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.
    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.
    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 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
    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

> 
> >      * - ``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 */
>