Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
* [FFmpeg-devel] [PATCH] checkasm: h264dsp: test luma_dc_dequant
@ 2025-06-12 17:25 Tristan Matthews
  2025-06-12 20:13 ` Martin Storsjö
  0 siblings, 1 reply; 13+ messages in thread
From: Tristan Matthews @ 2025-06-12 17:25 UTC (permalink / raw)
  To: ffmpeg-devel; +Cc: Tristan Matthews

---
 tests/checkasm/h264dsp.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
index d1228ed985..5fba31cf69 100644
--- a/tests/checkasm/h264dsp.c
+++ b/tests/checkasm/h264dsp.c
@@ -22,6 +22,7 @@
 #include "checkasm.h"
 #include "libavcodec/h264dsp.h"
 #include "libavcodec/h264data.h"
+#include "libavcodec/h264idct.h"
 #include "libavcodec/h264_parse.h"
 #include "libavutil/common.h"
 #include "libavutil/intreadwrite.h"
@@ -324,6 +325,41 @@ static void check_idct_multiple(void)
     }
 }
 
+static void check_idct_dequant(void)
+{
+    static const int depths[5] = { 8, 9, 10, 12, 14 };
+    LOCAL_ALIGNED_16(int16_t, src, [16]);
+    LOCAL_ALIGNED_16(int16_t, dst0, [16 * 16]);
+    LOCAL_ALIGNED_16(int16_t, dst1, [16 * 16]);
+    H264DSPContext h;
+    int bit_depth, i, qmul;
+    declare_func_emms(AV_CPU_FLAG_MMX | AV_CPU_FLAG_SSE2, void, int16_t *output, int16_t *input, int qmul);
+
+    for (int j = 0; j < 16; j++)
+        src[j] = (rnd() % 512) - 256;
+
+    qmul = rnd() % 4096;
+
+    memset(dst0, 0, 16 * 16 * sizeof(dst0[0]));
+    memset(dst1, 0, 16 * 16 * sizeof(dst1[0]));
+
+    for (i = 0; i < FF_ARRAY_ELEMS(depths); i++) {
+        bit_depth = depths[i];
+        ff_h264dsp_init(&h, bit_depth, 1);
+
+        if (check_func(h.h264_luma_dc_dequant_idct, "h264_luma_dc_dequant_idct_%d", bit_depth)) {
+
+            call_ref(dst0, src, qmul);
+            call_new(dst1, src, qmul);
+
+            if (memcmp(dst0, dst1, 16 * 16 * sizeof(*dst0)))
+                fail();
+
+            bench_new(dst1, src, qmul);
+        }
+    }
+}
+
 
 static void check_loop_filter(void)
 {
@@ -453,6 +489,7 @@ void checkasm_check_h264dsp(void)
 {
     check_idct();
     check_idct_multiple();
+    check_idct_dequant();
     report("idct");
 
     check_loop_filter();
-- 
2.48.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".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] checkasm: h264dsp: test luma_dc_dequant
  2025-06-12 17:25 [FFmpeg-devel] [PATCH] checkasm: h264dsp: test luma_dc_dequant Tristan Matthews
@ 2025-06-12 20:13 ` Martin Storsjö
  2025-06-13  4:11   ` Tristan Matthews
  0 siblings, 1 reply; 13+ messages in thread
From: Martin Storsjö @ 2025-06-12 20:13 UTC (permalink / raw)
  To: FFmpeg development discussions and patches; +Cc: Tristan Matthews

On Thu, 12 Jun 2025, Tristan Matthews wrote:

> ---
> tests/checkasm/h264dsp.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
> index d1228ed985..5fba31cf69 100644
> --- a/tests/checkasm/h264dsp.c
> +++ b/tests/checkasm/h264dsp.c
> @@ -22,6 +22,7 @@
> #include "checkasm.h"
> #include "libavcodec/h264dsp.h"
> #include "libavcodec/h264data.h"
> +#include "libavcodec/h264idct.h"
> #include "libavcodec/h264_parse.h"
> #include "libavutil/common.h"
> #include "libavutil/intreadwrite.h"
> @@ -324,6 +325,41 @@ static void check_idct_multiple(void)
>     }
> }
>
> +static void check_idct_dequant(void)
> +{
> +    static const int depths[5] = { 8, 9, 10, 12, 14 };
> +    LOCAL_ALIGNED_16(int16_t, src, [16]);
> +    LOCAL_ALIGNED_16(int16_t, dst0, [16 * 16]);
> +    LOCAL_ALIGNED_16(int16_t, dst1, [16 * 16]);
> +    H264DSPContext h;
> +    int bit_depth, i, qmul;
> +    declare_func_emms(AV_CPU_FLAG_MMX | AV_CPU_FLAG_SSE2, void, int16_t *output, int16_t *input, int qmul);
> +
> +    for (int j = 0; j < 16; j++)
> +        src[j] = (rnd() % 512) - 256;
> +
> +    qmul = rnd() % 4096;
> +
> +    memset(dst0, 0, 16 * 16 * sizeof(dst0[0]));
> +    memset(dst1, 0, 16 * 16 * sizeof(dst1[0]));
> +
> +    for (i = 0; i < FF_ARRAY_ELEMS(depths); i++) {
> +        bit_depth = depths[i];
> +        ff_h264dsp_init(&h, bit_depth, 1);
> +
> +        if (check_func(h.h264_luma_dc_dequant_idct, "h264_luma_dc_dequant_idct_%d", bit_depth)) {
> +
> +            call_ref(dst0, src, qmul);
> +            call_new(dst1, src, qmul);
> +
> +            if (memcmp(dst0, dst1, 16 * 16 * sizeof(*dst0)))
> +                fail();

If possible, use the checkasm_check_*() helpers for validation for new 
code; this gives you printout of the differing values if you run "checkasm 
-v" and more. In this case, I think checkasm_check(int16_t, dst0, 
16*sizeof(int16_t), dst1, 16*sizeof(int16_t), 16, 16, "dst") would be 
suitable one.

// Martin

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] checkasm: h264dsp: test luma_dc_dequant
  2025-06-12 20:13 ` Martin Storsjö
@ 2025-06-13  4:11   ` Tristan Matthews
  2025-06-13  4:12     ` [FFmpeg-devel] [PATCH 1/1] " Tristan Matthews
  2025-06-13  6:08     ` [FFmpeg-devel] [PATCH] " Martin Storsjö
  0 siblings, 2 replies; 13+ messages in thread
From: Tristan Matthews @ 2025-06-13  4:11 UTC (permalink / raw)
  To: Martin Storsjö; +Cc: FFmpeg development discussions and patches

On Thu, Jun 12, 2025 at 4:14 PM Martin Storsjö <martin@martin.st> wrote:
>
> On Thu, 12 Jun 2025, Tristan Matthews wrote:
>
> > ---
> > tests/checkasm/h264dsp.c | 37 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
> > index d1228ed985..5fba31cf69 100644
> > --- a/tests/checkasm/h264dsp.c
> > +++ b/tests/checkasm/h264dsp.c
> > @@ -22,6 +22,7 @@
> > #include "checkasm.h"
> > #include "libavcodec/h264dsp.h"
> > #include "libavcodec/h264data.h"
> > +#include "libavcodec/h264idct.h"
> > #include "libavcodec/h264_parse.h"
> > #include "libavutil/common.h"
> > #include "libavutil/intreadwrite.h"
> > @@ -324,6 +325,41 @@ static void check_idct_multiple(void)
> >     }
> > }
> >
> > +static void check_idct_dequant(void)
> > +{
> > +    static const int depths[5] = { 8, 9, 10, 12, 14 };
> > +    LOCAL_ALIGNED_16(int16_t, src, [16]);
> > +    LOCAL_ALIGNED_16(int16_t, dst0, [16 * 16]);
> > +    LOCAL_ALIGNED_16(int16_t, dst1, [16 * 16]);
> > +    H264DSPContext h;
> > +    int bit_depth, i, qmul;
> > +    declare_func_emms(AV_CPU_FLAG_MMX | AV_CPU_FLAG_SSE2, void, int16_t *output, int16_t *input, int qmul);
> > +
> > +    for (int j = 0; j < 16; j++)
> > +        src[j] = (rnd() % 512) - 256;
> > +
> > +    qmul = rnd() % 4096;
> > +
> > +    memset(dst0, 0, 16 * 16 * sizeof(dst0[0]));
> > +    memset(dst1, 0, 16 * 16 * sizeof(dst1[0]));
> > +
> > +    for (i = 0; i < FF_ARRAY_ELEMS(depths); i++) {
> > +        bit_depth = depths[i];
> > +        ff_h264dsp_init(&h, bit_depth, 1);
> > +
> > +        if (check_func(h.h264_luma_dc_dequant_idct, "h264_luma_dc_dequant_idct_%d", bit_depth)) {
> > +
> > +            call_ref(dst0, src, qmul);
> > +            call_new(dst1, src, qmul);
> > +
> > +            if (memcmp(dst0, dst1, 16 * 16 * sizeof(*dst0)))
> > +                fail();
>
> If possible, use the checkasm_check_*() helpers for validation for new
> code; this gives you printout of the differing values if you run "checkasm
> -v" and more. In this case, I think checkasm_check(int16_t, dst0,
> 16*sizeof(int16_t), dst1, 16*sizeof(int16_t), 16, 16, "dst") would be
> suitable one.

Good catch, also I realized that the output buffers were too small,
will be fixed in the next version.
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [FFmpeg-devel] [PATCH 1/1] checkasm: h264dsp: test luma_dc_dequant
  2025-06-13  4:11   ` Tristan Matthews
