| Message ID | 20230226060909.16477-1-nuomi2021@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] checkasm: add support for vvc alf | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Sun, 26 Feb 2023, Nuo Mi wrote: > +#include <string.h> > + > +#include "libavutil/intreadwrite.h" > +#include "libavutil/mem_internal.h" > + > +#include "libavcodec/avcodec.h" > + > +#include "libavcodec/vvcdsp.h" > +#include "libavcodec/vvcdec.h" > + > +#include "checkasm.h" > + > +static const uint32_t pixel_mask[3] = { 0xffffffff, 0x03ff03ff, 0x0fff0fff }; > + > +#define SIZEOF_PIXEL ((bit_depth + 7) / 8) > +#define PIXEL_STRIDE (ALF_SUBBLOCK_SIZE + 2 * ALF_PADDING_SIZE) > +#define BUF_SIZE (PIXEL_STRIDE * (MAX_CTU_SIZE + 3 * 2) * 2) //+3 * 2 for top and bottom row, *2 for high bit depth > +#define LUMA_PARAMS_SIZE (ALF_SUBBLOCK_SIZE / 4 * ALF_SUBBLOCK_SIZE / 4 * ALF_NUM_COEFF_LUMA) > + > +#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) > + > +#define randomize_buffers2(buf, size, filter) \ > + do { \ > + int k; \ > + if (filter) { \ > + for (k = 0; k < size; k++) { \ > + uint8_t r = rnd(); \ > + buf[k] = r; \ > + } \ > + } else { \ > + for (k = 0; k < size; k++) { \ > + int16_t r = rnd(); \ > + buf[k] = r; \ > + } \ > + } \ > + } while (0) I don't quite see the point of the extra uint8_t/int16_t variable r here - you could just as well assign it directly to buf[k], no? Unless you specifically want the effect where you're narrowing the random value to a smaller range beforehand. > + > +static void check_alf_luma_filter(VVCDSPContext *c, const int bit_depth) > +{ > + LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]); > + LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]); > + LOCAL_ALIGNED_32(uint8_t, src0, [BUF_SIZE]); > + LOCAL_ALIGNED_32(uint8_t, src1, [BUF_SIZE]); > + int8_t filter[LUMA_PARAMS_SIZE]; > + int16_t clip[LUMA_PARAMS_SIZE]; > + ptrdiff_t stride = PIXEL_STRIDE * SIZEOF_PIXEL; > + int offset = (3 * PIXEL_STRIDE + 3) * SIZEOF_PIXEL; > + > + declare_func_emms(AV_CPU_FLAG_AVX2, void, uint8_t *dst, ptrdiff_t dst_stride, const uint8_t *src, ptrdiff_t src_stride, > + int width, int height, const int8_t *filter, const int16_t *clip); > + > + randomize_buffers(src0, src1, BUF_SIZE); > + randomize_buffers2(filter, LUMA_PARAMS_SIZE, 1); > + randomize_buffers2(clip, LUMA_PARAMS_SIZE, 2); Here, both invocations of randomize_buffers2 are called with filter=1 or 2, both which will pick the "if (filter) {" case, so the else in randomize_buffers2 is unused? > + > + for (int h = 4; h < ALF_SUBBLOCK_SIZE; h += 4) { > + for (int w = 4; w < ALF_SUBBLOCK_SIZE; w += 4) { Wouldn't you want to use <= instead of < for the comparisons here? I don't know vvc so I can't say for sure, but that would seem logical to me. Are all aspect ratio combinations allowed here? E.g. the log mentions that you're testing 28x4 blocks. In dav1d, similar cases use logic of testing w in the range of [h/4, h*4] (plus limited to e.g. [4,64] at the same time). That would reduce the number of combinations to test, if they aren't really valid in practice. > + if (check_func(c->alf.filter[LUMA], "vvc_alf_filter_luma_%dx%d_%d", w, h, bit_depth)) { > + memset(dst0, 0, BUF_SIZE); > + memset(dst1, 0, BUF_SIZE); > + call_ref(dst0, stride, src0 + offset, stride, w, h, filter, clip); > + call_new(dst1, stride, src1 + offset, stride, w, h, filter, clip); > + for (int i = 0; i < h; i++) { > + if (memcmp(dst0 + i * stride, dst1 + i * stride, w * SIZEOF_PIXEL)) > + fail(); > + } > + bench_new(dst1, stride, src1 + offset, stride, w, h, filter, clip); > + } > + if (check_func(c->alf.filter[CHROMA], "vvc_alf_filter_chroma_%dx%d_%d", w, h, bit_depth)) { > + memset(dst0, 0, BUF_SIZE); > + memset(dst1, 0, BUF_SIZE); > + call_ref(dst0, stride, src0 + offset, stride, w, h, filter, clip); > + call_new(dst1, stride, src1 + offset, stride, w, h, filter, clip); > + for (int i = 0; i < h; i++) { > + if (memcmp(dst0 + i * stride, dst1 + i * stride, w * SIZEOF_PIXEL)) > + fail(); > + } > + bench_new(dst1, stride, src1 + offset, stride, w, h, filter, clip); > + } > + } > + } > +} > + > +void checkasm_check_vvc_alf(void) > +{ > + int bit_depth = 10; > + VVCDSPContext h; > + ff_vvc_dsp_init(&h, bit_depth); > + check_alf_luma_filter(&h, bit_depth); > + report("alf_filter"); Is 10 bits the only currently valid bitdepth here? Otherwise I think we should include all of them, even if there's only assembly for one case at the moment. (The pixel_mask seems to assume 8/10/12 bits.) // Martin
On Tue, Feb 28, 2023 at 7:51 PM Martin Storsjö <martin@martin.st> wrote: > On Sun, 26 Feb 2023, Nuo Mi wrote: > > > +#include <string.h> > > + > > +#include "libavutil/intreadwrite.h" > > +#include "libavutil/mem_internal.h" > > + > > +#include "libavcodec/avcodec.h" > > + > > +#include "libavcodec/vvcdsp.h" > > +#include "libavcodec/vvcdec.h" > > + > > +#include "checkasm.h" > > + > > +static const uint32_t pixel_mask[3] = { 0xffffffff, 0x03ff03ff, > 0x0fff0fff }; > > + > > +#define SIZEOF_PIXEL ((bit_depth + 7) / 8) > > +#define PIXEL_STRIDE (ALF_SUBBLOCK_SIZE + 2 * ALF_PADDING_SIZE) > > +#define BUF_SIZE (PIXEL_STRIDE * (MAX_CTU_SIZE + 3 * 2) * 2) //+3 * 2 > for top and bottom row, *2 for high bit depth > > +#define LUMA_PARAMS_SIZE (ALF_SUBBLOCK_SIZE / 4 * ALF_SUBBLOCK_SIZE / 4 > * ALF_NUM_COEFF_LUMA) > > + > > +#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) > > + > > +#define randomize_buffers2(buf, size, filter) \ > > + do { \ > > + int k; \ > > + if (filter) { \ > > + for (k = 0; k < size; k++) { \ > > + uint8_t r = rnd(); \ > > + buf[k] = r; \ > > + } \ > > + } else { \ > > + for (k = 0; k < size; k++) { \ > > + int16_t r = rnd(); \ > > + buf[k] = r; \ > > + } \ > > + } \ > > + } while (0) > > I don't quite see the point of the extra uint8_t/int16_t variable r here - > you could just as well assign it directly to buf[k], no? Unless you > specifically want the effect where you're narrowing the random value to a > smaller range beforehand. > yes. clips can only get special values. https://github.com/ffvvc/FFmpeg/blob/main/libavcodec/vvc_filter_template.c#L653 fixed by https://github.com/ffvvc/FFmpeg/pull/42/commits/3034ea8050f7f2db580ef4a36bd806e16be943d4#diff-99295cc07508b29c356b6432f8dbd72c5dceb37584fb8b4be0d34da19455864dR61 > > > + > > +static void check_alf_luma_filter(VVCDSPContext *c, const int bit_depth) > > +{ > > + LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]); > > + LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]); > > + LOCAL_ALIGNED_32(uint8_t, src0, [BUF_SIZE]); > > + LOCAL_ALIGNED_32(uint8_t, src1, [BUF_SIZE]); > > + int8_t filter[LUMA_PARAMS_SIZE]; > > + int16_t clip[LUMA_PARAMS_SIZE]; > > + ptrdiff_t stride = PIXEL_STRIDE * SIZEOF_PIXEL; > > + int offset = (3 * PIXEL_STRIDE + 3) * SIZEOF_PIXEL; > > + > > + declare_func_emms(AV_CPU_FLAG_AVX2, void, uint8_t *dst, ptrdiff_t > dst_stride, const uint8_t *src, ptrdiff_t src_stride, > > + int width, int height, const int8_t *filter, const int16_t > *clip); > > + > > + randomize_buffers(src0, src1, BUF_SIZE); > > + randomize_buffers2(filter, LUMA_PARAMS_SIZE, 1); > > + randomize_buffers2(clip, LUMA_PARAMS_SIZE, 2); > > Here, both invocations of randomize_buffers2 are called with filter=1 or > 2, both which will pick the "if (filter) {" case, so the else in > randomize_buffers2 is unused? > fixed > > > + > > + for (int h = 4; h < ALF_SUBBLOCK_SIZE; h += 4) { > > + for (int w = 4; w < ALF_SUBBLOCK_SIZE; w += 4) { > > Wouldn't you want to use <= instead of < for the comparisons here? I don't > know vvc so I can't say for sure, but that would seem logical to me. > you are right, fixed. > > Are all aspect ratio combinations allowed here? E.g. the log mentions that > you're testing 28x4 blocks. In dav1d, similar cases use logic of testing w > in the range of [h/4, h*4] (plus limited to e.g. [4,64] at the same time). > That would reduce the number of combinations to test, if they aren't > really valid in practice. > yes. almost all 4x width and height are valid. last col or row may have a small width and height. > > > + if (check_func(c->alf.filter[LUMA], > "vvc_alf_filter_luma_%dx%d_%d", w, h, bit_depth)) { > > + memset(dst0, 0, BUF_SIZE); > > + memset(dst1, 0, BUF_SIZE); > > + call_ref(dst0, stride, src0 + offset, stride, w, h, > filter, clip); > > + call_new(dst1, stride, src1 + offset, stride, w, h, > filter, clip); > > + for (int i = 0; i < h; i++) { > > + if (memcmp(dst0 + i * stride, dst1 + i * stride, w > * SIZEOF_PIXEL)) > > + fail(); > > + } > > + bench_new(dst1, stride, src1 + offset, stride, w, h, > filter, clip); > > + } > > + if (check_func(c->alf.filter[CHROMA], > "vvc_alf_filter_chroma_%dx%d_%d", w, h, bit_depth)) { > > + memset(dst0, 0, BUF_SIZE); > > + memset(dst1, 0, BUF_SIZE); > > + call_ref(dst0, stride, src0 + offset, stride, w, h, > filter, clip); > > + call_new(dst1, stride, src1 + offset, stride, w, h, > filter, clip); > > + for (int i = 0; i < h; i++) { > > + if (memcmp(dst0 + i * stride, dst1 + i * stride, w > * SIZEOF_PIXEL)) > > + fail(); > > + } > > + bench_new(dst1, stride, src1 + offset, stride, w, h, > filter, clip); > > + } > > + } > > + } > > +} > > + > > +void checkasm_check_vvc_alf(void) > > +{ > > + int bit_depth = 10; > > + VVCDSPContext h; > > + ff_vvc_dsp_init(&h, bit_depth); > > + check_alf_luma_filter(&h, bit_depth); > > + report("alf_filter"); > > Is 10 bits the only currently valid bitdepth here? Otherwise I think we > should include all of them, even if there's only assembly for one case at > the moment. (The pixel_mask seems to assume 8/10/12 bits.) > I use the 10 bits version to collect performance suggestions. After it done, I will add 8 and 12 bits version But you are right. I can add 8/12 bits test first. > > // Martin > > Hi Martin, Thank you for the review. comments inline.
diff --git a/tests/checkasm/Makefile b/tests/checkasm/Makefile index a6f06c7007..2ffe8bf112 100644 --- a/tests/checkasm/Makefile +++ b/tests/checkasm/Makefile @@ -34,6 +34,7 @@ AVCODECOBJS-$(CONFIG_V210_DECODER) += v210dec.o AVCODECOBJS-$(CONFIG_V210_ENCODER) += v210enc.o AVCODECOBJS-$(CONFIG_VORBIS_DECODER) += vorbisdsp.o AVCODECOBJS-$(CONFIG_VP9_DECODER) += vp9dsp.o +AVCODECOBJS-$(CONFIG_VVC_DECODER) += vvc_alf.o CHECKASMOBJS-$(CONFIG_AVCODEC) += $(AVCODECOBJS-yes) diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c index e96d84a7da..3c913eede4 100644 --- a/tests/checkasm/checkasm.c +++ b/tests/checkasm/checkasm.c @@ -171,6 +171,9 @@ static const struct { #if CONFIG_VORBIS_DECODER { "vorbisdsp", checkasm_check_vorbisdsp }, #endif + #if CONFIG_VVC_DECODER + { "vvc_alf", checkasm_check_vvc_alf }, + #endif #endif #if CONFIG_AVFILTER #if CONFIG_AFIR_FILTER diff --git a/tests/checkasm/checkasm.h b/tests/checkasm/checkasm.h index 8744a81218..b33f9f2505 100644 --- a/tests/checkasm/checkasm.h +++ b/tests/checkasm/checkasm.h @@ -91,6 +91,7 @@ void checkasm_check_vp8dsp(void); void checkasm_check_vp9dsp(void); void checkasm_check_videodsp(void); void checkasm_check_vorbisdsp(void); +void checkasm_check_vvc_alf(void); struct CheckasmPerf; diff --git a/tests/checkasm/vvc_alf.c b/tests/checkasm/vvc_alf.c new file mode 100644 index 0000000000..fe3d2ce351 --- /dev/null +++ b/tests/checkasm/vvc_alf.c @@ -0,0 +1,120 @@ +/* + * Copyright (c) 2023 Nuo Mi <nuomi2021@gmail.com> + * + * 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/vvcdsp.h" +#include "libavcodec/vvcdec.h" + +#include "checkasm.h" + +static const uint32_t pixel_mask[3] = { 0xffffffff, 0x03ff03ff, 0x0fff0fff }; + +#define SIZEOF_PIXEL ((bit_depth + 7) / 8) +#define PIXEL_STRIDE (ALF_SUBBLOCK_SIZE + 2 * ALF_PADDING_SIZE) +#define BUF_SIZE (PIXEL_STRIDE * (MAX_CTU_SIZE + 3 * 2) * 2) //+3 * 2 for top and bottom row, *2 for high bit depth +#define LUMA_PARAMS_SIZE (ALF_SUBBLOCK_SIZE / 4 * ALF_SUBBLOCK_SIZE / 4 * ALF_NUM_COEFF_LUMA) + +#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) + +#define randomize_buffers2(buf, size, filter) \ + do { \ + int k; \ + if (filter) { \ + for (k = 0; k < size; k++) { \ + uint8_t r = rnd(); \ + buf[k] = r; \ + } \ + } else { \ + for (k = 0; k < size; k++) { \ + int16_t r = rnd(); \ + buf[k] = r; \ + } \ + } \ + } while (0) + +static void check_alf_luma_filter(VVCDSPContext *c, const int bit_depth) +{ + LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]); + LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]); + LOCAL_ALIGNED_32(uint8_t, src0, [BUF_SIZE]); + LOCAL_ALIGNED_32(uint8_t, src1, [BUF_SIZE]); + int8_t filter[LUMA_PARAMS_SIZE]; + int16_t clip[LUMA_PARAMS_SIZE]; + ptrdiff_t stride = PIXEL_STRIDE * SIZEOF_PIXEL; + int offset = (3 * PIXEL_STRIDE + 3) * SIZEOF_PIXEL; + + declare_func_emms(AV_CPU_FLAG_AVX2, void, uint8_t *dst, ptrdiff_t dst_stride, const uint8_t *src, ptrdiff_t src_stride, + int width, int height, const int8_t *filter, const int16_t *clip); + + randomize_buffers(src0, src1, BUF_SIZE); + randomize_buffers2(filter, LUMA_PARAMS_SIZE, 1); + randomize_buffers2(clip, LUMA_PARAMS_SIZE, 2); + + for (int h = 4; h < ALF_SUBBLOCK_SIZE; h += 4) { + for (int w = 4; w < ALF_SUBBLOCK_SIZE; w += 4) { + if (check_func(c->alf.filter[LUMA], "vvc_alf_filter_luma_%dx%d_%d", w, h, bit_depth)) { + memset(dst0, 0, BUF_SIZE); + memset(dst1, 0, BUF_SIZE); + call_ref(dst0, stride, src0 + offset, stride, w, h, filter, clip); + call_new(dst1, stride, src1 + offset, stride, w, h, filter, clip); + for (int i = 0; i < h; i++) { + if (memcmp(dst0 + i * stride, dst1 + i * stride, w * SIZEOF_PIXEL)) + fail(); + } + bench_new(dst1, stride, src1 + offset, stride, w, h, filter, clip); + } + if (check_func(c->alf.filter[CHROMA], "vvc_alf_filter_chroma_%dx%d_%d", w, h, bit_depth)) { + memset(dst0, 0, BUF_SIZE); + memset(dst1, 0, BUF_SIZE); + call_ref(dst0, stride, src0 + offset, stride, w, h, filter, clip); + call_new(dst1, stride, src1 + offset, stride, w, h, filter, clip); + for (int i = 0; i < h; i++) { + if (memcmp(dst0 + i * stride, dst1 + i * stride, w * SIZEOF_PIXEL)) + fail(); + } + bench_new(dst1, stride, src1 + offset, stride, w, h, filter, clip); + } + } + } +} + +void checkasm_check_vvc_alf(void) +{ + int bit_depth = 10; + VVCDSPContext h; + ff_vvc_dsp_init(&h, bit_depth); + check_alf_luma_filter(&h, bit_depth); + report("alf_filter"); +}