[FFmpeg-devel] [PATCH, v2 4/4] tests/checkasm: add overflow test for hevc_add_res

[Fu, Linjie]

> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Monday, March 9, 2020 06:16
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH, v2 4/4] tests/checkasm: add overflow
> test for hevc_add_res
> 
> On Thu, Mar 05, 2020 at 03:48:28PM +0800, Linjie Fu wrote:
> > Add overflow test for hevc_add_res when int16_t coeff = -32768,
> > and doubled the test cases.
> >
> > The result of C is good, while ASM is not.
> >
> > To verify:
> >     make fate-checkasm-hevc_add_res
> >     ffmpeg/tests/checkasm/checkasm --test=hevc_add_res
> >
> > ./checkasm --test=hevc_add_res
> > checkasm: using random seed 679391863
> > MMXEXT:
> >     hevc_add_res_4x4_8_mmxext (hevc_add_res.c:69)
> >   - hevc_add_res.add_residual [FAILED]
> > SSE2:
> >     hevc_add_res_8x8_8_sse2 (hevc_add_res.c:69)
> >     hevc_add_res_16x16_8_sse2 (hevc_add_res.c:69)
> >     hevc_add_res_32x32_8_sse2 (hevc_add_res.c:69)
> >   - hevc_add_res.add_residual [FAILED]
> > AVX:
> >     hevc_add_res_8x8_8_avx (hevc_add_res.c:69)
> >     hevc_add_res_16x16_8_avx (hevc_add_res.c:69)
> >     hevc_add_res_32x32_8_avx (hevc_add_res.c:69)
> >   - hevc_add_res.add_residual [FAILED]
> > AVX2:
> >     hevc_add_res_32x32_8_avx2 (hevc_add_res.c:69)
> >   - hevc_add_res.add_residual [FAILED]
> > checkasm: 8 of 14 tests have failed
> >
> > Signed-off-by: Xu Guangxin <guangxin.xu at intel.com>
> > Signed-off-by: Linjie Fu <linjie.fu at intel.com>
> > ---
> > [v2]: test 2x cases to make sure enough random residuals
> > are tested.
> >
> >  tests/checkasm/hevc_add_res.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tests/checkasm/hevc_add_res.c
> b/tests/checkasm/hevc_add_res.c
> > index e92c6b4..8c82ac1 100644
> > --- a/tests/checkasm/hevc_add_res.c
> > +++ b/tests/checkasm/hevc_add_res.c
> > @@ -58,6 +58,7 @@ static void check_add_res(HEVCDSPContext h, int
> bit_depth)
> >
> >          randomize_buffers(res0, size);
> >          randomize_buffers2(dst0, size);
> > +        res0[0] = 0x8000;// overflow test
> >          memcpy(res1, res0, sizeof(*res0) * size);
> >          memcpy(dst1, dst0, sizeof(int16_t) * size);
> >
> > @@ -80,6 +81,7 @@ void checkasm_check_hevc_add_res(void)
> >
> >          ff_hevc_dsp_init(&h, bit_depth);
> >          check_add_res(h, bit_depth);
> > +        check_add_res(h, bit_depth);
> 
> Maybe iam mis-reading this diff but doesnt this run the same test
> twice instead of running it once with the frist elemet locked to 0x8000 and
> once freely floating ?
> 

Running twice seems to be enough for the randomness even if it's the smallest
block size (1/30 difference for 4x4 blocks).

Current patch to simply run twice:
0x8000 +  (size x size - 1) random residual; // 15 random residuals for 4x4
0x8000 +  (size x size - 1) random residual; // 15 random residuals for 4x4

If modify to lock only one time:
size x size		 random residual; // 16 random residuals for 4x4
0x8000 +  (size x size - 1) random residual; // 15 random residuals for 4x4


However as you've pointed out, this introduced misunderstandings and may be
treated as redundant code someday.

It would be better to be treated more explicitly as you've suggested.
Will update, thx.

- Linjie