Re: [PATCH v2 1/2] media: uapi: Add H264 low-level decoder API compound controls.

From: Alexandre Courbot <acourbot@chromium.org>
To: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Tomasz Figa <tfiga@chromium.org>,
	Pawel Osciak <posciak@chromium.org>,
	Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
	Chen-Yu Tsai <wens@csie.org>, LKML <linux-kernel@vger.kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	Linux Media Mailing List <linux-media@vger.kernel.org>,
	Nicolas Dufresne <nicolas.dufresne@collabora.com>,
	jenskuske@gmail.com, linux-sunxi@googlegroups.com,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Guenter Roeck <groeck@chromium.org>
Subject: Re: [PATCH v2 1/2] media: uapi: Add H264 low-level decoder API compound controls.
Date: Mon, 28 Jan 2019 14:54:27 +0900	[thread overview]
Message-ID: <CAPBb6MX9=+k=5M-w7SyCUAKUceAXN7UG_6==tFx4RhR13Etdwg@mail.gmail.com> (raw)
In-Reply-To: <20181115145650.9827-2-maxime.ripard@bootlin.com>

On Thu, Nov 15, 2018 at 11:56 PM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> From: Pawel Osciak <posciak@chromium.org>
>
> Stateless video codecs will require both the H264 metadata and slices in
> order to be able to decode frames.
>
> This introduces the definitions for a new pixel format for H264 slices that
> have been parsed, as well as the structures used to pass the metadata from
> the userspace to the kernel.
>
> Co-Developed-by: Maxime Ripard <maxime.ripard@bootlin.com>
> Signed-off-by: Pawel Osciak <posciak@chromium.org>
> Signed-off-by: Guenter Roeck <groeck@chromium.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

<snip>

> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index b854cceb19dc..e96c453208e8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -825,6 +825,11 @@ const char *v4l2_ctrl_get_name(u32 id)
>         case V4L2_CID_MPEG_VIDEO_H264_HIERARCHICAL_CODING_LAYER:return "H264 Number of HC Layers";
>         case V4L2_CID_MPEG_VIDEO_H264_HIERARCHICAL_CODING_LAYER_QP:
>                                                                 return "H264 Set QP Value for HC Layers";
> +       case V4L2_CID_MPEG_VIDEO_H264_SPS:                      return "H264 SPS";
> +       case V4L2_CID_MPEG_VIDEO_H264_PPS:                      return "H264 PPS";
> +       case V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX:           return "H264 Scaling Matrix";
> +       case V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS:             return "H264 Slice Parameters";
> +       case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS:            return "H264 Decode Parameters";
>         case V4L2_CID_MPEG_VIDEO_MPEG4_I_FRAME_QP:              return "MPEG4 I-Frame QP Value";
>         case V4L2_CID_MPEG_VIDEO_MPEG4_P_FRAME_QP:              return "MPEG4 P-Frame QP Value";
>         case V4L2_CID_MPEG_VIDEO_MPEG4_B_FRAME_QP:              return "MPEG4 B-Frame QP Value";

To make things future-proof I think it may be good to add a control
specifying the granularity of data sent with each request (see
https://lkml.org/lkml/2019/1/24/147).

Right now we have a consensus that to make things simple, we request
one frame of encoded data per request. But this will probably be
relaxed in the future, since allowing to process things at lower
granularity may improve latency. Moreover the granularity accepted by
the encoder is hardware/firmware dependent, so it is probably a good
idea to expose this from the beginning.

How about a new V4L2_CID_MPEG_VIDEO_H264_GRANULARITY control with only
one value at the moment, namely
V4L2_MPEG_VIDEO_H264_GRANULARITY_FRAME? We could extend this in the
future, and that way user-space will have no excuse for not checking
that the codec supports the input granularity it will send.

I'm wondering whether this could be made codec-independent, but I'm
afraid this would add confusion.