Re: [EXT] Re: [PATCH v8 03/15] media:Add v4l2 buf flag codec data.

[Nicolas Dufresne <nicolas@ndufresne.ca>]

Le jeudi 09 septembre 2021 à 02:20 +0000, Ming Qian a écrit :
> > -----Original Message-----
> > From: Nicolas Dufresne [mailto:nicolas@ndufresne.ca]
> > Sent: Wednesday, September 8, 2021 9:15 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 03/15] media:Add v4l2 buf flag codec data.
> > 
> > Caution: EXT Email
> > 
> > Hi Ming,
> > 
> > thanks for the patch. I'm doing a first pass review of the new APIs you are
> > introducing.
> > 
> > Le mardi 07 septembre 2021 à 17:49 +0800, Ming Qian a écrit :
> > > In some decoing scenarios, application may queue a buffer that only
> > > contains codec config data, and the driver needs to know whether is it
> > > a frame or not.
> > > So we add a buf flag to tell this case.
> > > 
> > > 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>
> > > ---
> > >  Documentation/userspace-api/media/v4l/buffer.rst | 7 +++++++
> > >  include/uapi/linux/videodev2.h                   | 1 +
> > >  2 files changed, 8 insertions(+)
> > > 
> > > diff --git a/Documentation/userspace-api/media/v4l/buffer.rst
> > > b/Documentation/userspace-api/media/v4l/buffer.rst
> > > index e991ba73d873..11013bcf8a41 100644
> > > --- a/Documentation/userspace-api/media/v4l/buffer.rst
> > > +++ b/Documentation/userspace-api/media/v4l/buffer.rst
> > > @@ -607,6 +607,13 @@ Buffer Flags
> > >       the format. Any subsequent call to the
> > >       :ref:`VIDIOC_DQBUF <VIDIOC_QBUF>` ioctl will not block anymore,
> > >       but return an ``EPIPE`` error code.
> > > +    * .. _`V4L2-BUF-FLAG-CODECCONFIG`:
> > > +
> > > +      - ``V4L2_BUF_FLAG_CODECCONFIG``
> > > +      - 0x00200000
> > > +      - The buffer only contains codec config data, eg. sps and pps.
> > > +    Applications can set this bit when ``type`` refers to an output
> > > +    stream, this flag is usually used by v4l2 decoder.
> > 
> > Currently, the bottom line is that all decoders needs a frame or field
> > accompanied with the headers (only Annex B. being defined and supported for
> > now). Decoders can be more flexible (and some are). The documentation here
> > is not clear enough, remember that we must not break compatibility.
> > 
> > I think you have to clarify the intention. This flag exist in OMX and have
> > been
> > source of confusion and inter-operability since the start.
> > 
> > - If this flag is entirely optional, and is just an optimization, then
> > adding it this
> > way is fine, but the documentation should be updated.
> > 
> > - If this flag is required only when the header is split, then this flag
> > need to be
> > accompanied with a V4L2_BUF_CAP_SUPPORTS_SPLIT_CODECCONFIG (or
> > similar name, shorter could be nice). This is so that userspace can detect
> > if that
> > feature is supported or not.
> > 
> > - If having split codecconfig is strictly needed for your driver, then this
> > flag is
> > not the proper solution. The only solution I'd see for that, would be to use
> > something else then V4L2_PIX_FMT_H264 so that existing userspace are not
> > tricked into trying to use your driver the wrong way. I think such header
> > could
> > make sense with H264_NO_SC (though not super clear what this is exactly,
> > it's
> > not really used), or a clearer new format H264_AVCC/AVCC3
> 
> Hi Nicolas,
> 
>     This flag is optional, and in fact, our driver doesn't want to receive a
> splited header,
> It's best that every buffer contains a frame.
>     But in some case, the client may enqueue some buffer that only contains
> the header data without any frame data.
> And our driver need to know this case, for this type of buffer, we have two
> cases to handle.
>     1. ignore the timestamp.
> 	2. the amphion decoder needs a ring buffer for decoding, driver will
> copy the coded data to the ring buffer, and update the write pointer, then
> pass a frame count to firmware, firmware will use the frame count to determine
> whether starting decoding a frame or not, if the frame count is incorrect, it
> may led to vpu hang. So for this type of buffer, we won't increase the frame
> count.
> 
>     The flag is required only when the header is split, I agree with you that
> it's better to add a capability flag. Actually our driver doesn't want meet
> this case, but this situation does exist in some applications, I add this flag
> to help handle it.
>     Currently we meet this case in android platform.

Thanks, that clarify were this comes from. Perhaps you could drop this from your
initial patchset, and we can handle this separatly ?

I remain a bit worried about the possible VPU hang, as the door is still wide
open to DoS on this HW from random streams. Have you considered raising this
issue to Amphion ? Perhaps there is a different way you could deal with partial
frames ?

> 
> > 
> > >      * .. _`V4L2-BUF-FLAG-REQUEST-FD`:
> > > 
> > >        - ``V4L2_BUF_FLAG_REQUEST_FD``
> > > diff --git a/include/uapi/linux/videodev2.h
> > > b/include/uapi/linux/videodev2.h index 167c0e40ec06..5bb0682b4a23
> > > 100644
> > > --- a/include/uapi/linux/videodev2.h
> > > +++ b/include/uapi/linux/videodev2.h
> > > @@ -1119,6 +1119,7 @@ static inline __u64 v4l2_timeval_to_ns(const
> > struct timeval *tv)
> > >  #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE         0x00010000
> > >  /* mem2mem encoder/decoder */
> > >  #define V4L2_BUF_FLAG_LAST                   0x00100000
> > > +#define V4L2_BUF_FLAG_CODECCONFIG            0x00200000
> > >  /* request_fd is valid */
> > >  #define V4L2_BUF_FLAG_REQUEST_FD             0x00800000
> > > 
> > 
>