[FFmpeg-devel] [PATCH 2/2] RFC: checkasm: motion: Test different h parameters

[Martin Storsjö]

On Thu, 4 Aug 2022, Martin Storsjö wrote:

> On Wed, 13 Jul 2022, Martin Storsjö wrote:
>
>> Previously, the checkasm test always passed h=8, so no other cases
>> were tested.
>> 
>> Out of the me_cmp functions, in practice, some functions are hardcoded
>> to always assume a 8x8 block (ignoring the h parameter), while others
>> do use the parameter. For those with hardcoded height, both the
>> reference C function and the assembly implementations ignore the
>> parameter similarly.
>> 
>> The documentation for the functions indicate that heights between
>> w/2 and 2*w, within the range of 4 to 16, should be supported. This
>> patch just tests random heights in that range, without knowing what
>> width the current function actually uses.
>> ---
>> I'm not sure if it's good to have checkasm exercise cases that
>> don't occur in practice or not. In particular, the aarch64
>> functions have a separate implementation for non-multiple-of-4
>> height, which probably doesn't ever get called in practice, while
>> other SIMD implementations lack that.
>> 
>> Alternatively, we'd improve the documentation for the expectations
>> for these functions and make the test match that, and remove the
>> unused non-multiple-of-4 case in the aarch64 assembly.
>> ---
>> tests/checkasm/motion.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>> 
>> diff --git a/tests/checkasm/motion.c b/tests/checkasm/motion.c
>> index 0112822174..79e4358941 100644
>> --- a/tests/checkasm/motion.c
>> +++ b/tests/checkasm/motion.c
>> @@ -45,7 +45,7 @@ static void test_motion(const char *name, me_cmp_func 
>> test_func)
>>     /* motion estimation can look up to 17 bytes ahead */
>>     static const int look_ahead = 17;
>> 
>> -    int i, x, y, d1, d2;
>> +    int i, x, y, h, d1, d2;
>>     uint8_t *ptr;
>>
>>     LOCAL_ALIGNED_16(uint8_t, img1, [WIDTH * HEIGHT]);
>> @@ -68,14 +68,16 @@ static void test_motion(const char *name, me_cmp_func 
>> test_func)
>>         for (i = 0; i < ITERATIONS; i++) {
>>             x = rnd() % (WIDTH - look_ahead);
>>             y = rnd() % (HEIGHT - look_ahead);
>> +            // Pick a random h between 4 and 16; pick an even value.
>> +            h = 4 + ((rnd() % (16 + 1 - 4)) & ~1);
>>
>>             ptr = img2 + y * WIDTH + x;
>> -            d2 = call_ref(NULL, img1, ptr, WIDTH, 8);
>> -            d1 = call_new(NULL, img1, ptr, WIDTH, 8);
>> +            d2 = call_ref(NULL, img1, ptr, WIDTH, h);
>> +            d1 = call_new(NULL, img1, ptr, WIDTH, h);
>>
>>             if (d1 != d2) {
>>                 fail();
>> -                printf("func: %s, x=%d y=%d, error: asm=%d c=%d\n", name, 
>> x, y, d1, d2);
>> +                printf("func: %s, x=%d y=%d h=%d, error: asm=%d c=%d\n", 
>> name, x, y, h, d1, d2);
>>                 break;
>>             }
>>         }
>> -- 
>> 2.25.1
>
> Ping

As there doesn't seem to be much opinion on this, I'll go ahead and land 
this (patch 1/2 was approved already), as this improves the test coverage 
for the aarch64 neon me_cmp assembly patches that are in progress.

// Martin