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@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.

> 
> 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