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 <[email protected]>
Signed-off-by: Shijie Qin <[email protected]>
Signed-off-by: Zhou Peng <[email protected]>
---
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.
* .. _`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
--
2.32.0
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 <[email protected]>
> Signed-off-by: Shijie Qin <[email protected]>
> Signed-off-by: Zhou Peng <[email protected]>
> ---
> 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
> * .. _`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
>
> -----Original Message-----
> From: Nicolas Dufresne [mailto:[email protected]]
> Sent: Wednesday, September 8, 2021 9:15 PM
> To: Ming Qian <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> dl-linux-imx <[email protected]>; Aisheng Dong <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]
> 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 <[email protected]>
> > Signed-off-by: Shijie Qin <[email protected]>
> > Signed-off-by: Zhou Peng <[email protected]>
> > ---
> > 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.
>
> > * .. _`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
> >
>
Le jeudi 09 septembre 2021 à 02:20 +0000, Ming Qian a écrit :
> > -----Original Message-----
> > From: Nicolas Dufresne [mailto:[email protected]]
> > Sent: Wednesday, September 8, 2021 9:15 PM
> > To: Ming Qian <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected]
> > Cc: [email protected]; [email protected]; [email protected];
> > dl-linux-imx <[email protected]>; Aisheng Dong <[email protected]>;
> > [email protected]; [email protected];
> > [email protected]; [email protected]
> > 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 <[email protected]>
> > > Signed-off-by: Shijie Qin <[email protected]>
> > > Signed-off-by: Zhou Peng <[email protected]>
> > > ---
> > > 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
> > >
> >
>
> -----Original Message-----
> From: Nicolas Dufresne [mailto:[email protected]]
> Sent: Friday, September 10, 2021 3:41 AM
> To: Ming Qian <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> dl-linux-imx <[email protected]>; Aisheng Dong <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [EXT] Re: [PATCH v8 03/15] media:Add v4l2 buf flag codec data.
>
> Caution: EXT Email
>
> Le jeudi 09 septembre 2021 à 02:20 +0000, Ming Qian a écrit :
> > > -----Original Message-----
> > > From: Nicolas Dufresne [mailto:[email protected]]
> > > Sent: Wednesday, September 8, 2021 9:15 PM
> > > To: Ming Qian <[email protected]>; [email protected];
> > > [email protected]; [email protected]; [email protected]
> > > Cc: [email protected]; [email protected];
> > > [email protected]; dl-linux-imx <[email protected]>; Aisheng Dong
> > > <[email protected]>; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]
> > > 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 <[email protected]>
> > > > Signed-off-by: Shijie Qin <[email protected]>
> > > > Signed-off-by: Zhou Peng <[email protected]>
> > > > ---
> > > > 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 ?
OK, I will drop it from this patchset, maybe we can submit a separate patch in the future.
>
> 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 ?
>
Yes, I agree with you, it's a hardware limitation that the vpu may stall at decoding if there is not enough frame data in the stream buffer.
We have discussed with amphion many times and made a series of workaround in the firmware.
But the root cause is still not fixed.
And now it is almost impossible to improve it as amphion has been acquired, and they're not focused on that anymore
> >
> > >
> > > > * .. _`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
> > > >
> > >
> >
>