[FFmpeg-devel] [WIP PATCH 2/2] checkasm: add hscale test

[Martin Storsjö]

On Thu, 7 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>
> ---
>
> Not quite ready to be committed but I wanted to submit it anyway so I
> could get some comments. I still need to do the rest of the scale sizes
> (such as 8bpp to 19bpp) and make the benchmarks work.

Not a proper review, but some comments just from reading it

> diff --git a/tests/checkasm/Makefile b/tests/checkasm/Makefile
> index de850c016e..c04c2a304e 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_hscale.o sw_rgb.o

Nitpick: The object file is called sw_hscale here, but the functions just 
sw_scale - can it be made consistent?

> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
> index d67147ae6f..8bf32d5b65 100644
> --- a/tests/checkasm/checkasm.c
> +++ b/tests/checkasm/checkasm.c
> @@ -182,6 +182,7 @@ static const struct {
>     #endif
> #endif
> #if CONFIG_SWSCALE
> +    { "sw_scale", checkasm_check_sw_scale },
>     { "sw_rgb", checkasm_check_sw_rgb },

Alphabetical order?

> #endif
> #if CONFIG_AVUTIL
> diff --git a/tests/checkasm/checkasm.h b/tests/checkasm/checkasm.h
> index 0a7f9f25c4..98d59aedf7 100644
> --- a/tests/checkasm/checkasm.h
> +++ b/tests/checkasm/checkasm.h
> @@ -68,6 +68,7 @@ void checkasm_check_opusdsp(void);
> void checkasm_check_pixblockdsp(void);
> void checkasm_check_sbrdsp(void);
> void checkasm_check_synth_filter(void);
> +void checkasm_check_sw_scale(void);
> void checkasm_check_sw_rgb(void);

Nit: These seem to be in alphabetical order here

> void checkasm_check_utvideodsp(void);
> void checkasm_check_v210dec(void);
> diff --git a/tests/checkasm/sw_hscale.c b/tests/checkasm/sw_hscale.c
> new file mode 100644
> index 0000000000..f54ed72d81
> --- /dev/null
> +++ b/tests/checkasm/sw_hscale.c
> @@ -0,0 +1,108 @@
> +/*
> + *
> + * 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)       \

Nit: Space around operators at the end

> +            AV_WN32(buf + j, rnd());      \
> +    } while (0)
> +
> +#define MAX_WIDTH 128
> +#define MAX_FILTER_WIDTH 40
> +
> +#define INIT_FILTER(bsrc, bdst, width) \
> +    ctx->srcBpc = bsrc; \
> +    ctx->dstBpc = bdst; \
> +    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; \

Would be great to test with a filter that doesn't have the same 
coefficient everywhere - this can easily let many implementation bugs slip 
through

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

Isn't this last bit here just padding for simd implementations that might 
overread? In that case it shouldn't matter what we set here - and 
certainly not seemingly valid filter coefficients?

> +    ff_getSwsFunc(ctx);
> +
> +#define CHECK_FILTER(width) \
> +    INIT_FILTER(8, 14, width) \
> +    if (check_func(ctx->hcScale, "hscale_8_to_15_width" #width)) { \

Can't this just be made into a proper loop instead of a macro that is 
expanded multiple times? check_func takes a format string, so you can make 
it "..._width%d", width). Just add an array of the widths to check and 
iterate over that.

> +        memset(dst0, 0, MAX_WIDTH * sizeof(dst0[0])); \
> +        memset(dst1, 0, MAX_WIDTH * sizeof(dst1[0])); \
> +        \
> +        call_ref(NULL, dst0, MAX_WIDTH, src, filter, filterPos, width); \
> +        call_new(NULL, dst1, MAX_WIDTH, src, filter, filterPos, width); \

Shouldn't you pass the sws context as the first argument here? In case an 
implementation would store anything of value there (otherwise there's no 
point in the function taking this parameter in the first place).

> +        if (memcmp(dst0, dst1, MAX_WIDTH)) \
> +            fail(); \
> +        /*bench_new();*/ \

IIRC benching shouldn't require much else than calling bench_new with the 
same arguments as call_new above.

> +    }
> +
> +static void check_hscale(void)
> +{
> +    int i, j;
> +    struct SwsContext *ctx;
> +
> +    // padded
> +    LOCAL_ALIGNED_32(uint8_t, src, [MAX_WIDTH + MAX_FILTER_WIDTH - 1]);
> +    LOCAL_ALIGNED_32(uint16_t, dst0, [MAX_WIDTH]);
> +    LOCAL_ALIGNED_32(uint16_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]);
> +
> +    declare_func(void, void*c, int16_t *dst, int dstW,

Nit: Space around * in "void*c"

> +                 const uint8_t *src, const int16_t *filter,
> +                 const int32_t *filterPos, int filterSize);
> +
> +    ctx = sws_alloc_context();
> +    if (sws_init_context(ctx, NULL, NULL) < 0)
> +        fail();
> +
> +    memset(src, 0, MAX_WIDTH + MAX_FILTER_WIDTH - 1);
> +    randomize_buffers(src, MAX_WIDTH);

Wouldn't it be better to have the trailing entries in the src buffer to 
also be initialized to random values instead of nice zeros (that can hide 
inconsistencies)? In this case we intentionally do read and include them 
in the filtering - they aren't just overread padding.

// Martin