Re: [RFC PATCH 1/2] media: Add AV1 uAPI

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: daniel.almeida@collabora.com, stevecho@google.com,
	shawnku@google.com, tzungbi@google.com, mcasas@google.com,
	nhebert@google.com, abodenha@google.com, randy.wu@mediatek.com,
	yunfei.dong@mediatek.com, gustavo.padovan@collabora.com,
	andrzej.pietrasiewicz@collabora.com,
	enric.balletbo@collabora.com, ezequiel@collabora.com,
	nicolas.dufresne@collabora.com, tomeu.vizoso@collabora.com,
	nick.milner@collabora.com, xiaoyong.lu@mediatek.com,
	mchehab@kernel.org
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
	kernel@collabora.com
Subject: Re: [RFC PATCH 1/2] media: Add AV1 uAPI
Date: Thu, 2 Sep 2021 17:10:02 +0200	[thread overview]
Message-ID: <731ff6d4-ba1c-1918-e134-013786be473a@xs4all.nl> (raw)
In-Reply-To: <20210810220552.298140-2-daniel.almeida@collabora.com>

Hi Daniel!

Two small comments below:

On 11/08/2021 00:05, daniel.almeida@collabora.com wrote:
> From: Daniel Almeida <daniel.almeida@collabora.com>
> 
> This patch adds the  AOMedia Video 1 (AV1) kernel uAPI.
> 
> This design is based on currently available AV1 API implementations and
> aims to support the development of AV1 stateless video codecs
> on Linux.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  .../userspace-api/media/v4l/biblio.rst        |   10 +
>  .../media/v4l/ext-ctrls-codec-stateless.rst   | 1268 +++++++++++++++++
>  .../media/v4l/pixfmt-compressed.rst           |   21 +
>  .../media/v4l/vidioc-g-ext-ctrls.rst          |   36 +
>  .../media/v4l/vidioc-queryctrl.rst            |   54 +
>  .../media/videodev2.h.rst.exceptions          |    9 +
>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  286 +++-
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |   79 +
>  drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
>  include/media/v4l2-ctrls.h                    |   12 +
>  include/uapi/linux/v4l2-controls.h            |  796 +++++++++++
>  include/uapi/linux/videodev2.h                |   15 +
>  12 files changed, 2586 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/biblio.rst b/Documentation/userspace-api/media/v4l/biblio.rst
> index 7b8e6738ff9e..7061144d10bb 100644
> --- a/Documentation/userspace-api/media/v4l/biblio.rst
> +++ b/Documentation/userspace-api/media/v4l/biblio.rst
> @@ -417,3 +417,13 @@ VP8
>  :title:     RFC 6386: "VP8 Data Format and Decoding Guide"
>  
>  :author:    J. Bankoski et al.
> +
> +.. _av1:
> +
> +AV1
> +===
> +
> +
> +:title:     AV1 Bitstream & Decoding Process Specification
> +
> +:author:    Peter de Rivaz, Argon Design Ltd, Jack Haughton, Argon Design Ltd

<snip>

> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> index 819a70a26e18..73ff5311b7ae 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> @@ -507,6 +507,60 @@ See also the examples in :ref:`control`.
>        - n/a
>        - A struct :c:type:`v4l2_ctrl_hevc_decode_params`, containing HEVC
>  	decoding parameters for stateless video decoders.
> +    * - ``V4L2_CTRL_TYPE_AV1_SEQUENCE``
> +      - n/a
> +      - n/a
> +      - n/a
> +      - A struct :c:type:`v4l2_ctrl_av1_sequence`, containing AV1 Sequence OBU
> +	decoding parameters for stateless video decoders.
> +    * - ``V4L2_CTRL_TYPE_AV1_TILE_GROUP``
> +      - n/a
> +      - n/a
> +      - n/a
> +      - A struct :c:type:`v4l2_ctrl_av1_tile_group`, containing AV1 Tile Group
> +	OBU decoding parameters for stateless video decoders.
> +    * - ``V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY``
> +      - n/a
> +      - n/a
> +      - n/a
> +      - A struct :c:type:`v4l2_ctrl_av1_tile_group`, containing AV1 Tile Group

I guess this should be:

Tile Group -> Tile Group Entry

> +	OBU decoding parameters for stateless video decoders.
> +    * - ``V4L2_CTRL_TYPE_AV1_TILE_LIST``
> +      - n/a
> +      - n/a
> +      - n/a
> +      - A struct :c:type:`v4l2_ctrl_av1_tile_list`, containing AV1 Tile List
> +	OBU decoding parameters for stateless video decoders.
> +    * - ``V4L2_CTRL_TYPE_AV1_TILE_LIST_ENTRY``
> +      - n/a
> +      - n/a
> +      - n/a
> +      - A struct :c:type:`v4l2_ctrl_av1_tile_list_entry`, containing AV1 Tile List

Also missing 'Entry'

> +	OBU decoding parameters for stateless video decoders.
> +    * - ``V4L2_CTRL_TYPE_AV1_FRAME_HEADER``
> +      - n/a
> +      - n/a
> +      - n/a
> +      - A struct :c:type:`v4l2_ctrl_av1_frame_header`, containing AV1 Frame/Frame
> +	Header OBU decoding parameters for stateless video decoders.
> +    * - ``V4L2_CTRL_TYPE_AV1_PROFILE``
> +      - n/a
> +      - n/a
> +      - n/a
> +      - A enum :c:type:`v4l2_ctrl_av1_profile`, indicating what AV1 profiles
> +	an AV1 stateless decoder might support.
> +    * - ``V4L2_CTRL_TYPE_AV1_LEVEL``
> +      - n/a
> +      - n/a
> +      - n/a
> +      - A enum :c:type:`v4l2_ctrl_av1_level`, indicating what AV1 levels
> +	an AV1 stateless decoder might support.
> +    * - ``V4L2_CTRL_TYPE_AV1_OPERATING_MODE``
> +      - n/a
> +      - n/a
> +      - n/a
> +      - A enum :c:type:`v4l2_ctrl_av1_operating_mode`, indicating what AV1
> +	operating modes an AV1 stateless decoder might support.
>  
>  .. raw:: latex
>  

Regards,

	Hans