Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <35256805b1ce0b2b7533f53f658c799a37255469.camel@ndufresne.ca>
Subject: Re: [PATCH] media: videobuf2: add V4L2_BUF_FLAG_HEADERS_ONLY flag
From:   Nicolas Dufresne <nicolas@ndufresne.ca>
To:     Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        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, linux-imx@nxp.com,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        Tomasz Figa <tfiga@chromium.org>
Date:   Fri, 25 Nov 2022 12:07:17 -0500
In-Reply-To: <15ba9b9e-f3f6-7f30-c200-d7c9593a4735@xs4all.nl>
References: <20220712093754.23370-1-ming.qian@nxp.com>
         <15ba9b9e-f3f6-7f30-c200-d7c9593a4735@xs4all.nl>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

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

Le jeudi 24 novembre 2022 =C3=A0 11:42 +0100, Hans Verkuil a =C3=A9crit=C2=
=A0:
> +CC Nicolas and Tomasz.
>=20
> I would like some feedback for this patch.
>=20
> 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.
> >=20
> > This flag needs to be used when header mode is set to
> > V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE.
> >=20
> > 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(-)
> >=20
> > diff --git a/Documentation/userspace-api/media/v4l/buffer.rst b/Documen=
tation/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
>=20
> 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.

>=20
> codec -> the codec
>=20
> > +	header, but does not contain any frame data. Usually the codec
> > +	header is merged to the next idr frame, with the flag
>=20
> to -> with
> idr -> IDR
>=20
> > +	``V4L2_BUF_FLAG_KEYFRAME``, but there is still some scenes that will
>=20
> is -> are
>=20
> scenes: do you mean 'scenarios'?
>=20
> > +	split the header and queue it separately. This flag can set only when
>=20
> "split the header and queue it separately" -> queue the header in a separ=
ate buffer
>=20
> can -> can be
>=20
> > +	codec support V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE,
>=20
> codec -> the codec
> support -> supports
>=20
> > +	and the header mode is set to V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE
> >      * .. _`V4L2-BUF-FLAG-REQUEST-FD`:
> > =20
> >        - ``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)
> > =20
> >  enum v4l2_mpeg_video_header_mode -
> > -    Determines whether the header is returned as the first buffer or i=
s
> > -    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
>=20
> or is it -> or if it is
>=20
> > +    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:
> > =20
> >  .. raw:: latex
> > @@ -1401,7 +1405,7 @@ enum v4l2_mpeg_video_header_mode -
> >      :stub-columns: 0
> > =20
> >      * - ``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 w=
ith 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/videod=
ev2.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 stru=
ct timeval *tv)
> >  #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
> >  /* mem2mem encoder/decoder */
> >  #define V4L2_BUF_FLAG_LAST			0x00100000
> > +/* Buffer only contains codec header */
>=20
> codec -> the codec
>=20
> > +#define V4L2_BUF_FLAG_HEADERS_ONLY		0x00200000
> >  /* request_fd is valid */
> >  #define V4L2_BUF_FLAG_REQUEST_FD		0x00800000
> > =20
>=20
> 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 f=
lag as well,
> I guess.
>=20
> And what I haven't seen here is *why* you need this flag. There are alrea=
dy drivers that
> support V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE, and they managed fine witho=
ut it.

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

For encoders, in WebRTC (any RTP, or streaming protocol with keyframe reque=
st),
we often ask the encoder to produce a new keyframe. We don't reset the enco=
der
this point. Some encoder may resend the headers, as the encoder is in "sepe=
rate
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 so=
rt).
So to me this enhancement is valid, it makes everything nicer. I agree it n=
eeds
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

>=20
> Regards,
>=20
> 	Hans