linux-kernel - Re: [PATCH v2 1/3] media: uapi: add WebP uAPI

Open Source and information security mailing list archives

Message-ID: <00c0405112b808929f6a7b8e8375b4182c26086e.camel@collabora.com>
Date: Wed, 20 Nov 2024 08:57:48 -0500
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Link Mauve <linkmauve@...kmauve.fr>, 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>, 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,

Le mercredi 20 novembre 2024 à 14:42 +0100, Link Mauve a écrit :
> 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.

This is a v2, you should have read v1 thread before commenting this in my
opinion. Let me resume it: While a VP8 HW decoder is compatible with WebP (lossy
only), a WebP (lossy) decoders are not always compatible wit VP8. The reason is
that some design may sacrifice support for references in order to allow larger
resolutions.

This is proven by Hantro implementation, which is the main implementation out
there since back when Google owned it this hardware design was given away for
free to anyone wanting to support VP8 in HW.

> 
> 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.

I don't expect the RIFF to be passed to the stateless decoder, this is a
stateless interface that is proposed here. Though, you need the RIFF to determin
if the stream is Lossy WebP (keyframe only), so if someone comes up with a
stateful HW design, it will face the same challenge. As said, what you do in
Onix is valid, but limiting. 

Let's first have a look at the code before taking out big guns, also make sure
you have the full context before proposing to nack something.

regards,
Nicolas

> 
> [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
> 

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives