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

[Martin Storsjö]

On Mon, 28 Mar 2022, Ben Avison wrote:

> 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.

Oh, I see. I just ran into it when trying to compile on macOS, then edited 
it out and saw that it built fine there, but didn't try building for other 
platforms with the same modification.

> 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,

FWIW I think blockdsp.c might have been a bad example in that regard, as 
it expands the whole testcase with macros. (I chose it mainly as it was 
one of the shortest testcases.)

I think e.g. vp8dsp would have been a better example - with the toplevel 
checkasm_check_*() function just calling individual functions for the 
tests for various function groups. As check_func() can take a format 
string, you don't usually need the macro expansion for filling that in.

> 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.

Yes, that seems correct to do. And looking at other examples, e.g. vp8dsp, 
that also uses such a structure, with declare_func_*() outside of 
check_func() - in a function like check_loopfilter_simple().

// Martin