Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <fffd24d3374ecb2fbfafa9b85fa0ef8012fc7efa.camel@ndufresne.ca>
Subject: Re: [PATCH v8 04/15] media:Add v4l2 event codec_error and skip
From:   Nicolas Dufresne <nicolas@ndufresne.ca>
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, linux-imx@nxp.com, aisheng.dong@nxp.com,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Date:   Wed, 08 Sep 2021 09:32:38 -0400
In-Reply-To: <647f84c1e7c2a48d6492d38fa4f06586235500b8.1631002447.git.ming.qian@nxp.com>
References: <cover.1631002447.git.ming.qian@nxp.com>
         <647f84c1e7c2a48d6492d38fa4f06586235500b8.1631002447.git.ming.qian@nxp.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.4 (3.40.4-1.fc34) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.

> +    * - ``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).

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