[FFmpeg-devel] [PATCH v2] checkasm: add hscale test

[Martin Storsjö]

On Wed, 13 May 2020, Josh de Kock wrote:

> This tests the hscale 8bpp to 14bpp functions with different filter
> sizes.
>
> Signed-off-by: Josh de Kock <josh at itanimul.li>
> ---
>
> Should address all previous comments.
>
> tests/checkasm/Makefile   |   2 +-
> tests/checkasm/checkasm.c |   1 +
> tests/checkasm/checkasm.h |   1 +
> tests/checkasm/sw_scale.c | 114 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 117 insertions(+), 1 deletion(-)
> create mode 100644 tests/checkasm/sw_scale.c
>
> diff --git a/tests/checkasm/Makefile b/tests/checkasm/Makefile
> index de850c016e..9e9569777b 100644
> --- a/tests/checkasm/Makefile
> +++ b/tests/checkasm/Makefile
> @@ -45,7 +45,7 @@ AVFILTEROBJS-$(CONFIG_NLMEANS_FILTER)    += vf_nlmeans.o
> CHECKASMOBJS-$(CONFIG_AVFILTER) += $(AVFILTEROBJS-yes)
> 
> # swscale tests
> -SWSCALEOBJS                             += sw_rgb.o
> +SWSCALEOBJS                             += sw_rgb.o sw_scale.o
> 
> CHECKASMOBJS-$(CONFIG_SWSCALE)  += $(SWSCALEOBJS)
> 
> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
> index d67147ae6f..91066514f6 100644
> --- a/tests/checkasm/checkasm.c
> +++ b/tests/checkasm/checkasm.c
> @@ -183,6 +183,7 @@ static const struct {
> #endif
> #if CONFIG_SWSCALE
>     { "sw_rgb", checkasm_check_sw_rgb },
> +    { "sw_scale", checkasm_check_sw_scale },
> #endif
> #if CONFIG_AVUTIL
>         { "fixed_dsp", checkasm_check_fixed_dsp },
> diff --git a/tests/checkasm/checkasm.h b/tests/checkasm/checkasm.h
> index 0a7f9f25c4..77e573774f 100644
> --- a/tests/checkasm/checkasm.h
> +++ b/tests/checkasm/checkasm.h
> @@ -69,6 +69,7 @@ void checkasm_check_pixblockdsp(void);
> void checkasm_check_sbrdsp(void);
> void checkasm_check_synth_filter(void);
> void checkasm_check_sw_rgb(void);
> +void checkasm_check_sw_scale(void);
> void checkasm_check_utvideodsp(void);
> void checkasm_check_v210dec(void);
> void checkasm_check_v210enc(void);
> diff --git a/tests/checkasm/sw_scale.c b/tests/checkasm/sw_scale.c
> new file mode 100644
> index 0000000000..52a15247d5
> --- /dev/null
> +++ b/tests/checkasm/sw_scale.c
> @@ -0,0 +1,114 @@
> +/*
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with FFmpeg; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <string.h>
> +
> +#include "libavutil/common.h"
> +#include "libavutil/intreadwrite.h"
> +#include "libavutil/mem.h"
> +
> +#include "libswscale/swscale.h"
> +#include "libswscale/swscale_internal.h"
> +
> +#include "checkasm.h"
> +
> +#define randomize_buffers(buf, size)      \
> +    do {                                  \
> +        int j;                            \
> +        for (j = 0; j < size; j+=4)       \
> +            AV_WN32(buf + j, rnd());      \
> +    } while (0)
> +
> +#define MAX_WIDTH 128
> +
> +#define FILTER_SIZES 5
> +int filter_sizes[FILTER_SIZES] = { 4, 8, 16, 32, 40 };
> +#define MAX_FILTER_WIDTH 40
> +
> +#define HSCALE_PAIRS 2
> +int hscale_pairs[HSCALE_PAIRS][2] = {
> +    { 8, 14 },
> +    { 8, 18 },
> +};

Make filter_sizes and hscale_pairs static const (they could even be moved 
into the function below I presume?)

> +
> +static void check_hscale(void)
> +{
> +    int i, j, fsi, hpi, width;
> +    struct SwsContext *ctx;
> +
> +    // padded
> +    LOCAL_ALIGNED_32(uint8_t, src, [MAX_WIDTH + MAX_FILTER_WIDTH - 1]);
> +    LOCAL_ALIGNED_32(uint32_t, dst0, [MAX_WIDTH]);
> +    LOCAL_ALIGNED_32(uint32_t, dst1, [MAX_WIDTH]);
> +
> +    // padded
> +    LOCAL_ALIGNED_32(int16_t, filter, [MAX_WIDTH * MAX_FILTER_WIDTH + MAX_FILTER_WIDTH]);
> +    LOCAL_ALIGNED_32(int32_t, filterPos, [MAX_WIDTH]);
> +
> +    // The dst parameter here is either int16_t or int32_t but we use void* to
> +    // just cover both cases.
> +    declare_func(void, void *c, void *dst, int dstW,
> +                 const uint8_t *src, const int16_t *filter,
> +                 const int32_t *filterPos, int filterSize);

Right, that's pretty tricky - probably works fine, but would be a bit of a 
pain later if one wants to debug cases (e.g. when developing new asm), 
wanting to print the actual produced values to figure out what happened. 
But I guess it's ok for keeping the code simple here.

> +
> +    ctx = sws_alloc_context();
> +    if (sws_init_context(ctx, NULL, NULL) < 0)
> +        fail();
> +
> +    randomize_buffers(src, MAX_WIDTH + MAX_FILTER_WIDTH - 1);
> +
> +    for (hpi = 0; hpi < HSCALE_PAIRS; hpi++) {
> +        for (fsi = 0; fsi < FILTER_SIZES; fsi++) {
> +            width = filter_sizes[fsi];
> +
> +            ctx->srcBpc = hscale_pairs[hpi][0];
> +            ctx->dstBpc = hscale_pairs[hpi][1];
> +            ctx->hLumFilterSize = ctx->hChrFilterSize = width;
> +            for (i = 0; i < MAX_WIDTH; i++) {
> +                filterPos[i] = i;
> +                for (j = 0; j < width; j++) {
> +                    filter[i * width + j] = (1 << 14) / width;

I would still like a less uniform test filter, where all the filter 
coefficients are different. One could have an implementation that 
mismatches filter coefficients to source pixels and still get the right 
result with this test.

There's also more corner cases that it would be ideal if the test 
exercised:
- Negative filter coefficients. The filters output signed values, and it
   should be possible to end up with negative output values.
- Positive clipping. The filter functions have clipping at (1<<15)-1 and
   (1<<19)-1.

(You can try to remove the FFMIN from the C implementation and try to 
tweak filter coefficients to see if you can make it mismatch the assembly 
versions - then you know you have inputs that actually require the 
clipping).

One simple thing you could try would e.g. be filling all the filter 
coefficients with -(1<<14)/(width-1), and setting one of them (e.g. 
randomly chosen from 0 to width-1) to (1<<15)-1. That should add up to 
(1<<14), while still having a lot of possibility for getting both negative 
values and overshoot.

> +                }
> +            }
> +
> +            for (i = 0; i < MAX_FILTER_WIDTH; i++) {
> +                filter[MAX_WIDTH * width + i] = (1 << 14) / width;
> +            }

Isn't this just padding elements, because some simd implementations 
overread the filter array? If that's true, they _shouldn't_ affect the 
output - so then could be whatever you want - including leaving them 
uninitialized.

If they are uninitialized, it should be fine to read them and do some 
calculation on them, as long as they don't contribute to the actual output 
bytes (that are checked with memcmp below) - right? If valgrind supports 
the instruction sets fully, it should be able to handle this case just 
fine and not report anything about it. But as it's on the stack it might 
be treated as initialized anyway.

I would at least want them to be notably different from filter 
coefficients actually intentionally used for the output.


// Martin