[FFmpeg-devel] [PATCH] lavc/vc1dsp: match C block layout in inv_trans_4x4_rvv

[FFmpeg-devel] [PATCH] lavc/vc1dsp: match C block layout in inv_trans_4x4_rvv

[Rémi Denis-Courmont]

Although checkasm does not verify this, the decoder requires that the
transform updates the input block exactly like the C code does.

This fixes vc1-ism, vc1_ilaced_twomv, vc1_sa00040, vc1_sa10091,
vc1_sa10143, vc1_sa20021, vc1test_smm0005 and wmv3-drm-dec tests.
---
 libavcodec/riscv/vc1dsp_rvv.S | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavcodec/riscv/vc1dsp_rvv.S b/libavcodec/riscv/vc1dsp_rvv.S
index ab10027ae5..c4517d54f5 100644
--- a/libavcodec/riscv/vc1dsp_rvv.S
+++ b/libavcodec/riscv/vc1dsp_rvv.S
@@ -380,12 +380,12 @@ func ff_vc1_inv_trans_4x4_rvv, zve32x
         vlsseg4e16.v v0, (a2), a3
         li           t1, 3
         jal          t0, ff_vc1_inv_trans_4_rvv
-        vsseg4e16.v  v0, (a2)
-        addi         t1, a2, 1 * 4 * 2
+        vssseg4e16.v v0, (a2), a3
+        addi         t1, a2, 2 * 4 * 2
         vle16.v      v0, (a2)
-        addi         t2, a2, 2 * 4 * 2
+        addi         t2, a2, 4 * 4 * 2
         vle16.v      v1, (t1)
-        addi         t3, a2, 3 * 4 * 2
+        addi         t3, a2, 6 * 4 * 2
         vle16.v      v2, (t2)
         vle16.v      v3, (t3)
         li           t1, 7
-- 
2.45.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] lavc/vc1dsp: match C block layout in inv_trans_4x8_rvv

[Rémi Denis-Courmont]

Although checkasm does not verify this, the decoder requires that the
transform updates the input block exactly like the C code does.

This fixes vc1-ism, vc1_ilaced_twomv, vc1_sa00040, vc1_sa10091,
vc1_sa10143, vc1_sa20021, vc1test_smm0005 and wmv3-drm-dec tests.
---
 libavcodec/riscv/vc1dsp_rvv.S | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/libavcodec/riscv/vc1dsp_rvv.S b/libavcodec/riscv/vc1dsp_rvv.S
index c4517d54f5..860b0cc5b1 100644
--- a/libavcodec/riscv/vc1dsp_rvv.S
+++ b/libavcodec/riscv/vc1dsp_rvv.S
@@ -303,15 +303,24 @@ func ff_vc1_inv_trans_4x8_rvv, zve32x
         vlsseg4e16.v v0, (a2), a3
         li           t1, 3
         jal          t0, ff_vc1_inv_trans_4_rvv
+        vssseg4e16.v v0, (a2), a3
+        vsetivli     zero, 4, e16, mf2, ta, ma
         addi         t1, a2, 1 * 8 * 2
-        vse16.v      v0, (a2)
+        vle16.v      v0, (a2)
         addi         t2, a2, 2 * 8 * 2
-        vse16.v      v1, (t1)
+        vle16.v      v1, (t1)
         addi         t3, a2, 3 * 8 * 2
-        vse16.v      v2, (t2)
-        vse16.v      v3, (t3)
-        vsetivli     zero, 4, e16, mf2, ta, ma
-        vlseg8e16.v  v0, (a2)
+        vle16.v      v2, (t2)
+        addi         t4, a2, 4 * 8 * 2
+        vle16.v      v3, (t3)
+        addi         t5, a2, 5 * 8 * 2
+        vle16.v      v4, (t4)
+        addi         t6, a2, 6 * 8 * 2
+        vle16.v      v5, (t5)
+        addi         t1, a2, 7 * 8 * 2
+        vle16.v      v6, (t6)
+        vle16.v      v7, (t1)
+
         jal          t0, ff_vc1_inv_trans_8_rvv
         vadd.vi      v4, v4, 1
         add          t0, a1, a0
