Re: [PATCH v10 6/9] media: uapi: Add a control for HANTRO driver
From: Ezequiel Garcia
Date: Wed May 05 2021 - 17:01:48 EST
On Wed, 2021-05-05 at 16:55 +0200, Hans Verkuil wrote:
> 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@xxxxxxxxxxxxx>
> > ---
> > .../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?
> The problem is that a generic decoder would have to know that the HW is a hantro,
Applications can just query which controls are exposed by a video device,
and if this control is found, then it means it needs to be set.
> 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.
The decision to move it out of the HEVC API is not really to avoid setting it.
In the end, most/all applications will end up required to set this