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

[Ming Qian]

>From: Nicolas Dufresne <nicolas@xxxxxxxxxxxx>
>Sent: 2022年7月5日 21:13
>To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx;
>hverkuil-cisco@xxxxxxxxx
>Cc: shawnguo@xxxxxxxxxx; robh+dt@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
>kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx
><linux-imx@xxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
>linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>Subject: Re: [EXT] Re: [PATCH] media: videobuf2: add
>V4L2_BUF_FLAG_CODECCONFIG flag
>
>Caution: EXT Email
>
>Le mardi 05 juillet 2022 à 11:34 +0000, Ming Qian a écrit :
>> > > From: Ming Qian
>> > > Sent: 2022年7月5日 9:52
>> > > To: Nicolas Dufresne <nicolas@xxxxxxxxxxxx>; mchehab@xxxxxxxxxx;
>> > > hverkuil-cisco@xxxxxxxxx
>> > > Cc: shawnguo@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
>> > > s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
>> > > dl-linux-imx <linux-imx@xxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
>> > > linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> > > Subject: RE: [EXT] Re: [PATCH] media: videobuf2: add
>> > > V4L2_BUF_FLAG_CODECCONFIG flag
>> > >
>> > > > From: Nicolas Dufresne <nicolas@xxxxxxxxxxxx>
>> > > > Sent: 2022年7月4日 23:53
>> > > > To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx;
>> > > > hverkuil-cisco@xxxxxxxxx
>> > > > Cc: shawnguo@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
>> > > > s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
>> > > > festevam@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
>> > > > linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>> > > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> > > > Subject: [EXT] Re: [PATCH] media: videobuf2: add
>> > > > V4L2_BUF_FLAG_CODECCONFIG flag
>> > > >
>> > > > Caution: EXT Email
>> > > >
>> > > > Le mardi 28 juin 2022 à 10:19 +0800, Ming Qian a écrit :
>> > > > > By setting the V4L2_BUF_FLAG_CODECCONFIG flag, user-space
>> > > > > should be able to hint decoder the vb2 only contains codec
>> > > > > config header, but does not contain any frame data.
>> > > > > It's only used for parsing header, and can't be decoded.
>> > > >
>> > > > This is copied from OMX specification. I think we we import
>> > > > this, we should at least refer to the original.
>> > > >
>> > >
>> > > Hi Nicolas,
>> > >    Do you mean OMX_BUFFERFLAG_CODECCONFIG?
>> > >    I'm sorry that I didn't notice it before.
>> > >    Currently we only encounter this requirement on Android, I'm
>> > > not sure if it has a reference to omx.
>> > >    And thank you very much for pointing out it.
>> > >
>> >
>> > Android media stack has been based on OMX for the last decade. They
>> > are slowly moving to CODEC2 which more or less is a similar
>> > abstraction with similar ideas. Let's research prior art, so we don't screw
>compatibility.
>> >
>>
>> I got it, I'll try to study the android codec2, and do you agree that
>> we should add V4L2_BUF_FLAG_CODECCONFIG flag, just like
>> OMX_BUFFERFLAG_CODECCONFIG?
>> Or is there any other solution that can handle this case?
>
>We can probably discuss the name. CODECCONFIG is a bit strange, could be
>CODEC_HEADER, HEADER_ONLY, CONFIG_ONLY, something along these lines.
>I'm just wondering what is the best rule, since more specification is needed
>here.
>Current userland expect full frames into each encoded buffer. If we start
>splitting these up, we'll break non-android userland (and Android userland does
>not seems to be very upstream thing, every vendor forks it).
>
>I also think that what the CODECCONFIG contains and its format need to be
>strictly documented for every CODEC that would allow it. In H.264 notably, the
>headers could be packed in Annex B. or AVCc NAL headers. If we look at
>FFMPEG, which uses codec_data name, they only requires this when the header
>are not "inline", which means only for AVCc. Also, many codec_data is for other
>codecs get wrapped into ISOMP4 or Matroska (webm) envelope.
>
>On the other hand, I don't remember if the 1 frame per buffer is an actual rule
>or simply what existing userland expect. Also, I'll be fair, there is no reason this
>must come from the driver. Android OMX or CODEC2 COMPONENT is a userland
>component, it could do bitstream scanning (using traditional boyer-more) to
>find and split appart the config to satisfy its internal API. This can be done with
>low overhead and zero-copy.
>

OK, I'll try to change the name, maybe CODEC_HEADER is better.
And I think we add the flag just to support more scenarios, it won't break the existing userland flow.

It's not easy to change the android codec2, and we indeed want to support it.

>>
>> > > Ming
>> > >
>> > > > >
>> > > > > Current, it's usually used by android.
>> > > > >
>> > > > > Signed-off-by: Ming Qian <ming.qian@xxxxxxx>
>> > > > > ---
>> > > > >  Documentation/userspace-api/media/v4l/buffer.rst | 9 +++++++++
>> > > > >  include/uapi/linux/videodev2.h                   | 2 ++
>> > > > >  2 files changed, 11 insertions(+)
>> > > > >
>> > > > > diff --git a/Documentation/userspace-api/media/v4l/buffer.rst
>> > > > > b/Documentation/userspace-api/media/v4l/buffer.rst
>> > > > > index 4638ec64db00..acdc4556f4f4 100644
>> > > > > --- a/Documentation/userspace-api/media/v4l/buffer.rst
>> > > > > +++ b/Documentation/userspace-api/media/v4l/buffer.rst
>> > > > > @@ -607,6 +607,15 @@ 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
>> > > > > +      - This flag may be set when the buffer only contains
>> > > > > + codec
>> > > > > config
>> > > > > +    header, but does not contain any frame data. Usually the
>> > > > > + codec
>> > > config
>> > > > > +    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.
>> > > >
>> > > > I think the documentation is clear. Now, if a driver uses this,
>> > > > will existing userland (perhaps good to check GStreamer, FFMPEG
>> > > > and Chromium ?) will break ?
>> > > > So we need existing driver to do this when flagged to, and just
>> > > > copy/append when the userland didn't opt-in that feature ?
>> > > >
>> > > > >      * .. _`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
>> > > > > 5311ac4fde35..8708ef257710
>> > > > > 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_CODECCONFIG            0x00200000
>> > > > >  /* request_fd is valid */
>> > > > >  #define V4L2_BUF_FLAG_REQUEST_FD             0x00800000
>> > > > >
>>