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

[Nicolas Dufresne <nicolas@ndufresne.ca>]

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@ndufresne.ca>; mchehab@kernel.org;
> > > hverkuil-cisco@xs4all.nl
> > > 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
> > > Subject: RE: [EXT] Re: [PATCH] media: videobuf2: add
> > > V4L2_BUF_FLAG_CODECCONFIG flag
> > > 
> > > > From: Nicolas Dufresne <nicolas@ndufresne.ca>
> > > > Sent: 2022年7月4日 23:53
> > > > To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
> > > > hverkuil-cisco@xs4all.nl
> > > > 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
> > > > 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.

> 
> > > Ming
> > > 
> > > > > 
> > > > > Current, it's usually used by android.
> > > > > 
> > > > > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > > > > ---
> > > > >  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
> > > > > 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel