diff mbox series

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

Message ID 1583394508-21848-1-git-send-email-linjie.fu@intel.com
State Accepted
Headers show
Series Untitled series #463
Related show

Commit Message

Fu, Linjie March 5, 2020, 7:48 a.m. UTC
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@intel.com>
Signed-off-by: Linjie Fu <linjie.fu@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(+)

Comments

Fu, Linjie March 8, 2020, 8:38 a.m. UTC | #1
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Linjie Fu
> Sent: Thursday, March 5, 2020 15:48
> To: ffmpeg-devel@ffmpeg.org
> Cc: Xu, Guangxin <guangxin.xu@intel.com>; Fu, Linjie <linjie.fu@intel.com>
> Subject: [FFmpeg-devel] [PATCH, v2 4/4] tests/checkasm: add overflow test
> for hevc_add_res
> 
> 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@intel.com>
> Signed-off-by: Linjie Fu <linjie.fu@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);
>      }
>      report("add_residual");
>  }
> --

Ping.
Michael Niedermayer March 8, 2020, 10:16 p.m. UTC | #2
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@intel.com>
> Signed-off-by: Linjie Fu <linjie.fu@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 ?

thx

[...]
Fu, Linjie March 9, 2020, 2:57 a.m. UTC | #3
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Monday, March 9, 2020 06:16
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@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@intel.com>
> > Signed-off-by: Linjie Fu <linjie.fu@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
diff mbox series

Patch

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);
     }
     report("add_residual");
 }