Re: [PATCH v3 1/3] media: uapi: h264: Clarify our expectations regarding NAL header format

[Boris Brezillon <boris.brezillon@collabora.com>]

On Fri, 5 Jul 2019 19:16:18 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Fri, 05 Jul 2019 13:40:03 -0300
> Ezequiel Garcia <ezequiel@collabora.com> wrote:
> 
> > Hi Boris, Paul,
> > 
> > On Wed, 2019-07-03 at 14:28 +0200, Boris Brezillon wrote:  
> > > Looks like some stateless decoders expect slices to be prefixed with
> > > ANNEX B start codes (they most likely do some kind of bitstream parsing
> > > and/or need that to delimit slices when doing per frame decoding).
> > > Since skipping those start codes for dummy stateless decoders (those
> > > expecting all params to be passed through controls) should be pretty
> > > easy, let's mandate that all slices be prepended with ANNEX B start
> > > codes.
> > > 
> > > If we ever need to support AVC headers, we can add a new menu control
> > > to select the type of NAL header to use.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > ---
> > > Changes in v3:
> > > * Add Paul's R-b
> > > 
> > > Changes in v2:
> > > * None
> > > ---
> > >  Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > index 7a1947f5be96..3ae1367806cf 100644
> > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > @@ -1726,6 +1726,7 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > >      :ref:`h264`, section 7.4.3 "Slice Header Semantics". For further
> > >      documentation, refer to the above specification, unless there is
> > >      an explicit comment stating otherwise.
> > > +    All slices should be prepended with an ANNEX B start code.
> > >      
> > 
> > Currently, the H264 slice V4L2_PIX_FMT_H264_SLICE_RAW,
> > is specified to _not_ contain the ANNEX B start code.  
> 
> Yep, we should provably rename the format.

Paul, are you okay with this rename?

s/V4L2_PIX_FMT_H264_SLICE_RAW/V4L2_PIX_FMT_H264_SLICE/

or

s/V4L2_PIX_FMT_H264_SLICE_RAW/V4L2_PIX_FMT_H264_SLICE_ANNEXB/

I'd also to discuss some concerns Ezequiel and I have regarding this
change. Some (most?) codec have alignment constraints on the buffer
they pass to the HW. For HW that support Annex B parsing, that's no
problem because the start of the buffer will be aligned on a page (I'm
assuming page alignment should cover 99% of the alignment constraints).
But HW that need to skip the start code will have to pass a non-aligned
buffer (annex B start code is 3 byte long).
Paul looked at the Cedrus driver and thinks it can be handled correctly
thanks to the VE_H264_VLD_OFFSET field (which encodes an offset in bit),
but I fear this might be a problem on other HW.

So, I'm asking again, are we sure we want to handle the raw (no start
code) and annex-b cases using the same pixel format? If we do, what's
the plan to address those potential alignment constraints? Should
we provide a way for userspace to define where the start-code ends so it
can align things properly (annex B can be extended with extra 00
bytes at the beginning)? If we do that, that means userspace has to
know about those alignment constraints, or take something big enough.
Another option would be to use a bounce buffer when things are not
aligned properly.

I'd really like to get feedback on those points before sending a v4.