@ 2025-06-13  4:12     ` Tristan Matthews
  2025-06-13  6:08     ` [FFmpeg-devel] [PATCH] " Martin Storsjö
  1 sibling, 0 replies; 13+ messages in thread
From: Tristan Matthews @ 2025-06-13  4:12 UTC (permalink / raw)
  To: ffmpeg-devel; +Cc: Tristan Matthews

---
 tests/checkasm/h264dsp.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
index d1228ed985..96540565af 100644
--- a/tests/checkasm/h264dsp.c
+++ b/tests/checkasm/h264dsp.c
@@ -22,6 +22,7 @@
 #include "checkasm.h"
 #include "libavcodec/h264dsp.h"
 #include "libavcodec/h264data.h"
+#include "libavcodec/h264idct.h"
 #include "libavcodec/h264_parse.h"
 #include "libavutil/common.h"
 #include "libavutil/intreadwrite.h"
@@ -324,6 +325,38 @@ static void check_idct_multiple(void)
     }
 }
 
+static void check_idct_dequant(void)
+{
+    static const int depths[5] = { 8, 9, 10, 12, 14 };
+    LOCAL_ALIGNED_16(int16_t, src, [16]);
+    LOCAL_ALIGNED_16(int16_t, dst0, [16 * 16 * 2]);
+    LOCAL_ALIGNED_16(int16_t, dst1, [16 * 16 * 2]);
+    H264DSPContext h;
+    int bit_depth, i, qmul;
+    declare_func_emms(AV_CPU_FLAG_MMX | AV_CPU_FLAG_SSE2, void, int16_t *output, int16_t *input, int qmul);
+
+    for (int j = 0; j < 16; j++)
+        src[j] = (rnd() % 512) - 256;
+
+    qmul = rnd() % 4096;
+
+    for (i = 0; i < FF_ARRAY_ELEMS(depths); i++) {
+        bit_depth = depths[i];
+        ff_h264dsp_init(&h, bit_depth, 1);
+
+        memset(dst0, 0, 16 * 16 * SIZEOF_COEF);
+        memset(dst1, 0, 16 * 16 * SIZEOF_COEF);
+
+        if (check_func(h.h264_luma_dc_dequant_idct, "h264_luma_dc_dequant_idct_%d", bit_depth)) {
+
+            call_ref(dst0, src, qmul);
+            call_new(dst1, src, qmul);
+            checkasm_check(int16_t, dst0, 16*sizeof(int16_t), dst1, 16*sizeof(int16_t), 16, 16, "dst");
+            bench_new(dst1, src, qmul);
+        }
+    }
+}
+
 
 static void check_loop_filter(void)
 {
@@ -453,6 +486,7 @@ void checkasm_check_h264dsp(void)
 {
     check_idct();
     check_idct_multiple();
+    check_idct_dequant();
     report("idct");
 
     check_loop_filter();
-- 
2.48.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".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] checkasm: h264dsp: test luma_dc_dequant
  2025-06-13  4:11   ` Tristan Matthews
  2025-06-13  4:12     ` [FFmpeg-devel] [PATCH 1/1] " Tristan Matthews
@ 2025-06-13  6:08     ` Martin Storsjö
  2025-06-13 13:21       ` Tristan Matthews
  1 sibling, 1 reply; 13+ messages in thread
From: Martin Storsjö @ 2025-06-13  6:08 UTC (permalink / raw)
  To: Tristan Matthews; +Cc: FFmpeg development discussions and patches

On Fri, 13 Jun 2025, Tristan Matthews wrote:

> On Thu, Jun 12, 2025 at 4:14 PM Martin Storsjö <martin@martin.st> wrote:
>>
>> On Thu, 12 Jun 2025, Tristan Matthews wrote:
>>
>>> ---
>>> tests/checkasm/h264dsp.c | 37 +++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 37 insertions(+)
>>>
>>> diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
>>> index d1228ed985..5fba31cf69 100644
>>> --- a/tests/checkasm/h264dsp.c
>>> +++ b/tests/checkasm/h264dsp.c
>>> @@ -22,6 +22,7 @@
>>> #include "checkasm.h"
>>> #include "libavcodec/h264dsp.h"
>>> #include "libavcodec/h264data.h"
>>> +#include "libavcodec/h264idct.h"
>>> #include "libavcodec/h264_parse.h"
>>> #include "libavutil/common.h"
>>> #include "libavutil/intreadwrite.h"
>>> @@ -324,6 +325,41 @@ static void check_idct_multiple(void)
>>>     }
>>> }
>>>
>>> +static void check_idct_dequant(void)
>>> +{
>>> +    static const int depths[5] = { 8, 9, 10, 12, 14 };
>>> +    LOCAL_ALIGNED_16(int16_t, src, [16]);
>>> +    LOCAL_ALIGNED_16(int16_t, dst0, [16 * 16]);
>>> +    LOCAL_ALIGNED_16(int16_t, dst1, [16 * 16]);
>>> +    H264DSPContext h;
>>> +    int bit_depth, i, qmul;
>>> +    declare_func_emms(AV_CPU_FLAG_MMX | AV_CPU_FLAG_SSE2, void, int16_t *output, int16_t *input, int qmul);
>>> +
>>> +    for (int j = 0; j < 16; j++)
>>> +        src[j] = (rnd() % 512) - 256;
>>> +
>>> +    qmul = rnd() % 4096;
>>> +
>>> +    memset(dst0, 0, 16 * 16 * sizeof(dst0[0]));
>>> +    memset(dst1, 0, 16 * 16 * sizeof(dst1[0]));
>>> +
>>> +    for (i = 0; i < FF_ARRAY_ELEMS(depths); i++) {
>>> +        bit_depth = depths[i];
>>> +        ff_h264dsp_init(&h, bit_depth, 1);
>>> +
>>> +        if (check_func(h.h264_luma_dc_dequant_idct, "h264_luma_dc_dequant_idct_%d", bit_depth)) {
>>> +
>>> +            call_ref(dst0, src, qmul);
>>> +            call_new(dst1, src, qmul);
>>> +
>>> +            if (memcmp(dst0, dst1, 16 * 16 * sizeof(*dst0)))
>>> +                fail();
>>
>> If possible, use the checkasm_check_*() helpers for validation for new
>> code; this gives you printout of the differing values if you run "checkasm
>> -v" and more. In this case, I think checkasm_check(int16_t, dst0,
>> 16*sizeof(int16_t), dst1, 16*sizeof(int16_t), 16, 16, "dst") would be
>> suitable one.
>
> Good catch, also I realized that the output buffers were too small,
> will be fixed in the next version.

Why was that too small? If we write (and check) 16x16 int16_t elements, 
the previous allocation of LOCAL_ALIGNED_16(int16_t, dst0, [16 * 16]) 
sounds just right? Or does the function use the [16*16,2*16*16) area of 
the destination as scratch space?

// Martin
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] checkasm: h264dsp: test luma_dc_dequant
  2025-06-13  6:08     ` [FFmpeg-devel] [PATCH] " Martin Storsjö
@ 2025-06-13 13:21       ` Tristan Matthews
  2025-06-13 13:26         ` Martin Storsjö
  0 siblings, 1 reply; 13+ messages in thread
From: Tristan Matthews @ 2025-06-13 13:21 UTC (permalink / raw)
  To: Martin Storsjö; +Cc: FFmpeg development discussions and patches

On Fri, Jun 13, 2025 at 2:08 AM Martin Storsjö <martin@martin.st> wrote:
>
> On Fri, 13 Jun 2025, Tristan Matthews wrote:
>
> > On Thu, Jun 12, 2025 at 4:14 PM Martin Storsjö <martin@martin.st> wrote:
> >>
> >> On Thu, 12 Jun 2025, Tristan Matthews wrote:
> >>
> >>> ---
> >>> tests/checkasm/h264dsp.c | 37 +++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 37 insertions(+)
> >>>
> >>> diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
> >>> index d1228ed985..5fba31cf69 100644
> >>> --- a/tests/checkasm/h264dsp.c
> >>> +++ b/tests/checkasm/h264dsp.c
> >>> @@ -22,6 +22,7 @@
> >>> #include "checkasm.h"
> >>> #include "libavcodec/h264dsp.h"
> >>> #include "libavcodec/h264data.h"
> >>> +#include "libavcodec/h264idct.h"
> >>> #include "libavcodec/h264_parse.h"
> >>> #include "libavutil/common.h"
> >>> #include "libavutil/intreadwrite.h"
> >>> @@ -324,6 +325,41 @@ static void check_idct_multiple(void)
> >>>     }
> >>> }
> >>>
> >>> +static void check_idct_dequant(void)
> >>> +{
> >>> +    static const int depths[5] = { 8, 9, 10, 12, 14 };
> >>> +    LOCAL_ALIGNED_16(int16_t, src, [16]);
> >>> +    LOCAL_ALIGNED_16(int16_t, dst0, [16 * 16]);
> >>> +    LOCAL_ALIGNED_16(int16_t, dst1, [16 * 16]);
> >>> +    H264DSPContext h;
> >>> +    int bit_depth, i, qmul;
> >>> +    declare_func_emms(AV_CPU_FLAG_MMX | AV_CPU_FLAG_SSE2, void, int16_t *output, int16_t *input, int qmul);
> >>> +
> >>> +    for (int j = 0; j < 16; j++)
> >>> +        src[j] = (rnd() % 512) - 256;
> >>> +
> >>> +    qmul = rnd() % 4096;
> >>> +
> >>> +    memset(dst0, 0, 16 * 16 * sizeof(dst0[0]));
> >>> +    memset(dst1, 0, 16 * 16 * sizeof(dst1[0]));
> >>> +
> >>> +    for (i = 0; i < FF_ARRAY_ELEMS(depths); i++) {
> >>> +        bit_depth = depths[i];
> >>> +        ff_h264dsp_init(&h, bit_depth, 1);
> >>> +
> >>> +        if (check_func(h.h264_luma_dc_dequant_idct, "h264_luma_dc_dequant_idct_%d", bit_depth)) {
> >>> +
> >>> +            call_ref(dst0, src, qmul);
> >>> +            call_new(dst1, src, qmul);
> >>> +
> >>> +            if (memcmp(dst0, dst1, 16 * 16 * sizeof(*dst0)))
> >>> +                fail();
> >>
> >> If possible, use the checkasm_check_*() helpers for validation for new
> >> code; this gives you printout of the differing values if you run "checkasm
> >> -v" and more. In this case, I think checkasm_check(int16_t, dst0,
> >> 16*sizeof(int16_t), dst1, 16*sizeof(int16_t), 16, 16, "dst") would be
> >> suitable one.
> >
> > Good catch, also I realized that the output buffers were too small,
> > will be fixed in the next version.
>
> Why was that too small? If we write (and check) 16x16 int16_t elements,
> the previous allocation of LOCAL_ALIGNED_16(int16_t, dst0, [16 * 16])
> sounds just right? Or does the function use the [16*16,2*16*16) area of
> the destination as scratch space?

That's what I thought too until I noticed the FATE failures (e.g.
https://patchwork.ffmpeg.org/check/124147/), and on further digging
realized that dctcoef (used for dst here:
https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/fb65ecbc9b805571e5ff707b935c343803137e54:/libavcodec/h264idct_template.c#l256
) will be either 2 or 4 bytes depending on bit-depth IIUC (see
https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/fb65ecbc9b805571e5ff707b935c343803137e54:/libavcodec/bit_depth_template.c#l54
)

Best,
Tristan
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] checkasm: h264dsp: test luma_dc_dequant
  2025-06-13 13:21       ` Tristan Matthews
@ 2025-06-13 13:26         ` Martin Storsjö
  2025-06-13 14:04           ` [FFmpeg-devel] [PATCH 1/2] checkasm: add checkasm_check_dctcoef Tristan Matthews
  0 siblings, 1 reply; 13+ messages in thread
From: Martin Storsjö @ 2025-06-13 13:26 UTC (permalink / raw)
  To: Tristan Matthews; +Cc: FFmpeg development discussions and patches

On Fri, 13 Jun 2025, Tristan Matthews wrote:

> On Fri, Jun 13, 2025 at 2:08 AM Martin Storsjö <martin@martin.st> wrote:
>>
>> On Fri, 13 Jun 2025, Tristan Matthews wrote:
>>
>>> Good catch, also I realized that the output buffers were too small,
>>> will be fixed in the next version.
>>
>> Why was that too small? If we write (and check) 16x16 int16_t elements,
>> the previous allocation of LOCAL_ALIGNED_16(int16_t, dst0, [16 * 16])
>> sounds just right? Or does the function use the [16*16,2*16*16) area of
>> the destination as scratch space?
>
> That's what I thought too until I noticed the FATE failures (e.g.
> https://patchwork.ffmpeg.org/check/124147/), and on further digging
> realized that dctcoef (used for dst here:
> https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/fb65ecbc9b805571e5ff707b935c343803137e54:/libavcodec/h264idct_template.c#l256
> ) will be either 2 or 4 bytes depending on bit-depth IIUC (see
> https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/fb65ecbc9b805571e5ff707b935c343803137e54:/libavcodec/bit_depth_template.c#l54
> )

Oh, I see. Well in that case, I think that using int16_t and *2 feels 
quite confusing; I think I'd rather have it be uint8 and *sizeof(int32_t) 
or something like that, to clarify what's going on.

I see that other preexisting tests, like vp9dsp.c, do use int16_t and an 
extra magical *2, but I think going plain uint8_t is clearer when it isn't 
always specifically int16_t.

In that case, using checkasm_check(int16_t) also is going to be wrong; we 
have similar cases for pixels, see the checkasm_check_pixel() macros in 
checkasm.h. Perhaps we need a similar checkasm_check_dctcoef() macro, 
which checks int16_t or int32_t depending on bit_depth?

// Martin
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [FFmpeg-devel] [PATCH 1/2] checkasm: add checkasm_check_dctcoef
  2025-06-13 13:26         ` Martin Storsjö
@ 2025-06-13 14:04           ` Tristan Matthews
  2025-06-13 14:04             ` [FFmpeg-devel] [PATCH 2/2] checkasm: h264dsp: test luma_dc_dequant Tristan Matthews
  0 siblings, 1 reply; 13+ messages in thread
From: Tristan Matthews @ 2025-06-13 14:04 UTC (permalink / raw)
  To: ffmpeg-devel; +Cc: Tristan Matthews

This is useful for tests that compare dctcoefs which will be either 2 bytes or
4 bytes, depending on bitdepth.
---
 tests/checkasm/checkasm.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tests/checkasm/checkasm.h b/tests/checkasm/checkasm.h
index 146bfdec35..e829942d58 100644
--- a/tests/checkasm/checkasm.h
+++ b/tests/checkasm/checkasm.h
@@ -443,5 +443,16 @@ DECL_CHECKASM_CHECK_FUNC(int32_t);
 #define checkasm_check_pixel_padded_align(...) \
     checkasm_check_pixel2(__VA_ARGS__, 8)
 
+/* This assumes that there is a local variable named "bit_depth".
+ * For tests that don't have that and only operate on a single
+ * bitdepth, just call checkasm_check(uint8_t, ...) directly. */
+#define checkasm_check_dctcoef(buf1, stride1, buf2, stride2, ...) \
+    ((bit_depth > 8) ?                                        \
+     checkasm_check(int32_t, (const int32_t*)buf1, stride1,   \
+                             (const int32_t*)buf2, stride2,   \
+                             __VA_ARGS__) :                   \
+     checkasm_check(int16_t, (const int16_t*)buf1, stride1,   \
+                             (const int16_t*)buf2, stride2,   \
+                             __VA_ARGS__))
 
 #endif /* TESTS_CHECKASM_CHECKASM_H */
-- 
2.48.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".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [FFmpeg-devel] [PATCH 2/2] checkasm: h264dsp: test luma_dc_dequant
  2025-06-13 14:04           ` [FFmpeg-devel] [PATCH 1/2] checkasm: add checkasm_check_dctcoef Tristan Matthews
@ 2025-06-13 14:04             ` Tristan Matthews
  2025-06-13 14:08               ` Martin Storsjö
  0 siblings, 1 reply; 13+ messages in thread
From: Tristan Matthews @ 2025-06-13 14:04 UTC (permalink / raw)
  To: ffmpeg-devel; +Cc: Tristan Matthews

---
 tests/checkasm/h264dsp.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
index d1228ed985..f5f9650224 100644
--- a/tests/checkasm/h264dsp.c
+++ b/tests/checkasm/h264dsp.c
@@ -22,6 +22,7 @@
 #include "checkasm.h"
 #include "libavcodec/h264dsp.h"
 #include "libavcodec/h264data.h"
+#include "libavcodec/h264idct.h"
 #include "libavcodec/h264_parse.h"
 #include "libavutil/common.h"
 #include "libavutil/intreadwrite.h"
@@ -324,6 +325,41 @@ static void check_idct_multiple(void)
     }
 }
 
