[FFmpeg-devel] [PATCH 1/2] checkasm: sw_scale: Fix the difference printing for approximate functions

[FFmpeg-devel] [PATCH 1/2] checkasm: sw_scale: Fix the difference printing for approximate functions

[Martin Storsjö]

Don't stop directly at the first differing pixel, but find the
one that differs by more than the expected accuracy.

Also print the failing value in check_yuv2yuvX.

Signed-off-by: Martin Storsjö <martin@martin.st>
---
 tests/checkasm/sw_scale.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tests/checkasm/sw_scale.c b/tests/checkasm/sw_scale.c
index cbe4460a99..d72506ed86 100644
--- a/tests/checkasm/sw_scale.c
+++ b/tests/checkasm/sw_scale.c
@@ -77,10 +77,10 @@ static void print_data(uint8_t *p, size_t len, size_t offset)
     }
 }
 
-static size_t show_differences(uint8_t *a, uint8_t *b, size_t len)
+static size_t show_differences(uint8_t *a, uint8_t *b, size_t len, int accuracy)
 {
     for (size_t i = 0; i < len; i++) {
-        if (a[i] != b[i]) {
+        if (abs(a[i] - b[i]) > accuracy) {
             size_t offset_of_mismatch = i;
             size_t offset;
             if (i >= 8) i-=8;
@@ -141,7 +141,7 @@ static void check_yuv2yuv1(int accurate)
                 if (cmp_off_by_n(dst0, dst1, dstW * sizeof(dst0[0]), accurate ? 0 : 2)) {
                     fail();
                     printf("failed: yuv2yuv1_%d_%di_%s\n", offset, dstW, accurate_str);
-                    fail_offset = show_differences(dst0, dst1, LARGEST_INPUT_SIZE * sizeof(dst0[0]));
+                    fail_offset = show_differences(dst0, dst1, LARGEST_INPUT_SIZE * sizeof(dst0[0]), accurate ? 0 : 2);
                     printf("failing values: src: 0x%04x dither: 0x%02x dst-c: %02x dst-asm: %02x\n",
                             (int) src_pixels[fail_offset],
                             (int) dither[(fail_offset + fail_offset) & 7],
@@ -169,6 +169,7 @@ static void check_yuv2yuvX(int accurate)
     static const int input_sizes[] = {8, 24, 128, 144, 256, 512};
     const int INPUT_SIZES = sizeof(input_sizes)/sizeof(input_sizes[0]);
     const char *accurate_str = (accurate) ? "accurate" : "approximate";
+    size_t fail_offset;
 
     declare_func_emms(AV_CPU_FLAG_MMX, void, const int16_t *filter,
                       int filterSize, const int16_t **src, uint8_t *dest,
@@ -224,7 +225,12 @@ static void check_yuv2yuvX(int accurate)
                     if (cmp_off_by_n(dst0, dst1, LARGEST_INPUT_SIZE * sizeof(dst0[0]), accurate ? 0 : 2)) {
                         fail();
                         printf("failed: yuv2yuvX_%d_%d_%d_%s\n", filter_sizes[fsi], osi, dstW, accurate_str);
-                        show_differences(dst0, dst1, LARGEST_INPUT_SIZE * sizeof(dst0[0]));
+                        fail_offset = show_differences(dst0, dst1, LARGEST_INPUT_SIZE * sizeof(dst0[0]), accurate ? 0 : 2);
+                        printf("failing values: src: 0x%04x dither: 0x%02x dst-c: %02x dst-asm: %02x\n",
+                                (int) src_pixels[fail_offset],
+                                (int) dither[(fail_offset + osi) & 7],
+                                (int) dst0[fail_offset],
+                                (int) dst1[fail_offset]);
                     }
                     if(dstW == LARGEST_INPUT_SIZE)
                         bench_new((const int16_t*)vFilterData, filter_sizes[fsi], src, dst1, dstW - osi, dither, osi);
-- 
2.25.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: sw_scale: Reduce range of test data in the yuv2yuvX test to get closer to real data

[Martin Storsjö]

This avoids overflows on some inputs in the x86 case, where the
assembly version would clip/overflow differently from the
C reference function.

This doesn't seem to be a real issue with actual input data, but
only with the previous fully random input data.

This also makes the test produce a bit more realistic output pixel
values, instead of having essentially all pixels clipped to either
0 or 255.

Signed-off-by: Martin Storsjö <martin@martin.st>
---
 tests/checkasm/sw_scale.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/checkasm/sw_scale.c b/tests/checkasm/sw_scale.c
index d72506ed86..89403da317 100644
--- a/tests/checkasm/sw_scale.c
+++ b/tests/checkasm/sw_scale.c
@@ -187,8 +187,16 @@ static void check_yuv2yuvX(int accurate)
     } *vFilterData;
     uint8_t d_val = rnd();
     memset(dither, d_val, LARGEST_INPUT_SIZE);
+
     randomize_buffers((uint8_t*)src_pixels, LARGEST_FILTER * LARGEST_INPUT_SIZE * sizeof(int16_t));
     randomize_buffers((uint8_t*)filter_coeff, LARGEST_FILTER * sizeof(int16_t));
+    // Limit the range of the filter coefficients and intermediate
+    // pixel values, to avoid risk of clipping filter intermediates on x86.
+    for (i = 0; i < LARGEST_FILTER; i++)
+        filter_coeff[i] >>= 2;
+    for (i = 0; i < LARGEST_FILTER * LARGEST_INPUT_SIZE; i++)
+        src_pixels[i] >>= 2;
+
     ctx = sws_alloc_context();
     if (accurate)
         ctx->flags |= SWS_ACCURATE_RND;
-- 
2.25.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: sw_scale: Reduce range of test data in the yuv2yuvX test to get closer to real data

[Ronald S. Bultje]

Hi,

On Wed, Aug 17, 2022 at 4:32 PM Martin Storsjö <martin@martin.st> wrote:

> This avoids overflows on some inputs in the x86 case, where the
> assembly version would clip/overflow differently from the
> C reference function.
>
> This doesn't seem to be a real issue with actual input data, but
> only with the previous fully random input data.
>

I'm a bit scared of this change... If we can trigger overflows with
specific pixel patterns, doesn't that make FFmpeg input-data exploitable?
Think of how that would go with corporate users with user-provided input
data.

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 2/2] checkasm: sw_scale: Reduce range of test data in the yuv2yuvX test to get closer to real data

[Martin Storsjö]

On Wed, 17 Aug 2022, Ronald S. Bultje wrote:

> On Wed, Aug 17, 2022 at 4:32 PM Martin Storsjö <martin@martin.st> wrote:
>       This avoids overflows on some inputs in the x86 case, where the
>       assembly version would clip/overflow differently from the
>       C reference function.
>
>       This doesn't seem to be a real issue with actual input data, but
>       only with the previous fully random input data.
> 
> 
> I'm a bit scared of this change... If we can trigger overflows with specific
> pixel patterns, doesn't that make FFmpeg input-data exploitable? Think of
> how that would go with corporate users with user-provided input data.

No, this most probably isn't a real issue with actual filters - it's only 
that the current checkasm test was overly simplistic.

The input to this DSP function isn't raw user-provided input pixels, but 
16 bit integers produced as the output of the first (horizontal) scaling 
step. Yesterday when I wrote the patch, I hadn't checked exactly what the 
range of those values were, and I assumed it wasn't the whole int16_t 
range - but apparently they can range at least up to max int16_t. (They 
most probably can't range down to the minimum negative int16_t though - 
simulating that aspect would be nice too.)

The filter coefficients should add up to 4096. The input sample range is 
15 bits (plus sign), and filter coefficients add a total magnitude of 12 
bits, giving a total range of 27 bits (plus sign). After shifting down by 
19 bits at the end, this produces 8 bits output (which is clipped).

The critical stage here is the 27 bits, where there's still plenty of 
headroom (4 bits) for filter overshoot - with a real filter.

However in the current test, the filter coefficients are just plain random 
int16_t values in the whole range - and that can easily cause overflows in 
the intermediates.

So I guess we shouldn't scale down the input "pixels" here as they 
actually can use the whole range up to max int16_t (but ideally, they 
wouldn't range further down below zero than what you'd get from the 
maximal negative filter overshoot either), but we should scale down the 
fully random filter coefficients, so that they can't overflow even if they 
all happen to align in the worst way.

Alternatively we could construct a more realistic test filter, e.g. 
something like what's used in the hscale test. There, if the filter should 
add up to 1<<F, and we have N filter coefficients, we have all of them but 
one be set to -((1<<F)/(N-1)) and one set to ((1<<(F+1)) - 1). It doesn't 
look much like a real actual filter, but keeps most properties - it adds 
up to the right sum and doesn't trigger unreal overflows and it produces 
both positive and negative values.

Anyway, I'll update the patch and make a clearer comment for it.

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