[PATCH] media: videobuf2: add V4L2_BUF_FLAG_HEADERS_ONLY flag

[PATCH] media: videobuf2: add V4L2_BUF_FLAG_HEADERS_ONLY flag

[Ming Qian]

By setting the V4L2_BUF_FLAG_HEADERS_ONLY flag,
hint the vb2 only contains stream header,
but does not contain any frame data.

This flag needs to be used when header mode is set to
V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE.

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
 Documentation/userspace-api/media/v4l/buffer.rst      | 11 +++++++++++
 .../userspace-api/media/v4l/ext-ctrls-codec.rst       | 10 +++++++---
 include/uapi/linux/videodev2.h                        |  2 ++
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/buffer.rst b/Documentation/userspace-api/media/v4l/buffer.rst
index 4638ec64db00..18a6f5fcc822 100644
--- a/Documentation/userspace-api/media/v4l/buffer.rst
+++ b/Documentation/userspace-api/media/v4l/buffer.rst
@@ -607,6 +607,17 @@ 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-HEADERS-ONLY`:
+
+      - ``V4L2_BUF_FLAG_HEADERS_ONLY``
+      - 0x00200000
+      - This flag may be set when the buffer only contains codec
+	header, but does not contain any frame data. Usually the codec
+	header is merged to the next idr frame, with the flag
+	``V4L2_BUF_FLAG_KEYFRAME``, but there is still some scenes that will
+	split the header and queue it separately. This flag can set only when
+	codec support V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE,
+	and the header mode is set to V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE
     * .. _`V4L2-BUF-FLAG-REQUEST-FD`:
 
       - ``V4L2_BUF_FLAG_REQUEST_FD``
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 6183f43f4d73..478b6af4205d 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1386,8 +1386,12 @@ enum v4l2_mpeg_video_intra_refresh_period_type -
     (enum)
 
 enum v4l2_mpeg_video_header_mode -
-    Determines whether the header is returned as the first buffer or is
-    it returned together with the first frame. Applicable to encoders.
+    Determines whether the header is returned as the first buffer
+    with flag V4L2_BUF_FLAG_HEADERS_ONLY or is
+    it returned together with the first frame.
+    Applicable to encoders and decoders.
+    If it's not implemented in a driver,
+    V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME is to be assumed,
     Possible values are:
 
 .. raw:: latex
@@ -1401,7 +1405,7 @@ enum v4l2_mpeg_video_header_mode -
     :stub-columns: 0
 
     * - ``V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE``
-      - The stream header is returned separately in the first buffer.
+      - The stream header is returned separately in the first buffer with the flag V4L2_BUF_FLAG_HEADERS_ONLY.
     * - ``V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME``
       - The stream header is returned together with the first encoded
 	frame.
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 5311ac4fde35..6fd96acd6080 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1131,6 +1131,8 @@ 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
+/* Buffer only contains codec header */
+#define V4L2_BUF_FLAG_HEADERS_ONLY		0x00200000
 /* request_fd is valid */
 #define V4L2_BUF_FLAG_REQUEST_FD		0x00800000
 
-- 
2.36.1

Re: [PATCH] media: videobuf2: add V4L2_BUF_FLAG_HEADERS_ONLY flag

[Hans Verkuil]

+CC Nicolas and Tomasz.

I would like some feedback for this patch.

On 12/07/2022 11:37, Ming Qian wrote:
> By setting the V4L2_BUF_FLAG_HEADERS_ONLY flag,
> hint the vb2 only contains stream header,
> but does not contain any frame data.
> 
> This flag needs to be used when header mode is set to
> V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE.
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
>  Documentation/userspace-api/media/v4l/buffer.rst      | 11 +++++++++++
>  .../userspace-api/media/v4l/ext-ctrls-codec.rst       | 10 +++++++---
>  include/uapi/linux/videodev2.h                        |  2 ++
>  3 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/buffer.rst b/Documentation/userspace-api/media/v4l/buffer.rst
> index 4638ec64db00..18a6f5fcc822 100644
> --- a/Documentation/userspace-api/media/v4l/buffer.rst
> +++ b/Documentation/userspace-api/media/v4l/buffer.rst
> @@ -607,6 +607,17 @@ 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-HEADERS-ONLY`:
> +
> +      - ``V4L2_BUF_FLAG_HEADERS_ONLY``
> +      - 0x00200000
> +      - This flag may be set when the buffer only contains codec

