Le jeudi 09 septembre 2021 à 02:20 +0000, Ming Qian a écrit : > > -----Original Message----- > > From: Nicolas Dufresne [mailto:nicolas@xxxxxxxxxxxx] > > Sent: Wednesday, September 8, 2021 9:15 PM > > To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx; > > shawnguo@xxxxxxxxxx; robh+dt@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx > > Cc: hverkuil-cisco@xxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; > > dl-linux-imx <linux-imx@xxxxxxx>; Aisheng Dong <aisheng.dong@xxxxxxx>; > > linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > > 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@xxxxxxx> > > > Signed-off-by: Shijie Qin <shijie.qin@xxxxxxx> > > > Signed-off-by: Zhou Peng <eagle.zhou@xxxxxxx> > > > --- > > > 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 > > > > > >
