Message-ID: <Zz3nK9FvSpxgVzmo@desktop>
Date: Wed, 20 Nov 2024 14:42:03 +0100
From: Link Mauve <linkmauve@...kmauve.fr>
To: Hugues Fruchet <hugues.fruchet@...s.st.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Hans Verkuil <hverkuil-cisco@...all.nl>,
	Fritz Koenig <frkoenig@...omium.org>,
	Sebastian Fricke <sebastian.fricke@...labora.com>,
	Daniel Almeida <daniel.almeida@...labora.com>,
	Andrzej Pietrasiewicz <andrzej.p@...labora.com>,
	Nicolas Dufresne <nicolas.dufresne@...labora.com>,
	Benjamin Gaignard <benjamin.gaignard@...labora.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-rockchip@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH v2 1/3] media: uapi: add WebP uAPI

Hi,

On Wed, Nov 20, 2024 at 12:01:03PM +0100, Hugues Fruchet wrote:
> This patch adds the WebP picture decoding kernel uAPI.
> 
> This design is based on currently available VP8 API implementation and
> aims to support the development of WebP stateless video codecs
> on Linux.

Why do you need this new uAPI exactly?  The WebP format is more complex
than the simple 'VP8 ' format, the 'VP8X' fourcc for instance is an
animated format which may contain multiple VP8 keyframes, or an alpha
side channel, and just like any other video container we queue each
VP8 frame separately in V4L2 for decoding, not the whole file.

In Onix[1] I parse the WebP header and pass the raw VP8 frame to V4L2
without the RIFF around it.

So I’d rather NACK this patch, I don’t think it’s a good idea to
hardcode the simplest version of the WebP container in the uAPI, to the
detriment of all other possible WebP files.

[1] git clone https://git.linkmauve.fr/onix.git/

> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@...s.st.com>
> ---
>  Documentation/userspace-api/media/v4l/biblio.rst  |  9 +++++++++
>  .../userspace-api/media/v4l/pixfmt-compressed.rst | 15 +++++++++++++++
>  drivers/media/v4l2-core/v4l2-ioctl.c              |  1 +
>  include/uapi/linux/videodev2.h                    |  1 +
>  4 files changed, 26 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/biblio.rst b/Documentation/userspace-api/media/v4l/biblio.rst
> index 35674eeae20d..df3e963fc54f 100644
> --- a/Documentation/userspace-api/media/v4l/biblio.rst
> +++ b/Documentation/userspace-api/media/v4l/biblio.rst
> @@ -447,3 +447,12 @@ AV1
>  :title:     AV1 Bitstream & Decoding Process Specification
>  
>  :author:    Peter de Rivaz, Argon Design Ltd, Jack Haughton, Argon Design Ltd
> +
> +.. _webp:
> +
> +WEBP
> +====
> +
> +:title:     WEBP picture Bitstream & Decoding Process Specification
> +
> +:author:    Google (https://developers.google.com/speed/webp)
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> index 806ed73ac474..e664e70b0619 100644
> --- a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> +++ b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> @@ -169,6 +169,21 @@ Compressed Formats
>  	this pixel format. The output buffer must contain the appropriate number
>  	of macroblocks to decode a full corresponding frame to the matching
>  	capture buffer.
> +    * .. _V4L2-PIX-FMT-WEBP-FRAME:
> +
> +      - ``V4L2_PIX_FMT_WEBP_FRAME``
> +      - 'WEBP'
> +      - WEBP VP8 parsed frame, excluding WEBP RIFF header, keeping only the VP8
> +	bistream including the frame header, as extracted from the container.
> +	This format is adapted for stateless video decoders that implement a
> +	WEBP pipeline with the :ref:`stateless_decoder`.
> +	Metadata associated with the frame to decode is required to be passed
> +	through the ``V4L2_CID_STATELESS_VP8_FRAME`` control.
> +	See the :ref:`associated Codec Control IDs <v4l2-codec-stateless-vp8>`.
> +	Exactly one output and one capture buffer must be provided for use with
> +	this pixel format. The output buffer must contain the appropriate number
> +	of macroblocks to decode a full corresponding frame to the matching
> +	capture buffer.
>  
>      * .. _V4L2-PIX-FMT-VP9:
>  
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 0304daa8471d..e2ff03d0d773 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1501,6 +1501,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>  		case V4L2_PIX_FMT_VC1_ANNEX_L:	descr = "VC-1 (SMPTE 412M Annex L)"; break;
>  		case V4L2_PIX_FMT_VP8:		descr = "VP8"; break;
>  		case V4L2_PIX_FMT_VP8_FRAME:    descr = "VP8 Frame"; break;
> +		case V4L2_PIX_FMT_WEBP_FRAME:    descr = "WEBP VP8 Frame"; break;
>  		case V4L2_PIX_FMT_VP9:		descr = "VP9"; break;
>  		case V4L2_PIX_FMT_VP9_FRAME:    descr = "VP9 Frame"; break;
>  		case V4L2_PIX_FMT_HEVC:		descr = "HEVC"; break; /* aka H.265 */
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index e7c4dce39007..09fff269e852 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -757,6 +757,7 @@ struct v4l2_pix_format {
>  #define V4L2_PIX_FMT_VC1_ANNEX_L v4l2_fourcc('V', 'C', '1', 'L') /* SMPTE 421M Annex L compliant stream */
>  #define V4L2_PIX_FMT_VP8      v4l2_fourcc('V', 'P', '8', '0') /* VP8 */
>  #define V4L2_PIX_FMT_VP8_FRAME v4l2_fourcc('V', 'P', '8', 'F') /* VP8 parsed frame */
> +#define V4L2_PIX_FMT_WEBP_FRAME v4l2_fourcc('W', 'B', 'P', 'F') /* WEBP VP8 parsed frame */
>  #define V4L2_PIX_FMT_VP9      v4l2_fourcc('V', 'P', '9', '0') /* VP9 */
>  #define V4L2_PIX_FMT_VP9_FRAME v4l2_fourcc('V', 'P', '9', 'F') /* VP9 parsed frame */
>  #define V4L2_PIX_FMT_HEVC     v4l2_fourcc('H', 'E', 'V', 'C') /* HEVC aka H.265 */
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

-- 
Link Mauve

Powered by blists - more mailing lists