[FFmpeg-devel] [FFmpeg-cvslog] checkasm: test for abs_pow34

[Michael Niedermayer]

On Tue, Jan 23, 2024 at 04:40:46PM -0300, James Almer wrote:
> On 1/23/2024 4:33 PM, Michael Niedermayer wrote:
> > On Tue, Jan 23, 2024 at 04:17:22PM -0300, James Almer wrote:
> > > On 1/23/2024 4:07 PM, Michael Niedermayer wrote:
> > > > On Mon, Dec 11, 2023 at 04:42:26PM +0000, sunyuechi wrote:
> > > > > ffmpeg | branch: master | sunyuechi <sunyuechi at iscas.ac.cn> | Tue Nov 28 14:08:12 2023 +0800| [1c3620b2bbe73db9239fcf605e8f535b58f03b86] | committer: Rémi Denis-Courmont
> > > > > 
> > > > > checkasm: test for abs_pow34
> > > > > 
> > > > > Signed-off-by: Rémi Denis-Courmont <remi at remlab.net>
> > > > > 
> > > > > > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=1c3620b2bbe73db9239fcf605e8f535b58f03b86
> > > > > ---
> > > > > 
> > > > >    tests/checkasm/Makefile    |  1 +
> > > > >    tests/checkasm/aacencdsp.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > >    tests/checkasm/checkasm.c  |  3 ++
> > > > >    tests/checkasm/checkasm.h  |  1 +
> > > > >    tests/fate/checkasm.mak    |  3 +-
> > > > >    5 files changed, 77 insertions(+), 1 deletion(-)
> > > > [...]
> > > > > +static void test_abs_pow34(AACEncContext *s) {
> > > > > +#define BUF_SIZE 1024
> > > > > +    LOCAL_ALIGNED_32(float, in, [BUF_SIZE]);
> > > > > +
> > > > > +    declare_func(void, float *, const float *, int);
> > > > > +
> > > > > +    randomize_float(in, BUF_SIZE);
> > > > > +
> > > > > +    if (check_func(s->abs_pow34, "abs_pow34")) {
> > > > > +        LOCAL_ALIGNED_32(float, out, [BUF_SIZE]);
> > > > > +        LOCAL_ALIGNED_32(float, out2, [BUF_SIZE]);
> > > > > +
> > > > > +        call_ref(out, in, BUF_SIZE);
> > > > > +        call_new(out2, in, BUF_SIZE);
> > > > > +
> > > > > +        if (memcmp(out, out2, BUF_SIZE * sizeof(float)) != 0)
> > > > > +            fail();
> > > > 
> > > > This is wrong if one of the functions is implemented in C
> > > > C does not specify float to give bitexact results. Any equality check with
> > > > float thats expected to end in a reproduceable outcome is generally wrong
> > > 
> > > Yes, that's why float_near_abs_eps_array() or float_near_ulp_array() should
> > > be used.
> > > 
> > > > 
> > > > it also fails in practice
> > > > 
> > > > TEST    checkasm-aacencdsp
> > > > Test checkasm-aacencdsp failed. Look at tests/data/fate/checkasm-aacencdsp.err for details.
> > > > src/tests/Makefile:308: recipe for target 'fate-checkasm-aacencdsp' failed
> > > > make: *** [fate-checkasm-aacencdsp] Error 1
> > > > 
> > > > checkasm: using random seed 2523748868
> > > > SSE:
> > > >      abs_pow34_sse (aacencdsp.c:55)
> > > >    - aacencdsp.abs_pow34 [FAILED]
> > > > checkasm: 1 of 1 tests have failed
> > > > threads=1
> > > > 
> > > > thats on plain simple x86-32
> > > 
> > > Can you check if the following passes for x86-32?
> > > 
> > > > diff --git a/tests/checkasm/aacencdsp.c b/tests/checkasm/aacencdsp.c
> > > > index 684c775862..7cc8a01158 100644
> > > > --- a/tests/checkasm/aacencdsp.c
> > > > +++ b/tests/checkasm/aacencdsp.c
> > > > @@ -19,6 +19,7 @@
> > > >    */
> > > > 
> > > >   #include <string.h>
> > > > +#include <float.h>
> > > > 
> > > >   #include "libavutil/mem.h"
> > > >   #include "libavutil/mem_internal.h"
> > > > @@ -51,7 +52,7 @@ static void test_abs_pow34(AACEncContext *s) {
> > > >           call_ref(out, in, BUF_SIZE);
> > > >           call_new(out2, in, BUF_SIZE);
> > > > 
> > > > -        if (memcmp(out, out2, BUF_SIZE * sizeof(float)) != 0)
> > > > +        if (!float_near_abs_eps_array(out, out2, FLT_EPSILON, BUF_SIZE))
> > > >               fail();
> > > > 
> > > >           bench_new(out, in, BUF_SIZE);
> > > 
> > > That target tends to be pretty picky about the epsilon value because of the
> > > 80 bits precision for floats.
> > 
> > 
> > with float.h
> > 
> > float_near_abs_eps_array
> > 
> > Test checkasm-aacencdsp failed. Look at tests/data/fate/checkasm-aacencdsp.err for details.
> > checkasm: using random seed 4097080137
> > test failed comparing 5.68635 with 5.68635 (abs diff=4.76837e-07 with EPS=1.19209e-07)
> > SSE:
> >     abs_pow34_sse (aacencdsp.c:56)
> >   - aacencdsp.abs_pow34 [FAILED]
> > checkasm: 1 of 1 tests have failed
> > threads=1
> > src/tests/Makefile:317: recipe for target 'fate-checkasm-aacencdsp' failed
> > make: *** [fate-checkasm-aacencdsp] Error 1
> > 
> > if (!float_near_abs_eps_array(out, out2, FLT_EPSILON*10, BUF_SIZE))
> > works but feels wrong
> > 
> > this works too (only tested 100x on only x86-32 yet)
> > if (!float_near_ulp_array(out, out2, 1, BUF_SIZE))
> > 
> > thx
> 
> 1 is excessive. Some tests use an epsilon value like 0.005, so
> FLT_EPSILON*10 is IMO fine. Or we could hardcode something like 0.000005
> instead if it passes.

1 ULP is the smallest that works
tested 100x on mips, arm, mingw32 now too

epsilon anything is IMHO wrong but iam happy to hear an argument why
absolute is better than relative here (which is the difference bascially
IIUC)

absolute is bad as the difference in what can be represented is relative
to the values size and so a absolute check will inevitably be very loose
on small values or will fail on large ones

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The day soldiers stop bringing you their problems is the day you have stopped 
leading them. They have either lost confidence that you can help or concluded 
you do not care. Either case is a failure of leadership. - Colin Powell
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240123/2260e949/attachment.sig>