linux-kernel - Re: [PATCH 2/2] media: verisilicon: add WebP decoding support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <01020191e2672cd9-0b3804cc-def2-4dfb-aa44-8eddbd5e99fb-000000@eu-west-1.amazonses.com>
Date: Wed, 11 Sep 2024 18:44:58 +0000
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Hugues Fruchet <hugues.fruchet@...s.st.com>, 
	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>, 
	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 2/2] media: verisilicon: add WebP decoding support

Le mercredi 11 septembre 2024 à 13:58 -0400, Nicolas Dufresne a écrit :
> Hi Hugues,
> 
> Le mercredi 11 septembre 2024 à 15:50 +0200, Hugues Fruchet a écrit :
> > Add WebP picture decoding support to VP8 stateless decoder.
> 
> Unless when its obvious, the commit message should explain what is being
> changed.
> 
> > 
> > Signed-off-by: Hugues Fruchet <hugues.fruchet@...s.st.com>
> > ---
> >  drivers/media/platform/verisilicon/hantro_g1_regs.h    | 1 +
> >  drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c | 7 +++++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/media/platform/verisilicon/hantro_g1_regs.h b/drivers/media/platform/verisilicon/hantro_g1_regs.h
> > index c623b3b0be18..e7d4db788e57 100644
> > --- a/drivers/media/platform/verisilicon/hantro_g1_regs.h
> > +++ b/drivers/media/platform/verisilicon/hantro_g1_regs.h
> > @@ -232,6 +232,7 @@
> >  #define     G1_REG_DEC_CTRL7_DCT7_START_BIT(x)		(((x) & 0x3f) << 0)
> >  #define G1_REG_ADDR_STR					0x030
> >  #define G1_REG_ADDR_DST					0x034
> > +#define G1_REG_ADDR_DST_CHROMA				0x038
> >  #define G1_REG_ADDR_REF(i)				(0x038 + ((i) * 0x4))
> >  #define     G1_REG_ADDR_REF_FIELD_E			BIT(1)
> >  #define     G1_REG_ADDR_REF_TOPC_E			BIT(0)
> > diff --git a/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c b/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c
> > index 851eb67f19f5..c6a7584b716a 100644
> > --- a/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c
> > +++ b/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c
> > @@ -427,6 +427,11 @@ static void cfg_buffers(struct hantro_ctx *ctx,
> >  
> >  	dst_dma = hantro_get_dec_buf_addr(ctx, &vb2_dst->vb2_buf);
> >  	vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST);
> > +
> > +	if (hdr->flags & V4L2_VP8_FRAME_FLAG_WEBP)
> > +		vdpu_write_relaxed(vpu, dst_dma +
> > +				   ctx->dst_fmt.height * ctx->dst_fmt.width,
> 
> I'm not really not fan of that type of formula using padded width/height. Not
> sure if its supported already, but if we have foreign buffers with a bigger
> bytesperline, the IP may endup overwriting the luma. Please use the per-plane
> bytesperline, we have v4l2-common to help with that when needed.
> > +				   G1_REG_ADDR_DST_CHROMA);
> 
> I have a strong impression this patch is incomplete (not generic enough). The
> documentation I have indicates that the resolution range for WebP can be
> different for different synthesis. See swreg54 (0xd8), if bit 19 is set, then it
> can support 16K x 16K resolution. There is no other way around that then
> signalling explicitly at the format level that this is webp, since otherwise you
> can't know from userspace and can't enumerate the different resolution. I'm
> curious what is the difference at bitstream level, would be nice to clarify too.

I've also found that when the PP is used, you need to fill some extended
dimension (SWREG92) with the missing bit of the width/height, as the dimension
don't fit the usual register.

More notes, I noticed that WebP supports having a second frame for the alpha,
similar to WebM Alpha, for that we expect 2 requests, so no issue on this front.
WebP Loss-less is a completely different codec, and should have its own format.

I think overall, from my read of the spec, that its normal VP8, but the
resolution will exceed the normal one. We also can't always enable WebP, since
it will break references.

Nicolas

> 
> On GStreamer side, the formats are entirely seperate, image/webp vs video/x-vp8
> are the mime types. Seems a lot safe to keep these two as seperate formats. They
> can certainly share the same stateless frame structure, with the additional flag
> imho.
> 
> Nicolas
> 
> >  }
> >  
> >  int hantro_g1_vp8_dec_run(struct hantro_ctx *ctx)
> > @@ -471,6 +476,8 @@ int hantro_g1_vp8_dec_run(struct hantro_ctx *ctx)
> >  		reg |= G1_REG_DEC_CTRL0_SKIP_MODE;
> >  	if (hdr->lf.level == 0)
> >  		reg |= G1_REG_DEC_CTRL0_FILTERING_DIS;
> > +	if (hdr->flags & V4L2_VP8_FRAME_FLAG_WEBP)
> > +		reg |= G1_REG_DEC_CTRL0_WEBP_E;
> >  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
> >  
> >  	/* Frame dimensions */
>