Re: [PATCH] media: videobuf2: add V4L2_BUF_FLAG_HEADERS_ONLY flag
From: Hans Verkuil
Date: Tue Dec 06 2022 - 04:05:30 EST
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@xxxxxxx>
>>> ---
>>> 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
>