-- 
2.45.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vc1dsp: match C block layout in inv_trans_4x8_rvv

[Ronald S. Bultje]

Hi,

On Mon, Jun 10, 2024 at 3:20 PM Rémi Denis-Courmont <remi@remlab.net> wrote:

> Although checkasm does not verify this, the decoder requires that the
> transform updates the input block exactly like the C code does.
>

Would it be possible to update the checkasm test to verify this?

Ronald
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vc1dsp: match C block layout in inv_trans_4x8_rvv

[Rémi Denis-Courmont]

Le 11 juin 2024 15:09:42 GMT+03:00, "Ronald S. Bultje" <rsbultje@gmail.com> a écrit :
>Hi,
>
>On Mon, Jun 10, 2024 at 3:20 PM Rémi Denis-Courmont <remi@remlab.net> wrote:
>
>> Although checkasm does not verify this, the decoder requires that the
>> transform updates the input block exactly like the C code does.
>>
>
>Would it be possible to update the checkasm test to verify this?

In theory, it has to be possible. I can't do it for lack of knowledge about VC-1 though. James tried to make such a patch but it ended up failing both AArch64 and RVV 8x4 transforms. I don't know if this points to a latent bug or is a false positive.

>
>Ronald
>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel@ffmpeg.org
>https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>To unsubscribe, visit link above, or email
>ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vc1dsp: match C block layout in inv_trans_4x8_rvv

[James Almer]

On 6/11/2024 9:19 AM, Rémi Denis-Courmont wrote:
> 
> 
> Le 11 juin 2024 15:09:42 GMT+03:00, "Ronald S. Bultje" <rsbultje@gmail.com> a écrit :
>> Hi,
>>
>> On Mon, Jun 10, 2024 at 3:20 PM Rémi Denis-Courmont <remi@remlab.net> wrote:
>>
>>> Although checkasm does not verify this, the decoder requires that the
>>> transform updates the input block exactly like the C code does.
>>>
>>
>> Would it be possible to update the checkasm test to verify this?
> 
> In theory, it has to be possible. I can't do it for lack of knowledge about VC-1 though. James tried to make such a patch but it ended up failing both AArch64 and RVV 8x4 transforms. I don't know if this points to a latent bug or is a false positive.

My patch added a memcmp of the block buffer. If it fails, then the asm 
implementation is probably writing more than it should.
Can you see what bytes don't match? It might give you an idea of what 
store instruction is touching the wrong bytes.

> 
>>
>> Ronald
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vc1dsp: match C block layout in inv_trans_4x8_rvv

[Rémi Denis-Courmont]

Le tiistaina 11. kesäkuuta 2024, 15.23.11 EEST James Almer a écrit :
> On 6/11/2024 9:19 AM, Rémi Denis-Courmont wrote:
> > Le 11 juin 2024 15:09:42 GMT+03:00, "Ronald S. Bultje" 
<rsbultje@gmail.com> a écrit :
> >> Hi,
> >> 
> >> On Mon, Jun 10, 2024 at 3:20 PM Rémi Denis-Courmont <remi@remlab.net> 
wrote:
> >>> Although checkasm does not verify this, the decoder requires that the
> >>> transform updates the input block exactly like the C code does.
> >> 
> >> Would it be possible to update the checkasm test to verify this?
> > 
> > In theory, it has to be possible. I can't do it for lack of knowledge
> > about VC-1 though. James tried to make such a patch but it ended up
> > failing both AArch64 and RVV 8x4 transforms. I don't know if this points
> > to a latent bug or is a false positive.
> My patch added a memcmp of the block buffer. If it fails, then the asm
> implementation is probably writing more than it should.

The patch has two bugs that end up cancelling one another.

> Can you see what bytes don't match? It might give you an idea of what
> store instruction is touching the wrong bytes.

The RVV transform is failing because it stores different values in the right 
places. I will attach patches here, but it is entirely unclear to me if this 
is necessary at all; at least, it does not seem to fix anything in FATE, and it 
causes the pre-existing AArch64 functions to fail checkasm.