Set by the driver or userspace? Or either, depending on whether it is
an encoder or decoder?

codec -> the codec

> +	header, but does not contain any frame data. Usually the codec
> +	header is merged to the next idr frame, with the flag

to -> with
idr -> IDR

> +	``V4L2_BUF_FLAG_KEYFRAME``, but there is still some scenes that will

is -> are

scenes: do you mean 'scenarios'?

> +	split the header and queue it separately. This flag can set only when

"split the header and queue it separately" -> queue the header in a separate buffer

can -> can be

> +	codec support V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE,

codec -> the codec
support -> supports

> +	and the header mode is set to V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE
>      * .. _`V4L2-BUF-FLAG-REQUEST-FD`:
>  
>        - ``V4L2_BUF_FLAG_REQUEST_FD``
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 6183f43f4d73..478b6af4205d 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1386,8 +1386,12 @@ enum v4l2_mpeg_video_intra_refresh_period_type -
>      (enum)
>  
>  enum v4l2_mpeg_video_header_mode -
> -    Determines whether the header is returned as the first buffer or is
> -    it returned together with the first frame. Applicable to encoders.
> +    Determines whether the header is returned as the first buffer
> +    with flag V4L2_BUF_FLAG_HEADERS_ONLY or is

or is it -> or if it is

> +    it returned together with the first frame.
> +    Applicable to encoders and decoders.
> +    If it's not implemented in a driver,
> +    V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME is to be assumed,
>      Possible values are:
>  
>  .. raw:: latex
> @@ -1401,7 +1405,7 @@ enum v4l2_mpeg_video_header_mode -
>      :stub-columns: 0
>  
>      * - ``V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE``
> -      - The stream header is returned separately in the first buffer.
> +      - The stream header is returned separately in the first buffer with the flag V4L2_BUF_FLAG_HEADERS_ONLY.
>      * - ``V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME``
>        - The stream header is returned together with the first encoded
>  	frame.
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 5311ac4fde35..6fd96acd6080 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1131,6 +1131,8 @@ 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
> +/* Buffer only contains codec header */

codec -> the codec

> +#define V4L2_BUF_FLAG_HEADERS_ONLY		0x00200000
>  /* request_fd is valid */
>  #define V4L2_BUF_FLAG_REQUEST_FD		0x00800000
>  

Of course, there needs to be a driver that uses this as well. And drivers that support
V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE would need to add support for this flag as well,
I guess.

And what I haven't seen here is *why* you need this flag. There are already drivers that
support V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE, and they managed fine without it.

Regards,

	Hans

RE: [EXT] Re: [PATCH] media: videobuf2: add V4L2_BUF_FLAG_HEADERS_ONLY flag

[Ming Qian]

>From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>Sent: 2022年11月24日 18:42
>To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org
>Cc: shawnguo@kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de;
>kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
>imx@nxp.com>; linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
>linux-arm-kernel@lists.infradead.org; Nicolas Dufresne
><nicolas@ndufresne.ca>; Tomasz Figa <tfiga@chromium.org>
>Subject: [EXT] Re: [PATCH] media: videobuf2: add
>V4L2_BUF_FLAG_HEADERS_ONLY flag
>
>Caution: EXT Email
>
>+CC Nicolas and Tomasz.
>
>I would like some feedback for this patch.
>
>On 12/07/2022 11:37, Ming Qian wrote:
>> By setting the V4L2_BUF_FLAG_HEADERS_ONLY flag, hint the vb2 only
>> contains stream header, but does not contain any frame data.
>>
>> This flag needs to be used when header mode is set to
>> V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE.
>>
>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>> ---
>>  Documentation/userspace-api/media/v4l/buffer.rst      | 11 +++++++++++
>>  .../userspace-api/media/v4l/ext-ctrls-codec.rst       | 10 +++++++---
>>  include/uapi/linux/videodev2.h                        |  2 ++
>>  3 files changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/buffer.rst
>> b/Documentation/userspace-api/media/v4l/buffer.rst
>> index 4638ec64db00..18a6f5fcc822 100644
>> --- a/Documentation/userspace-api/media/v4l/buffer.rst
>> +++ b/Documentation/userspace-api/media/v4l/buffer.rst
>> @@ -607,6 +607,17 @@ 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-HEADERS-ONLY`:
>> +
>> +      - ``V4L2_BUF_FLAG_HEADERS_ONLY``
>> +      - 0x00200000
>> +      - This flag may be set when the buffer only contains codec
>
>Set by the driver or userspace? Or either, depending on whether it is an
>encoder or decoder?
>
>codec -> the codec
>
>> +     header, but does not contain any frame data. Usually the codec
>> +     header is merged to the next idr frame, with the flag
>
>to -> with
>idr -> IDR
>
>> +     ``V4L2_BUF_FLAG_KEYFRAME``, but there is still some scenes that
>> + will
>
>is -> are
>
>scenes: do you mean 'scenarios'?
>
>> +     split the header and queue it separately. This flag can set only
>> + when
>
>"split the header and queue it separately" -> queue the header in a separate
>buffer
>
>can -> can be
>
>> +     codec support V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE,
>
>codec -> the codec
>support -> supports
>
>> +     and the header mode is set to
>> + V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE
>>      * .. _`V4L2-BUF-FLAG-REQUEST-FD`:
>>
>>        - ``V4L2_BUF_FLAG_REQUEST_FD``
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> index 6183f43f4d73..478b6af4205d 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -1386,8 +1386,12 @@ enum
>v4l2_mpeg_video_intra_refresh_period_type -
>>      (enum)
>>
>>  enum v4l2_mpeg_video_header_mode -
>> -    Determines whether the header is returned as the first buffer or is
>> -    it returned together with the first frame. Applicable to encoders.
>> +    Determines whether the header is returned as the first buffer
>> +    with flag V4L2_BUF_FLAG_HEADERS_ONLY or is
>
>or is it -> or if it is
>
>> +    it returned together with the first frame.
>> +    Applicable to encoders and decoders.
>> +    If it's not implemented in a driver,
>> +    V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME is to be
>> + assumed,
>>      Possible values are:
>>
>>  .. raw:: latex
>> @@ -1401,7 +1405,7 @@ enum v4l2_mpeg_video_header_mode -
>>      :stub-columns: 0
>>
>>      * - ``V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE``
>> -      - The stream header is returned separately in the first buffer.
>> +      - The stream header is returned separately in the first buffer with the
>flag V4L2_BUF_FLAG_HEADERS_ONLY.
>>      * - ``V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME``
>>        - The stream header is returned together with the first encoded
>>       frame.
>> diff --git a/include/uapi/linux/videodev2.h
>> b/include/uapi/linux/videodev2.h index 5311ac4fde35..6fd96acd6080
>> 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -1131,6 +1131,8 @@ 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
>> +/* Buffer only contains codec header */
>
>codec -> the codec
>
>> +#define V4L2_BUF_FLAG_HEADERS_ONLY           0x00200000
>>  /* request_fd is valid */
>>  #define V4L2_BUF_FLAG_REQUEST_FD             0x00800000
>>
>
>Of course, there needs to be a driver that uses this as well. And drivers that
>support V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE would need to add
>support for this flag as well, I guess.
>
>And what I haven't seen here is *why* you need this flag. There are already
>drivers that support V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE, and
>they managed fine without it.
>
>Regards,
>
>        Hans

Hi Hans,
   The V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE is only supported by encoder currently,
And I want to apply it to decoder too. As the user may queue the codec header in a separate buffer.
Especially in android case, but the amphion vpu requires one buffer contains one frame, if not, driver will merge the header to the next IDR frame. So we need this flag to handle such case.

   And for encoder, if the V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE is set, the stream header is returned separately in the first buffer. But there are some codecs that can produce sps, pps, vps separately, it means there may be more than 1 buffers returned with header data only. So I think this flag is also an enhancement for encoder.

Ming

Re: [PATCH] media: videobuf2: add V4L2_BUF_FLAG_HEADERS_ONLY flag

[Nicolas Dufresne]

Replying on top, a bit unusual, but I think it makes sense ....

Le jeudi 24 novembre 2022 à 11:42 +0100, Hans Verkuil a écrit :
> +CC Nicolas and Tomasz.
> 
> I would like some feedback for this patch.
> 
> On 12/07/2022 11:37, Ming Qian wrote:
> > By setting the V4L2_BUF_FLAG_HEADERS_ONLY flag,
> > hint the vb2 only contains stream header,
> > but does not contain any frame data.
> > 
> > This flag needs to be used when header mode is set to
> > V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE.
> > 
> > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > ---
> >  Documentation/userspace-api/media/v4l/buffer.rst      | 11 +++++++++++
> >  .../userspace-api/media/v4l/ext-ctrls-codec.rst       | 10 +++++++---
> >  include/uapi/linux/videodev2.h                        |  2 ++
> >  3 files changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/buffer.rst b/Documentation/userspace-api/media/v4l/buffer.rst
> > index 4638ec64db00..18a6f5fcc822 100644
> > --- a/Documentation/userspace-api/media/v4l/buffer.rst
> > +++ b/Documentation/userspace-api/media/v4l/buffer.rst
> > @@ -607,6 +607,17 @@ 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-HEADERS-ONLY`:
> > +
> > +      - ``V4L2_BUF_FLAG_HEADERS_ONLY``
> > +      - 0x00200000
> > +      - This flag may be set when the buffer only contains codec
> 
> Set by the driver or userspace? Or either, depending on whether it is
> an encoder or decoder?

