[PATCH 0/2] Add WebP support to hantro decoder

[PATCH 0/2] Add WebP support to hantro decoder

[Hugues Fruchet]

Add WebP image decoding support to stateless V4L2 VP8 decoder.

Tested on STM32MP257F-EV1 evaluation board with GStreamer
using an updated version of V4L2 VP8 stateless decoder element:

wget https://www.gstatic.com/webp/gallery/1.webp
gst-launch-1.0 filesrc location= 1.webp ! typefind ! v4l2slvp8dec ! imagefreeze num-buffers=20 ! waylandsink fullscreen=true

Hugues Fruchet (2):
  media: uapi: add WebP VP8 frame flag
  media: verisilicon: add WebP decoding support

 .../userspace-api/media/v4l/ext-ctrls-codec-stateless.rst  | 3 +++
 drivers/media/platform/verisilicon/hantro_g1_regs.h        | 1 +
 drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c     | 7 +++++++
 include/uapi/linux/v4l2-controls.h                         | 1 +
 4 files changed, 12 insertions(+)

-- 
2.25.1

[PATCH 1/2] media: uapi: add WebP VP8 frame flag

[Hugues Fruchet]

Add a flag indicating that VP8 bitstream is a WebP picture.

Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com>
---
 .../userspace-api/media/v4l/ext-ctrls-codec-stateless.rst      | 3 +++
 include/uapi/linux/v4l2-controls.h                             | 1 +
 2 files changed, 4 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
index 0da635691fdc..bb08aacddc9c 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
@@ -1062,6 +1062,9 @@ FWHT Flags
     * - ``V4L2_VP8_FRAME_FLAG_SIGN_BIAS_ALT``
       - 0x20
       - Sign of motion vectors when the alt frame is referenced.