-- 
レミ・デニ-クールモン
http://www.remlab.net/



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 1/2] lavc/vc1dsp: match C block content in inv_trans_8x4_rvv

[Rémi Denis-Courmont]

This shifts the mid-point (after horizontal, before vertical) block
state of the transform to match the C code. This forces shifting 8
vectors of 4 elements instead of 4 vectors of 8 elements and is thus
slight slower.
---
 libavcodec/riscv/vc1dsp_rvv.S | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/libavcodec/riscv/vc1dsp_rvv.S b/libavcodec/riscv/vc1dsp_rvv.S
index 4b7ab33307..7e1fb84b0c 100644
--- a/libavcodec/riscv/vc1dsp_rvv.S
+++ b/libavcodec/riscv/vc1dsp_rvv.S
@@ -257,6 +257,9 @@ func ff_vc1_inv_trans_8x4_rvv, zve32x
         vsetivli    zero, 4, e16, mf2, ta, ma
         vlseg8e16.v v0, (a2)
         jal         t0, ff_vc1_inv_trans_8_rvv
+        .irp    n,0,1,2,3,4,5,6,7
+        vssra.vi    v\n, v\n, 3
+        .endr
         vsseg8e16.v v0, (a2)
         addi        a3, a2, 1 * 8 * 2
         vsetivli    zero, 8, e16, m1, ta, ma
@@ -266,10 +269,6 @@ func ff_vc1_inv_trans_8x4_rvv, zve32x
         addi        a5, a2, 3 * 8 * 2
         vle16.v     v2, (a4)
         vle16.v     v3, (a5)
-        .irp    n,0,1,2,3
-        # shift 4 vectors of 8 elems after transpose instead of 8 of 4
-        vssra.vi    v\n, v\n, 3
-        .endr
         li          t1, 7
         jal         t0, ff_vc1_inv_trans_4_rvv
         add         a3, a1, a0
-- 
2.45.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2] checkasm/vc1dsp: check the not-in-place block content

[Rémi Denis-Courmont]

This seems to cause issues in FATE for 4x4 and 4x8 transforms. But then
again, FATE does not seem to care in the 8x4 case.

Note that AArch64 NEON code is known to fail this test.
---
 tests/checkasm/vc1dsp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/checkasm/vc1dsp.c b/tests/checkasm/vc1dsp.c
index f18f0f8251..2cc6785a0c 100644
--- a/tests/checkasm/vc1dsp.c
+++ b/tests/checkasm/vc1dsp.c
@@ -317,11 +317,13 @@ static void check_inv_trans_adding(void)
             for (int j = 0; j < tests[t].height; ++j)
                 for (int i = 0; i < tests[t].width; ++i) {
                     int idx = j * 8 + i;
-                    inv_trans_in1[idx] = inv_trans_in0[idx] = coeffs->d[j * tests[t].width + i];
+                    inv_trans_in0[idx] = coeffs->d[j * tests[t].width + i];
                 }
+            memcpy(inv_trans_in1, inv_trans_in0, 8 * 8 * 2);
             call_ref(inv_trans_out0 + 24 + 8, 24, inv_trans_in0);
             call_new(inv_trans_out1 + 24 + 8, 24, inv_trans_in1);
-            if (memcmp(inv_trans_out0, inv_trans_out1, 10 * 24))
+            if (memcmp(inv_trans_in0,   inv_trans_in1, 8 * 8 * 2) ||
+                memcmp(inv_trans_out0, inv_trans_out1, 10 * 24))
                 fail();
             bench_new(inv_trans_out1 + 24 + 8, 24, inv_trans_in1 + 8);
             av_free(coeffs);
-- 
2.45.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] checkasm/vc1dsp: check the not-in-place block content

[James Almer]