I think it should be set by the driver when encoding, and set by user space when
decoding. And of course, should be documented as previous review underline.

> 
> codec -> the codec
> 
> > +	header, but does not contain any frame data. Usually the codec
> > +	header is merged to the next idr frame, with the flag
> 
> to -> with
> idr -> IDR
> 
> > +	``V4L2_BUF_FLAG_KEYFRAME``, but there is still some scenes that will
> 
> is -> are
> 
> scenes: do you mean 'scenarios'?
> 
> > +	split the header and queue it separately. This flag can set only when
> 
> "split the header and queue it separately" -> queue the header in a separate buffer
> 
> can -> can be
> 
> > +	codec support V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE,
> 
> codec -> the codec
> support -> supports
> 
> > +	and the header mode is set to V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE
> >      * .. _`V4L2-BUF-FLAG-REQUEST-FD`:
> >  
> >        - ``V4L2_BUF_FLAG_REQUEST_FD``
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index 6183f43f4d73..478b6af4205d 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -1386,8 +1386,12 @@ enum v4l2_mpeg_video_intra_refresh_period_type -
> >      (enum)
> >  
> >  enum v4l2_mpeg_video_header_mode -
> > -    Determines whether the header is returned as the first buffer or is
> > -    it returned together with the first frame. Applicable to encoders.
> > +    Determines whether the header is returned as the first buffer
> > +    with flag V4L2_BUF_FLAG_HEADERS_ONLY or is
> 
> or is it -> or if it is
> 
> > +    it returned together with the first frame.
> > +    Applicable to encoders and decoders.
> > +    If it's not implemented in a driver,
> > +    V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME is to be assumed,
> >      Possible values are:
> >  
> >  .. raw:: latex
> > @@ -1401,7 +1405,7 @@ enum v4l2_mpeg_video_header_mode -
> >      :stub-columns: 0
> >  
> >      * - ``V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE``
> > -      - The stream header is returned separately in the first buffer.
> > +      - The stream header is returned separately in the first buffer with the flag V4L2_BUF_FLAG_HEADERS_ONLY.
> >      * - ``V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME``
> >        - The stream header is returned together with the first encoded
> >  	frame.
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 5311ac4fde35..6fd96acd6080 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1131,6 +1131,8 @@ 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
> > +/* Buffer only contains codec header */
> 
> codec -> the codec
> 
> > +#define V4L2_BUF_FLAG_HEADERS_ONLY		0x00200000
> >  /* request_fd is valid */
> >  #define V4L2_BUF_FLAG_REQUEST_FD		0x00800000
> >  
> 
> Of course, there needs to be a driver that uses this as well. And drivers that support
> V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE would need to add support for this flag as well,
> I guess.
> 
> And what I haven't seen here is *why* you need this flag. There are already drivers that
> support V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE, and they managed fine without it.