+    * - ``V4L2_VP8_FRAME_FLAG_WEBP``
+      - 0x40
+      - Indicates that this frame is a WebP picture.
 
 .. c:type:: v4l2_vp8_entropy_coder_state
 
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 974fd254e573..e41b62f2cb2b 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1897,6 +1897,7 @@ struct v4l2_vp8_entropy_coder_state {
 #define V4L2_VP8_FRAME_FLAG_MB_NO_SKIP_COEFF	0x08
 #define V4L2_VP8_FRAME_FLAG_SIGN_BIAS_GOLDEN	0x10
 #define V4L2_VP8_FRAME_FLAG_SIGN_BIAS_ALT	0x20
+#define V4L2_VP8_FRAME_FLAG_WEBP		0x40
 
 #define V4L2_VP8_FRAME_IS_KEY_FRAME(hdr) \
 	(!!((hdr)->flags & V4L2_VP8_FRAME_FLAG_KEY_FRAME))
-- 
2.25.1

Re: [PATCH 1/2] media: uapi: add WebP VP8 frame flag

[Nicolas Dufresne]

Hi Hugues,

Le mercredi 11 septembre 2024 à 15:50 +0200, Hugues Fruchet a écrit :
> Add a flag indicating that VP8 bitstream is a WebP picture.

Sounds like there should be some code changes in GStreamer that you haven't
disclosed. Mind sharing how this new uAPI is used ? I would also expect this
commit message to give more insight on what is special about WebP that makes
this flag required.

I would also need some more API or documentation that explain how we can
differentiate a upstream decoder that is capable of WebP decoding from one that
does not. I wonder if it would not have been better to define a new format ?
That being said, I haven't looked at all in the specification and only rely on
your cover letter and patch series.

Nicolas

> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-codec-stateless.rst      | 3 +++
>  include/uapi/linux/v4l2-controls.h                             | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
> index 0da635691fdc..bb08aacddc9c 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
> @@ -1062,6 +1062,9 @@ FWHT Flags
>      * - ``V4L2_VP8_FRAME_FLAG_SIGN_BIAS_ALT``
>        - 0x20
>        - Sign of motion vectors when the alt frame is referenced.
> +    * - ``V4L2_VP8_FRAME_FLAG_WEBP``
> +      - 0x40
> +      - Indicates that this frame is a WebP picture.
>  
>  .. c:type:: v4l2_vp8_entropy_coder_state
>  
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 974fd254e573..e41b62f2cb2b 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1897,6 +1897,7 @@ struct v4l2_vp8_entropy_coder_state {
>  #define V4L2_VP8_FRAME_FLAG_MB_NO_SKIP_COEFF	0x08
>  #define V4L2_VP8_FRAME_FLAG_SIGN_BIAS_GOLDEN	0x10
>  #define V4L2_VP8_FRAME_FLAG_SIGN_BIAS_ALT	0x20
> +#define V4L2_VP8_FRAME_FLAG_WEBP		0x40
>  
>  #define V4L2_VP8_FRAME_IS_KEY_FRAME(hdr) \
>  	(!!((hdr)->flags & V4L2_VP8_FRAME_FLAG_KEY_FRAME))

Re: [PATCH 1/2] media: uapi: add WebP VP8 frame flag

[Hugues FRUCHET]

Hi Nicolas,

Thanks for reviewing.

On 9/11/24 19:12, Nicolas Dufresne wrote:
> Hi Hugues,
> 
> Le mercredi 11 septembre 2024 à 15:50 +0200, Hugues Fruchet a écrit :
>> Add a flag indicating that VP8 bitstream is a WebP picture.
> 
> Sounds like there should be some code changes in GStreamer that you haven't
> disclosed. Mind sharing how this new uAPI is used ? I would also expect this
> commit message to give more insight on what is special about WebP that makes
> this flag required.


GStreamer changes here:
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/138ecfac54ce85b273a26ff6f0fefe3998f8d436?merge_request_iid=7505

Verisilicon datasheet is not explicit on why WebP must be signaled to 
hardware but WebP decoding fails if not.
Seems to me that such a simple addition on an already existing flag is 
something acceptable and preferable to the development of a new complete 
uAPI for WebP decoding.

> 
> I would also need some more API or documentation that explain how we can
> differentiate a upstream decoder that is capable of WebP decoding from one that
> does not. I wonder if it would not have been better to define a new format ?
> That being said, I haven't looked at all in the specification and only rely on
> your cover letter and patch series.
> 
> Nicolas


> 
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com>
>> ---
>>   .../userspace-api/media/v4l/ext-ctrls-codec-stateless.rst      | 3 +++
>>   include/uapi/linux/v4l2-controls.h                             | 1 +
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
>> index 0da635691fdc..bb08aacddc9c 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
>> @@ -1062,6 +1062,9 @@ FWHT Flags
>>       * - ``V4L2_VP8_FRAME_FLAG_SIGN_BIAS_ALT``
>>         - 0x20
>>         - Sign of motion vectors when the alt frame is referenced.
>> +    * - ``V4L2_VP8_FRAME_FLAG_WEBP``
>> +      - 0x40
>> +      - Indicates that this frame is a WebP picture.
>>   
>>   .. c:type:: v4l2_vp8_entropy_coder_state
>>   
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 974fd254e573..e41b62f2cb2b 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -1897,6 +1897,7 @@ struct v4l2_vp8_entropy_coder_state {
>>   #define V4L2_VP8_FRAME_FLAG_MB_NO_SKIP_COEFF	0x08
>>   #define V4L2_VP8_FRAME_FLAG_SIGN_BIAS_GOLDEN	0x10
>>   #define V4L2_VP8_FRAME_FLAG_SIGN_BIAS_ALT	0x20
>> +#define V4L2_VP8_FRAME_FLAG_WEBP		0x40
>>   
>>   #define V4L2_VP8_FRAME_IS_KEY_FRAME(hdr) \
>>   	(!!((hdr)->flags & V4L2_VP8_FRAME_FLAG_KEY_FRAME))
> 

BR,
Hugues.

[PATCH 2/2] media: verisilicon: add WebP decoding support

[Hugues Fruchet]

Add WebP picture decoding support to VP8 stateless decoder.

Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.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,
+				   G1_REG_ADDR_DST_CHROMA);
 }
 
 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 */
-- 
2.25.1

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

[Nicolas Dufresne]

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

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 */

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

[Nicolas Dufresne]

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@foss.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 */
> 

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

[Hugues FRUCHET]

Hi Nicolas,

Thanks for reviewing.

GStreamer changes are provided through this merge request: 
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7505

Code:
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/138ecfac54ce85b273a26ff6f0fefe3998f8d436?merge_request_iid=7505



On 9/11/24 20:44, Nicolas Dufresne wrote:
> 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@foss.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);

