Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Ezequiel Garcia <ezequiel@collabora.com>
To: Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Benjamin Gaignard <benjamin.gaignard@collabora.com>,
	p.zabel@pengutronix.de, mchehab@kernel.org, robh+dt@kernel.org,
	shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com,
	lee.jones@linaro.org, gregkh@linuxfoundation.org,
	mripard@kernel.org, paul.kocialkowski@bootlin.com, wens@csie.org,
	jernej.skrabec@siol.net, emil.l.velikov@gmail.com
Cc: kernel@pengutronix.de, linux-imx@nxp.com,
	linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
	kernel@collabora.com, cphealy@gmail.com
Subject: Re: [PATCH v10 6/9] media: uapi: Add a control for HANTRO driver
Date: Sun, 16 May 2021 20:04:05 -0300
Message-ID: <f46b9c889914360348f218b45f10a06bd1bd8595.camel@collabora.com> (raw)
In-Reply-To: <815a4bd6-599b-cfb8-9ddc-efa4b7092c23@xs4all.nl>

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


  parent reply index

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-20 12:10 [PATCH v10 0/9] Add HANTRO G2/HEVC decoder support for IMX8MQ Benjamin Gaignard
2021-04-20 12:10 ` [PATCH v10 1/9] media: hevc: Add fields and flags for hevc PPS Benjamin Gaignard
2021-04-20 12:10 ` [PATCH v10 2/9] media: hevc: Add decode params control Benjamin Gaignard
2021-04-20 12:10 ` [PATCH v10 3/9] media: hantro: change hantro_codec_ops run prototype to return errors Benjamin Gaignard
2021-04-20 12:10 ` [PATCH v10 4/9] media: hantro: Define HEVC codec profiles and supported features Benjamin Gaignard
2021-04-20 12:10 ` [PATCH v10 5/9] media: hantro: Only use postproc when post processed formats are defined Benjamin Gaignard
2021-04-20 12:10 ` [PATCH v10 6/9] media: uapi: Add a control for HANTRO driver Benjamin Gaignard
2021-04-29 13:43   ` Ezequiel Garcia
2021-05-05 14:55   ` Hans Verkuil
2021-05-05 15:20     ` Benjamin Gaignard
2021-05-06 12:50       ` Hans Verkuil
2021-05-06 13:11         ` John Cox
2021-05-06 14:29           ` Nicolas Dufresne
2021-05-16 23:04         ` Ezequiel Garcia [this message]
2021-05-18 17:17           ` Nicolas Dufresne
2021-05-05 21:01     ` Ezequiel Garcia
2021-05-05 15:18   ` John Cox
2021-05-05 15:33     ` Nicolas Dufresne
2021-04-20 12:10 ` [PATCH v10 7/9] media: hantro: handle V4L2_PIX_FMT_HEVC_SLICE control Benjamin Gaignard
2021-04-20 12:10 ` [PATCH v10 8/9] media: hantro: Introduce G2/HEVC decoder Benjamin Gaignard
2021-05-05 14:47   ` Hans Verkuil
2021-04-20 12:10 ` [PATCH v10 9/9] media: hantro: IMX8M: add variant for G2/HEVC codec Benjamin Gaignard
2021-05-05 14:42 ` [PATCH v10 0/9] Add HANTRO G2/HEVC decoder support for IMX8MQ Hans Verkuil
2021-05-25 15:39   ` Hans Verkuil
2021-05-25 16:14     ` Benjamin Gaignard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f46b9c889914360348f218b45f10a06bd1bd8595.camel@collabora.com \
    --to=ezequiel@collabora.com \
    --cc=benjamin.gaignard@collabora.com \
    --cc=cphealy@gmail.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=devicetree@vger.kernel.org \
    --cc=emil.l.velikov@gmail.com \
    --cc=festevam@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hverkuil-cisco@xs4all.nl \
    --cc=jernej.skrabec@siol.net \
    --cc=kernel@collabora.com \
    --cc=kernel@pengutronix.de \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=mchehab@kernel.org \
    --cc=mripard@kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=paul.kocialkowski@bootlin.com \
    --cc=robh+dt@kernel.org \
    --cc=s.hauer@pengutronix.de \
    --cc=shawnguo@kernel.org \
    --cc=wens@csie.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git