+static void check_idct_dequant(void)
+{
+    static const int depths[5] = { 8, 9, 10, 12, 14 };
+    LOCAL_ALIGNED_16(int16_t, src, [16]);
+    /* Ensure dst buffers are large enough to hold dctcoefs of all bit-depths. */
+    LOCAL_ALIGNED_16(uint8_t, dst0, [16 * 16 * sizeof(int32_t)]);
+    LOCAL_ALIGNED_16(uint8_t, dst1, [16 * 16 * sizeof(int32_t)]);
+    int16_t *dst_ref = (int16_t *)dst0;
+    int16_t *dst_new = (int16_t *)dst1;
+    H264DSPContext h;
+    int bit_depth, i, qmul;
+    declare_func_emms(AV_CPU_FLAG_MMX | AV_CPU_FLAG_SSE2, void, int16_t *output, int16_t *input, int qmul);
+
+    for (int j = 0; j < 16; j++)
+        src[j] = (rnd() % 512) - 256;
+
+    qmul = rnd() % 4096;
+
+    for (i = 0; i < FF_ARRAY_ELEMS(depths); i++) {
+        bit_depth = depths[i];
+        ff_h264dsp_init(&h, bit_depth, 1);
+
+        memset(dst0, 0, 16 * 16 * SIZEOF_COEF);
+        memset(dst1, 0, 16 * 16 * SIZEOF_COEF);
+
+        if (check_func(h.h264_luma_dc_dequant_idct, "h264_luma_dc_dequant_idct_%d", bit_depth)) {
+
+            call_ref(dst_ref, src, qmul);
+            call_new(dst_new, src, qmul);
+            checkasm_check_dctcoef(dst_ref, 16*SIZEOF_COEF, dst_new, 16*SIZEOF_COEF, 16, 16, "dst");
+            bench_new(dst_new, src, qmul);
+        }
+    }
+}
+
 
 static void check_loop_filter(void)
 {
@@ -453,6 +489,7 @@ void checkasm_check_h264dsp(void)
 {
     check_idct();
     check_idct_multiple();
+    check_idct_dequant();
     report("idct");
 
     check_loop_filter();
-- 
2.48.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".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/2] checkasm: h264dsp: test luma_dc_dequant
  2025-06-13 14:04             ` [FFmpeg-devel] [PATCH 2/2] checkasm: h264dsp: test luma_dc_dequant Tristan Matthews
@ 2025-06-13 14:08               ` Martin Storsjö
  2025-06-15 23:39                 ` Michael Niedermayer
  0 siblings, 1 reply; 13+ messages in thread
From: Martin Storsjö @ 2025-06-13 14:08 UTC (permalink / raw)
  To: FFmpeg development discussions and patches; +Cc: Tristan Matthews

On Fri, 13 Jun 2025, Tristan Matthews wrote:

> ---
> tests/checkasm/h264dsp.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)

Both patches LGTM, thank you!

// Martin

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/2] checkasm: h264dsp: test luma_dc_dequant
  2025-06-13 14:08               ` Martin Storsjö
@ 2025-06-15 23:39                 ` Michael Niedermayer
  2025-06-16  6:53                   ` Zhao Zhili
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Niedermayer @ 2025-06-15 23:39 UTC (permalink / raw)
  To: FFmpeg development discussions and patches


[-- Attachment #1.1: Type: text/plain, Size: 507 bytes --]

On Fri, Jun 13, 2025 at 05:08:30PM +0300, Martin Storsjö wrote:
> On Fri, 13 Jun 2025, Tristan Matthews wrote:
> 
> > ---
> > tests/checkasm/h264dsp.c | 37 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 37 insertions(+)
> 
> Both patches LGTM, thank you!

will apply

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/2] checkasm: h264dsp: test luma_dc_dequant
  2025-06-15 23:39                 ` Michael Niedermayer
@ 2025-06-16  6:53                   ` Zhao Zhili
  2025-06-16 13:58                     ` Michael Niedermayer
  0 siblings, 1 reply; 13+ messages in thread
From: Zhao Zhili @ 2025-06-16  6:53 UTC (permalink / raw)
  To: FFmpeg development discussions and patches



> On Jun 16, 2025, at 07:39, Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> On Fri, Jun 13, 2025 at 05:08:30PM +0300, Martin Storsjö wrote:
>> On Fri, 13 Jun 2025, Tristan Matthews wrote:
>> 
>>> ---
>>> tests/checkasm/h264dsp.c | 37 +++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 37 insertions(+)
>> 
>> Both patches LGTM, thank you!
> 
> will apply

This has a stack overflow.

https://ffmpeg.org/pipermail/ffmpeg-devel/2025-June/345305.html

> 
> thx
> 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Good people do not need laws to tell them to act responsibly, while bad
> people will find a way around the laws. -- Plato
> 
> _______________________________________________
> 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".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/2] checkasm: h264dsp: test luma_dc_dequant
  2025-06-16  6:53                   ` Zhao Zhili
@ 2025-06-16 13:58                     ` Michael Niedermayer
  0 siblings, 0 replies; 13+ messages in thread
From: Michael Niedermayer @ 2025-06-16 13:58 UTC (permalink / raw)
  To: FFmpeg development discussions and patches


[-- Attachment #1.1: Type: text/plain, Size: 953 bytes --]

Hi Zhao

On Mon, Jun 16, 2025 at 02:53:59PM +0800, Zhao Zhili wrote:
> 
> 
> > On Jun 16, 2025, at 07:39, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > 
> > On Fri, Jun 13, 2025 at 05:08:30PM +0300, Martin Storsjö wrote:
> >> On Fri, 13 Jun 2025, Tristan Matthews wrote:
> >> 
> >>> ---
> >>> tests/checkasm/h264dsp.c | 37 +++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 37 insertions(+)
> >> 
> >> Both patches LGTM, thank you!
> > 
> > will apply
> 
> This has a stack overflow.

I think this should have been a reply to the author or reviewer

i just pushed it after it was reviewed, I did not look at the code at all

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 2
"100% positive feedback" - "All either got their money back or didnt complain"
"Best seller ever, very honest" - "Seller refunded buyer after failed scam"

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2025-06-16 13:59 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-06-12 17:25 [FFmpeg-devel] [PATCH] checkasm: h264dsp: test luma_dc_dequant Tristan Matthews
2025-06-12 20:13 ` Martin Storsjö
2025-06-13  4:11   ` Tristan Matthews
2025-06-13  4:12     ` [FFmpeg-devel] [PATCH 1/1] " Tristan Matthews
2025-06-13  6:08     ` [FFmpeg-devel] [PATCH] " Martin Storsjö
2025-06-13 13:21       ` Tristan Matthews
2025-06-13 13:26         ` Martin Storsjö
2025-06-13 14:04           ` [FFmpeg-devel] [PATCH 1/2] checkasm: add checkasm_check_dctcoef Tristan Matthews
2025-06-13 14:04             ` [FFmpeg-devel] [PATCH 2/2] checkasm: h264dsp: test luma_dc_dequant Tristan Matthews
2025-06-13 14:08               ` Martin Storsjö
2025-06-15 23:39                 ` Michael Niedermayer
2025-06-16  6:53                   ` Zhao Zhili
2025-06-16 13:58                     ` Michael Niedermayer

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git