OK, I'll check that.

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

See below WebP image details.

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

Yes there are additional registers to set in postproc for large image > 
3472x4672 and image input bitstream larger than 16777215 bytes.
I have not tested such large images for now.
Additionally I don't have postproc support on STM32MP25.
Anyway I can guard for those limits in code...

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

As far as I have understood & tested, WebP is just an encapsulation of 
VP8 video chunk:
  * Webp image RIFF header
  *
  * 52 49 46 46 f6 00 00 00 57 45 42 50 56 50 38 20  RIFF....WEBPVP8
  * ea 00 00 00 90 09 00 9d 01 2a 30 00 30 00 3e 35  .........*0.0.>5
  *           | \______/ \______/
  *           |       |         \__VP8 startcode
  *           |        \__VP8 frame_tag
  *           |
  *            \__End of WebP RIFF header: 20 bytes, then VP8 chunk

At least for lossy WebP.

There are two others WebP formats which are loss-less WebP and animated 
WebP but untested on my side, I don't even know if those formats are 
supported by the hardware IP.

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

Really very few changes needed on VP8 codebase to support WebP. On my 
opinion it doesn't need a fork of codec for that, hence just the minor 
addition of "WebP"  signaling on uAPI see GStreamer limited changes in 
VP8 codebase to support WebP:
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/138ecfac54ce85b273a26ff6f0fefe3998f8d436?merge_request_iid=7505

>>
>>>   }
>>>   
>>>   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 */
>>
> 

BR,
Hugues.

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

[Nicolas Dufresne]

Le jeudi 12 septembre 2024 à 14:18 +0200, Hugues FRUCHET a écrit :
> Hi Nicolas,
> 
> Thanks for reviewing.
> 
> GStreamer changes are provided through this merge request: 
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7505
> 
> Code:
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/138ecfac54ce85b273a26ff6f0fefe3998f8d436?merge_request_iid=7505
> 
> 
> 
> On 9/11/24 20:44, Nicolas Dufresne wrote:
> > 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@foss.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);
> 
> OK, I'll check that.
> 
> > > 
> > > 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.
> 
> See below WebP image details.
> 
> > 
> > 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.
> > 
> 
> Yes there are additional registers to set in postproc for large image > 
> 3472x4672 and image input bitstream larger than 16777215 bytes.
> I have not tested such large images for now.
> Additionally I don't have postproc support on STM32MP25.
> Anyway I can guard for those limits in code...
> 
> > 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
> > 
> 
> As far as I have understood & tested, WebP is just an encapsulation of 
> VP8 video chunk:
>   * Webp image RIFF header
>   *
>   * 52 49 46 46 f6 00 00 00 57 45 42 50 56 50 38 20  RIFF....WEBPVP8
>   * ea 00 00 00 90 09 00 9d 01 2a 30 00 30 00 3e 35  .........*0.0.>5
>   *           | \______/ \______/
>   *           |       |         \__VP8 startcode
>   *           |        \__VP8 frame_tag
>   *           |
>   *            \__End of WebP RIFF header: 20 bytes, then VP8 chunk
> 
> At least for lossy WebP.
> 
> There are two others WebP formats which are loss-less WebP and animated 
> WebP but untested on my side, I don't even know if those formats are 
> supported by the hardware IP.
> 
> > > 
> > > 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
> 
> Really very few changes needed on VP8 codebase to support WebP. On my 
> opinion it doesn't need a fork of codec for that, hence just the minor 
> addition of "WebP"  signaling on uAPI see GStreamer limited changes in 
> VP8 codebase to support WebP:
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/138ecfac54ce85b273a26ff6f0fefe3998f8d436?merge_request_iid=7505

If it was identical, we'd need no flag. The requirement to use the flag is not
discoverable. What I'm guessing is that anything above 1080p needs the flag. But
then if you enable that flag, you loose the ability to use references, so that
would equally break normal VP8. It seems like a VP8 decoder is compatible with
WebP, but a WebP decoder is not compatible with VP8.

I cannot accept what you believe is a simple solution since its not discover-
able by userspace. The Hantro VP8 decoder driver is not the only VP8 driver, so
the GStreamer implementation would break randomly on other SoC.