On 6/11/2024 11:55 AM, Rémi Denis-Courmont wrote:
> This seems to cause issues in FATE for 4x4 and 4x8 transforms. But then
> again, FATE does not seem to care in the 8x4 case.
> 
> Note that AArch64 NEON code is known to fail this test.
> ---
>   tests/checkasm/vc1dsp.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/checkasm/vc1dsp.c b/tests/checkasm/vc1dsp.c
> index f18f0f8251..2cc6785a0c 100644
> --- a/tests/checkasm/vc1dsp.c
> +++ b/tests/checkasm/vc1dsp.c
> @@ -317,11 +317,13 @@ static void check_inv_trans_adding(void)
>               for (int j = 0; j < tests[t].height; ++j)
>                   for (int i = 0; i < tests[t].width; ++i) {
>                       int idx = j * 8 + i;
> -                    inv_trans_in1[idx] = inv_trans_in0[idx] = coeffs->d[j * tests[t].width + i];
> +                    inv_trans_in0[idx] = coeffs->d[j * tests[t].width + i];
>                   }
> +            memcpy(inv_trans_in1, inv_trans_in0, 8 * 8 * 2);

Is there any gain doing this?

>               call_ref(inv_trans_out0 + 24 + 8, 24, inv_trans_in0);
>               call_new(inv_trans_out1 + 24 + 8, 24, inv_trans_in1);
> -            if (memcmp(inv_trans_out0, inv_trans_out1, 10 * 24))
> +            if (memcmp(inv_trans_in0,   inv_trans_in1, 8 * 8 * 2) ||

nit: sizeof (int16_t)

> +                memcmp(inv_trans_out0, inv_trans_out1, 10 * 24))
>                   fail();
>               bench_new(inv_trans_out1 + 24 + 8, 24, inv_trans_in1 + 8);
>               av_free(coeffs);
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] checkasm/vc1dsp: check the not-in-place block content

[Rémi Denis-Courmont]

Le tiistaina 11. kesäkuuta 2024, 18.08.51 EEST James Almer a écrit :
> On 6/11/2024 11:55 AM, Rémi Denis-Courmont wrote:
> > This seems to cause issues in FATE for 4x4 and 4x8 transforms. But then
> > again, FATE does not seem to care in the 8x4 case.
> > 
> > Note that AArch64 NEON code is known to fail this test.
> > ---
> > 
> >   tests/checkasm/vc1dsp.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/checkasm/vc1dsp.c b/tests/checkasm/vc1dsp.c
> > index f18f0f8251..2cc6785a0c 100644
> > --- a/tests/checkasm/vc1dsp.c
> > +++ b/tests/checkasm/vc1dsp.c
> > @@ -317,11 +317,13 @@ static void check_inv_trans_adding(void)
> > 
> >               for (int j = 0; j < tests[t].height; ++j)
> >               
> >                   for (int i = 0; i < tests[t].width; ++i) {
> >                   
> >                       int idx = j * 8 + i;
> > 
> > -                    inv_trans_in1[idx] = inv_trans_in0[idx] = coeffs->d[j
> > * tests[t].width + i]; +                    inv_trans_in0[idx] =
> > coeffs->d[j * tests[t].width + i];> 
> >                   }
> > 
> > +            memcpy(inv_trans_in1, inv_trans_in0, 8 * 8 * 2);
> 
> Is there any gain doing this?

Checking that assembler does not write out of bounds in the later memcmp().

> >               call_ref(inv_trans_out0 + 24 + 8, 24, inv_trans_in0);
> >               call_new(inv_trans_out1 + 24 + 8, 24, inv_trans_in1);
> > 
> > -            if (memcmp(inv_trans_out0, inv_trans_out1, 10 * 24))
> > +            if (memcmp(inv_trans_in0,   inv_trans_in1, 8 * 8 * 2) ||
> 
> nit: sizeof (int16_t)

Yes well this code cannot be merged because we (at least I) do not know if it 
is correct, and it breaks AArch64.

> > +                memcmp(inv_trans_out0, inv_trans_out1, 10 * 24))
> > 
> >                   fail();
> >               
> >               bench_new(inv_trans_out1 + 24 + 8, 24, inv_trans_in1 + 8);
> >               av_free(coeffs);
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".


-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".