Re: [PATCH v10 6/9] media: uapi: Add a control for HANTRO driver

[Ezequiel Garcia <ezequiel@collabora.com>]

Hi Hans,

On Thu, 2021-05-06 at 14:50 +0200, Hans Verkuil wrote:
> On 05/05/2021 17:20, Benjamin Gaignard wrote:
> > 
> > Le 05/05/2021 à 16:55, Hans Verkuil a écrit :
> > > On 20/04/2021 14:10, Benjamin Gaignard wrote:
> > > > The HEVC HANTRO driver needs to know the number of bits to skip at
> > > > the beginning of the slice header.
> > > > That is a hardware specific requirement so create a dedicated control
> > > > for this purpose.
> > > > 
> > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > > ---
> > > >   .../userspace-api/media/drivers/hantro.rst    | 19 +++++++++++++++++++
> > > >   .../userspace-api/media/drivers/index.rst     |  1 +
> > > >   include/media/hevc-ctrls.h                    | 13 +++++++++++++
> > > >   3 files changed, 33 insertions(+)
> > > >   create mode 100644 Documentation/userspace-api/media/drivers/hantro.rst
> > > > 
> > > > diff --git a/Documentation/userspace-api/media/drivers/hantro.rst b/Documentation/userspace-api/media/drivers/hantro.rst
> > > > new file mode 100644
> > > > index 000000000000..cd9754b4e005
> > > > --- /dev/null
> > > > +++ b/Documentation/userspace-api/media/drivers/hantro.rst
> > > > @@ -0,0 +1,19 @@
> > > > +.. SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +Hantro video decoder driver
> > > > +===========================
> > > > +
> > > > +The Hantro video decoder driver implements the following driver-specific controls:
> > > > +
> > > > +``V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP (integer)``
> > > > +    Specifies to Hantro HEVC video decoder driver the number of data (in bits) to
> > > > +    skip in the slice segment header.
> > > > +    If non-IDR, the bits to be skipped go from syntax element "pic_output_flag"
> > > > +    to before syntax element "slice_temporal_mvp_enabled_flag".
> > > > +    If IDR, the skipped bits are just "pic_output_flag"
> > > > +    (separate_colour_plane_flag is not supported).
> > > I'm not very keen on this. Without this information the video data cannot be
> > > decoded, or will it just be suboptimal?
> > 
> > Without that information the video can't be decoded.
> > 
> > > 
> > > The problem is that a generic decoder would have to know that the HW is a hantro,
> > > and then call this control. If they don't (and are testing on non-hantro HW), then
> > > it won't work, thus defeating the purpose of the HW independent decoder API.
> > > 
> > > Since hantro is widely used, and if there is no other way to do this beside explitely
> > > setting this control, then perhaps this should be part of the standard HEVC API.
> > > Non-hantro drivers that do not need this can just skip it.
> > 
> > Even if I put this parameter in decode_params structure that would means that a generic
> > userland decoder will have to know how the compute this value for hantro HW since it
> > isn't something that could be done on kernel side.
> 
> But since hantro is very common, any userland decoder will need to calculate this anyway.
> So perhaps it is better to have this as part of the decode_params?
> 
> I'd like to know what others think about this.
> 

As you know, I'm not a fan of carrying these "unstable" APIs around.
I know it's better than nothing, but I feel they create the illusion
of the interface being supported in mainline. Since it's unstable,
it's difficult for applications to adopt them.

As Nicolas mentioned, this means neither FFmpeg nor GStreamer will adopt
these APIs, which worries me, as that means we lose two major user bases.

My personal take from this, is that we need to find ways to stabilize
our stateless codec APIs in less time and perhaps with less effort.

IMO, a less stiff interface could help us here, and that's why I think
having hardware-specific controls can be useful. Hardware designers
can be so creative :)

I'm not against introducing this specific parameter in
v4l2_ctrl_hevc_codec_params, arguing that Hantro is widely used,
but I'd like us to be open to hardware-specific controls as a way
to extend the APIs seamlessly.

Applications won't have to _know_ what hardware they are running on,
they can just use VIDIOC_QUERYCTRL to find out which controls are needed.

Thanks,
Ezequiel