My recommendation is to introduce V4L2_PIX_FMT_WEBP_FRAME, and make it so that
format reused 100% of the VP8_FRAME format (very little work, no flag needed
since the format holds that). This way, drivers can be very explicit through
their ENUM_FORMAT implementation, and can also expose different resolution
ranges properly.

Nicolas

p.s. you should draft the required synthesis check and postproc code, I can test
it for you.

> 
> > > 
> > > >   }
> > > >   
> > > >   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 */
> > > 
> > 
> 
> BR,
> Hugues.

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

[Hugues FRUCHET]

Thanks Nicolas,


On 9/12/24 15:12, Nicolas Dufresne wrote:
> Le jeudi 12 septembre 2024 à 14:18 +0200, Hugues FRUCHET a écrit :
>> Hi Nicolas,
>>
>> Thanks for reviewing.
>>
>> GStreamer changes are provided through this merge request:
>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7505
>>
>> Code:
>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/138ecfac54ce85b273a26ff6f0fefe3998f8d436?merge_request_iid=7505
>>
>>
>>
>> On 9/11/24 20:44, Nicolas Dufresne wrote:
>>> 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@foss.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);
>>
>> OK, I'll check that.
>>
>>>>
>>>> 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.
>>
>> See below WebP image details.
>>
>>>
>>> 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.
>>>
>>
>> Yes there are additional registers to set in postproc for large image >
>> 3472x4672 and image input bitstream larger than 16777215 bytes.
>> I have not tested such large images for now.
>> Additionally I don't have postproc support on STM32MP25.
>> Anyway I can guard for those limits in code...
>>
>>> 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
>>>
>>
>> As far as I have understood & tested, WebP is just an encapsulation of
>> VP8 video chunk:
>>    * Webp image RIFF header
>>    *
>>    * 52 49 46 46 f6 00 00 00 57 45 42 50 56 50 38 20  RIFF....WEBPVP8
>>    * ea 00 00 00 90 09 00 9d 01 2a 30 00 30 00 3e 35  .........*0.0.>5
>>    *           | \______/ \______/
>>    *           |       |         \__VP8 startcode
>>    *           |        \__VP8 frame_tag
>>    *           |
>>    *            \__End of WebP RIFF header: 20 bytes, then VP8 chunk
>>
>> At least for lossy WebP.
>>
>> There are two others WebP formats which are loss-less WebP and animated
>> WebP but untested on my side, I don't even know if those formats are
>> supported by the hardware IP.
>>
>>>>
>>>> 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
>>
>> Really very few changes needed on VP8 codebase to support WebP. On my
>> opinion it doesn't need a fork of codec for that, hence just the minor
>> addition of "WebP"  signaling on uAPI see GStreamer limited changes in
>> VP8 codebase to support WebP:
>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/138ecfac54ce85b273a26ff6f0fefe3998f8d436?merge_request_iid=7505
> 
> If it was identical, we'd need no flag. The requirement to use the flag is not
> discoverable. What I'm guessing is that anything above 1080p needs the flag. But
> then if you enable that flag, you loose the ability to use references, so that
> would equally break normal VP8. It seems like a VP8 decoder is compatible with
> WebP, but a WebP decoder is not compatible with VP8.
> 
> I cannot accept what you believe is a simple solution since its not discover-
> able by userspace. The Hantro VP8 decoder driver is not the only VP8 driver, so
> the GStreamer implementation would break randomly on other SoC.
> 
> My recommendation is to introduce V4L2_PIX_FMT_WEBP_FRAME, and make it so that
> format reused 100% of the VP8_FRAME format (very little work, no flag needed
> since the format holds that). This way, drivers can be very explicit through
> their ENUM_FORMAT implementation, and can also expose different resolution
> ranges properly.
> 
> Nicolas

OK, I'll wait for your comments on GStreamer side to propose a new 
kernel patchset based on this new format.

> 
> p.s. you should draft the required synthesis check and postproc code, I can test
> it for you.
> 

Thanks for that ;)

>>
>>>>
>>>>>    }
>>>>>    
>>>>>    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 */
>>>>
>>>
>>
>> BR,
>> Hugues.
> 
BR,
Hugues.