[FFmpeg-devel] [PATCH 01/10] checkasm: Add vc1dsp in-loop deblocking filter tests

[Ben Avison]

On 25/03/2022 22:53, Martin Storsjö wrote:
> On Fri, 25 Mar 2022, Ben Avison wrote:
> 
>> +#define 
>> CHECK_LOOP_FILTER(func)                                             \
>> +    do 
>> {                                                                    \
>> +        if (check_func(h.func, "vc1dsp." #func)) 
>> {                          \
>> +            declare_func_emms(AV_CPU_FLAG_MMX, void, uint8_t *, int, 
>> int);  \
>> +            for (int count = 1000; count > 0; --count) 
>> {                    \
>> +                int pq = rnd() % 31 + 
>> 1;                                    \
>> +                RANDOMIZE_BUFFER8_MID_WEIGHTED(filter_buf, 24 * 
>> 24);        \
>> +                call_ref(filter_buf0 + 4 * 24 + 4, 24, 
>> pq);                 \
>> +                call_new(filter_buf1 + 4 * 24 + 4, 24, 
>> pq);                 \
>> +                if (memcmp(filter_buf0, filter_buf1, 24 * 
>> 24))              \
>> +                    
>> fail();                                                 \
>> +            
>> }                                                               \
>> +        
>> }                                                                   \
>> +        for (int j = 0; j < 24; 
>> ++j)                                        \
>> +            for (int i = 0; i < 24; 
>> ++i)                                    \
>> +                filter_buf1[24*j + i] = 0x60 + 0x40 * (i >= 4 && j >= 
>> 4);   \
>> +        if (check_func(h.func, "vc1dsp." #func "_bestcase")) 
>> {              \
>> +            declare_func_emms(AV_CPU_FLAG_MMX, void, uint8_t *, int, 
>> int);  \
>> +            bench_new(filter_buf1 + 4 * 24 + 4, 24, 
>> 1);                     \
>> +            (void) 
>> checked_call;                                            \
>> +        
>> }                                                                   \
>> +        if (check_func(h.func, "vc1dsp." #func "_worstcase")) 
>> {             \
>> +            declare_func_emms(AV_CPU_FLAG_MMX, void, uint8_t *, int, 
>> int);  \
>> +            bench_new(filter_buf1 + 4 * 24 + 4, 24, 
>> 31);                    \
>> +            (void) 
>> checked_call;                                            \
>> +        
>> }                                                                   \
> 
> (not a full review, just something that cropped up in initial build 
> testing)
> 
> Why do you have the "(void) checked_call;" here? The checked_call isn't 
> something that is universally defined; its availability depends on the 
> OS/arch combinations, on other combinations, call_new/call_ref just call 
> the function straight away without a wrapper.

OK, I missed that subtlety. My aim was to avoid the "unused variable" 
compiler warnings generated as a result of there being twice as many 
benchmark tests as correctness tests. I believe we need separate calls 
of check_func() to initialise the cycle counts for each benchmark, and 
copying the sequence of macros from checkasm/blockdsp.c, I was placing 
the declare_func_emms() invocations inside the if block that used 
check_func(). That meant that checked_call was initialised, but since 
the correctness test (call_ref / call_new) was in a different block 
scope, this checked_call declaration was never used.

Upon further investigation, I think it's valid to move the 
declare_func_emms() invocation up to the next largest block scope. That 
means it would only appear once rather than 3 times, and it wouldn't 
need the cast-to-void any more. Please do correct me if I'm wrong.

Ben