Re: [FFmpeg-devel] [PATCH v4 1/2] checkasm: add hevc_deblock chroma test

["Martin Storsjö" <martin@martin.st>]

On Wed, 29 Mar 2023, J. Dekker wrote:

> Signed-off-by: J. Dekker <jdek@itanimul.li>
> ---
>
> Will support other variants in the second version of these tests.
>
> tests/checkasm/Makefile       |   2 +-
> tests/checkasm/checkasm.c     |   1 +
> tests/checkasm/checkasm.h     |   1 +
> tests/checkasm/hevc_deblock.c | 100 ++++++++++++++++++++++++++++++++++
> tests/fate/checkasm.mak       |   1 +
> 5 files changed, 104 insertions(+), 1 deletion(-)
> create mode 100644 tests/checkasm/hevc_deblock.c
>
> diff --git a/tests/checkasm/Makefile b/tests/checkasm/Makefile
> index a6f06c7007..3e62a22bd6 100644
> --- a/tests/checkasm/Makefile
> +++ b/tests/checkasm/Makefile
> @@ -28,7 +28,7 @@ AVCODECOBJS-$(CONFIG_HUFFYUV_DECODER)   += huffyuvdsp.o
> AVCODECOBJS-$(CONFIG_JPEG2000_DECODER)  += jpeg2000dsp.o
> AVCODECOBJS-$(CONFIG_OPUS_DECODER)      += opusdsp.o
> AVCODECOBJS-$(CONFIG_PIXBLOCKDSP)       += pixblockdsp.o
> -AVCODECOBJS-$(CONFIG_HEVC_DECODER)      += hevc_add_res.o hevc_idct.o hevc_sao.o hevc_pel.o
> +AVCODECOBJS-$(CONFIG_HEVC_DECODER)      += hevc_add_res.o hevc_deblock.o hevc_idct.o hevc_sao.o hevc_pel.o
> AVCODECOBJS-$(CONFIG_UTVIDEO_DECODER)   += utvideodsp.o
> AVCODECOBJS-$(CONFIG_V210_DECODER)      += v210dec.o
> AVCODECOBJS-$(CONFIG_V210_ENCODER)      += v210enc.o
> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
> index e96d84a7da..c2184d260d 100644
> --- a/tests/checkasm/checkasm.c
> +++ b/tests/checkasm/checkasm.c
> @@ -116,6 +116,7 @@ static const struct {
>     #endif
>     #if CONFIG_HEVC_DECODER
>         { "hevc_add_res", checkasm_check_hevc_add_res },
> +        { "hevc_deblock", checkasm_check_hevc_deblock },
>         { "hevc_idct", checkasm_check_hevc_idct },
>         { "hevc_pel", checkasm_check_hevc_pel },
>         { "hevc_sao", checkasm_check_hevc_sao },
> diff --git a/tests/checkasm/checkasm.h b/tests/checkasm/checkasm.h
> index 8744a81218..89c643e6a0 100644
> --- a/tests/checkasm/checkasm.h
> +++ b/tests/checkasm/checkasm.h
> @@ -60,6 +60,7 @@ void checkasm_check_h264dsp(void);
> void checkasm_check_h264pred(void);
> void checkasm_check_h264qpel(void);
> void checkasm_check_hevc_add_res(void);
> +void checkasm_check_hevc_deblock(void);
> void checkasm_check_hevc_idct(void);
> void checkasm_check_hevc_pel(void);
> void checkasm_check_hevc_sao(void);
> diff --git a/tests/checkasm/hevc_deblock.c b/tests/checkasm/hevc_deblock.c
> new file mode 100644
> index 0000000000..f73e68e8a6
> --- /dev/null
> +++ b/tests/checkasm/hevc_deblock.c
> @@ -0,0 +1,100 @@
> +/*
> + * 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/intreadwrite.h"
> +#include "libavutil/mem_internal.h"
> +
> +#include "libavcodec/avcodec.h"
> +#include "libavcodec/hevcdsp.h"
> +
> +#include "checkasm.h"
> +
> +static const uint32_t pixel_mask[3] = { 0xffffffff, 0x03ff03ff, 0x0fff0fff };
> +
> +#define SIZEOF_PIXEL ((bit_depth + 7) / 8)
> +#define BUF_STRIDE (8 * 2)
> +#define BUF_LINES (8)
> +#define BUF_OFFSET (BUF_STRIDE * BUF_LINES)
> +#define BUF_SIZE (BUF_STRIDE * BUF_LINES + BUF_OFFSET * 2)
> +
> +#define randomize_buffers(buf0, buf1, size)                 \
> +    do {                                                    \
> +        uint32_t mask = pixel_mask[(bit_depth - 8) >> 1];   \
> +        int k;                                              \
> +        for (k = 0; k < size; k += 4) {                     \
> +            uint32_t r = rnd() & mask;                      \
> +            AV_WN32A(buf0 + k, r);                          \
> +            AV_WN32A(buf1 + k, r);                          \
> +        }                                                   \
> +    } while (0)
> +
> +static void check_deblock_chroma(HEVCDSPContext h, int bit_depth)
> +{
> +    int32_t tc[2] = { 0, 0 };
> +    // no_p, no_q can only be { 0,0 } for assembly functions, see deblocking_filter_CTB() in hevc_filter.c

It's not strictly about assembly functions; there's just two separate 
function pointers, hevc_v_loop_filter_chroma and 
hevc_v_loop_filter_chroma_c - you can implement both in assembly. I guess 
the intent of the _c variant originally was that it wasn't the assembly 
version for whatever assembly implementation there was at the time, but we 
can support both in assembly too.

(As a later step, we can probably template this test somehow to test both 
of them.)

> +    uint8_t no_p[2] = { 0, 0 };
> +    uint8_t no_q[2] = { 0, 0 };
> +    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);
> +    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);
> +
> +    declare_func_emms(AV_CPU_FLAG_MMX, void, uint8_t *pix, ptrdiff_t stride, int32_t *tc, uint8_t *no_p, uint8_t *no_q);
> +
> +    if (check_func(h.hevc_h_loop_filter_chroma, "hevc_h_loop_filter_chroma%d", bit_depth)) {
> +        randomize_buffers(buf0, buf1, BUF_SIZE);
> +        for (int i = 0; i < 4; i++) {
> +            // see betatable[] in hevc_filter.c
> +            tc[0] = (rnd() & 63) + (rnd() & 1);
> +            tc[1] = (rnd() & 63) + (rnd() & 1);
> +
> +            call_ref(buf0 + BUF_OFFSET, BUF_STRIDE, tc, no_p, no_q);
> +            call_new(buf1 + BUF_OFFSET, BUF_STRIDE, tc, no_p, no_q);
> +            if (memcmp(buf0, buf1, BUF_SIZE))
> +                fail();
> +        }
> +        bench_new(buf1 + BUF_OFFSET, BUF_STRIDE, tc, no_p, no_q);
> +    }
> +
> +    if (check_func(h.hevc_v_loop_filter_chroma, "hevc_v_loop_filter_chroma%d", bit_depth)) {
> +        randomize_buffers(buf0, buf1, BUF_SIZE);
> +        for (int i = 0; i < 4; i++) {
> +            // see betatable[] in hevc_filter.c
> +            tc[0] = (rnd() & 63) + (rnd() & 1);
> +            tc[1] = (rnd() & 63) + (rnd() & 1);

I wonder if it'd be better to test with a freshly randomized buffer 
instead of iterating over the same data multiple times? Worst case, the 
deblocked area converges to so little differences that the last few 
iterations don't make any difference.

I.e., I'd suggest moving the randomize_buffers call into the loop here.


Other than that, this patch LGTM.

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