I think this can make sense, I'm not user of this, since this is OMX/Android
specific behaviour, but I think I can make sense of it.

For encoders, in WebRTC (any RTP, or streaming protocol with keyframe request),
we often ask the encoder to produce a new keyframe. We don't reset the encoder
this point. Some encoder may resend the headers, as the encoder is in "seperate
mode" it should send it separately. Userland can then handle resending the last
seem header if it wasn't there. It also help locating when the request was
actually honoured (I'm guessing there is already a keyframe flag of some sort).
So to me this enhancement is valid, it makes everything nicer. I agree it needs
a solid adoption, so any driver supporting this should be ported (could be blind
ported and tested by their maintainers).

For decoders, if a a decoder is in "separate mode", it seems that sending
headers must happen this way. If this uses a separate path internally, the
kernel needs also to be aware which buffers are what (and we don't parse in the
kernel). In very basic case, the driver assume that the first buffer after
streamon is a header, but that prevents resolution changes without a
drain+flush, which android and chromeos folks seems to use. (I always drain and
flush for what I'm doing).

Nicolas

> 
> Regards,
> 
> 	Hans

Re: [PATCH] media: videobuf2: add V4L2_BUF_FLAG_HEADERS_ONLY flag

[Hans Verkuil]

Hi Nicolas,

On 25/11/2022 18:07, Nicolas Dufresne wrote:
> Replying on top, a bit unusual, but I think it makes sense ....
> 
> Le jeudi 24 novembre 2022 à 11:42 +0100, Hans Verkuil a écrit :
>> +CC Nicolas and Tomasz.
>>
>> I would like some feedback for this patch.
>>
>> On 12/07/2022 11:37, Ming Qian wrote:
>>> By setting the V4L2_BUF_FLAG_HEADERS_ONLY flag,
>>> hint the vb2 only contains stream header,
>>> but does not contain any frame data.
>>>
>>> This flag needs to be used when header mode is set to
>>> V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE.
>>>
>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>> ---
>>>  Documentation/userspace-api/media/v4l/buffer.rst      | 11 +++++++++++
>>>  .../userspace-api/media/v4l/ext-ctrls-codec.rst       | 10 +++++++---
>>>  include/uapi/linux/videodev2.h                        |  2 ++
>>>  3 files changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/buffer.rst b/Documentation/userspace-api/media/v4l/buffer.rst
>>> index 4638ec64db00..18a6f5fcc822 100644
>>> --- a/Documentation/userspace-api/media/v4l/buffer.rst
>>> +++ b/Documentation/userspace-api/media/v4l/buffer.rst
>>> @@ -607,6 +607,17 @@ 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-HEADERS-ONLY`:
>>> +
>>> +      - ``V4L2_BUF_FLAG_HEADERS_ONLY``
>>> +      - 0x00200000
>>> +      - This flag may be set when the buffer only contains codec
>>
>> Set by the driver or userspace? Or either, depending on whether it is
>> an encoder or decoder?
> 
> I think it should be set by the driver when encoding, and set by user space when
> decoding. And of course, should be documented as previous review underline.

Makes sense.

> 
>>
>> codec -> the codec
>>
>>> +	header, but does not contain any frame data. Usually the codec
>>> +	header is merged to the next idr frame, with the flag
>>
>> to -> with
>> idr -> IDR
>>
>>> +	``V4L2_BUF_FLAG_KEYFRAME``, but there is still some scenes that will
>>
>> is -> are
>>
>> scenes: do you mean 'scenarios'?
>>
>>> +	split the header and queue it separately. This flag can set only when
>>
>> "split the header and queue it separately" -> queue the header in a separate buffer
>>
>> can -> can be
>>
>>> +	codec support V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE,
>>
>> codec -> the codec
>> support -> supports
>>
>>> +	and the header mode is set to V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE
>>>      * .. _`V4L2-BUF-FLAG-REQUEST-FD`:
>>>  
>>>        - ``V4L2_BUF_FLAG_REQUEST_FD``
>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> index 6183f43f4d73..478b6af4205d 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> @@ -1386,8 +1386,12 @@ enum v4l2_mpeg_video_intra_refresh_period_type -
>>>      (enum)
>>>  
>>>  enum v4l2_mpeg_video_header_mode -
>>> -    Determines whether the header is returned as the first buffer or is
>>> -    it returned together with the first frame. Applicable to encoders.
>>> +    Determines whether the header is returned as the first buffer
>>> +    with flag V4L2_BUF_FLAG_HEADERS_ONLY or is
>>
>> or is it -> or if it is
>>
>>> +    it returned together with the first frame.
>>> +    Applicable to encoders and decoders.
>>> +    If it's not implemented in a driver,
>>> +    V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME is to be assumed,
>>>      Possible values are:
>>>  
>>>  .. raw:: latex
>>> @@ -1401,7 +1405,7 @@ enum v4l2_mpeg_video_header_mode -
>>>      :stub-columns: 0
>>>  
>>>      * - ``V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE``
>>> -      - The stream header is returned separately in the first buffer.
>>> +      - The stream header is returned separately in the first buffer with the flag V4L2_BUF_FLAG_HEADERS_ONLY.
>>>      * - ``V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME``
>>>        - The stream header is returned together with the first encoded
>>>  	frame.
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 5311ac4fde35..6fd96acd6080 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -1131,6 +1131,8 @@ 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
>>> +/* Buffer only contains codec header */
>>
>> codec -> the codec
>>
>>> +#define V4L2_BUF_FLAG_HEADERS_ONLY		0x00200000
>>>  /* request_fd is valid */
>>>  #define V4L2_BUF_FLAG_REQUEST_FD		0x00800000
>>>  
>>
>> Of course, there needs to be a driver that uses this as well. And drivers that support
>> V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE would need to add support for this flag as well,
>> I guess.
>>
>> And what I haven't seen here is *why* you need this flag. There are already drivers that
>> support V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE, and they managed fine without it.
> 
> I think this can make sense, I'm not user of this, since this is OMX/Android
> specific behaviour, but I think I can make sense of it.
> 
> For encoders, in WebRTC (any RTP, or streaming protocol with keyframe request),
> we often ask the encoder to produce a new keyframe. We don't reset the encoder
> this point. Some encoder may resend the headers, as the encoder is in "seperate
> mode" it should send it separately. Userland can then handle resending the last
> seem header if it wasn't there. It also help locating when the request was
> actually honoured (I'm guessing there is already a keyframe flag of some sort).
> So to me this enhancement is valid, it makes everything nicer. I agree it needs
> a solid adoption, so any driver supporting this should be ported (could be blind
> ported and tested by their maintainers).
> 
> For decoders, if a a decoder is in "separate mode", it seems that sending
> headers must happen this way. If this uses a separate path internally, the
> kernel needs also to be aware which buffers are what (and we don't parse in the
> kernel). In very basic case, the driver assume that the first buffer after
> streamon is a header, but that prevents resolution changes without a
> drain+flush, which android and chromeos folks seems to use. (I always drain and
> flush for what I'm doing).

OK, thank you for the explanation.

So if this is going to be added, then existing drivers that use
V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE have to be adapted to use the new flag
as well.

From what I can tell those are the mediatek vcodec, venus and s5p-mfc encoders.
I suspect/hope that it won't be too difficult to add this new flag there.

Regards,

	Hans

> 
> Nicolas
> 
>>
>> Regards,
>>
>> 	Hans
> 

Re: [PATCH] media: videobuf2: add V4L2_BUF_FLAG_HEADERS_ONLY flag

[Nicolas Dufresne]

Le mardi 06 décembre 2022 à 10:05 +0100, Hans Verkuil a écrit :
> > For decoders, if a a decoder is in "separate mode", it seems that sending
> > headers must happen this way. If this uses a separate path internally, the
> > kernel needs also to be aware which buffers are what (and we don't parse in
> > the
> > kernel). In very basic case, the driver assume that the first buffer after
> > streamon is a header, but that prevents resolution changes without a
> > drain+flush, which android and chromeos folks seems to use. (I always drain
> > and
> > flush for what I'm doing).
> 
> OK, thank you for the explanation.
> 
> So if this is going to be added, then existing drivers that use
> V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE have to be adapted to use the new flag
> as well.
> 
> From what I can tell those are the mediatek vcodec, venus and s5p-mfc
> encoders.
> I suspect/hope that it won't be too difficult to add this new flag there.

The exercise will also be very informative for the reviewers, so yes, I would
ask Ming to update all the drivers, though its fine to only compile test this 
and leave it to the maintainers to verify (at least that's my opinion). I don't
see this change as a break, as any existing userspace will just ignore this, and
maybe managed to support it by deep parsing (which will keep working).
Otherwise, existing userspace using this mode have been broken for
renegotiation, and that change will not deteriorate (nor improve) the end
result.

Nicolas

> 
> Regards,
> 
> 	Hans

RE: [EXT] Re: [PATCH] media: videobuf2: add V4L2_BUF_FLAG_HEADERS_ONLY flag

[Ming Qian]

>-----Original Message-----
>From: Nicolas Dufresne <nicolas@ndufresne.ca>
>Le mardi 06 décembre 2022 à 10:05 +0100, Hans Verkuil a écrit :
>> > For decoders, if a a decoder is in "separate mode", it seems that
>> > sending headers must happen this way. If this uses a separate path
>> > internally, the kernel needs also to be aware which buffers are what
>> > (and we don't parse in the kernel). In very basic case, the driver
>> > assume that the first buffer after streamon is a header, but that
>> > prevents resolution changes without a
>> > drain+flush, which android and chromeos folks seems to use. (I
>> > drain+always drain
>> > and
>> > flush for what I'm doing).
>>
>> OK, thank you for the explanation.
>>
>> So if this is going to be added, then existing drivers that use
>> V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE have to be adapted to use
>the new
>> flag as well.
>>
>> From what I can tell those are the mediatek vcodec, venus and s5p-mfc
>> encoders.
>> I suspect/hope that it won't be too difficult to add this new flag there.
>
>The exercise will also be very informative for the reviewers, so yes, I would
>ask Ming to update all the drivers, though its fine to only compile test this and
>leave it to the maintainers to verify (at least that's my opinion). I don't see this
>change as a break, as any existing userspace will just ignore this, and maybe
>managed to support it by deep parsing (which will keep working).
>Otherwise, existing userspace using this mode have been broken for
>renegotiation, and that change will not deteriorate (nor improve) the end
>result.
>
>Nicolas
>
>>
>> Regards,
>>
>>       Hans

Hi Hans and Nicolas,
    I'll try to prepare the patch to update all the drivers that needs to apply this flag.
Ming