[FFmpeg-devel] [PATCHv2 1/1] checkasm/lpc: test compute_autocorr

[Martin Storsjö]

On Sun, 17 Dec 2023, Rémi Denis-Courmont wrote:

> Le sunnuntaina 17. joulukuuta 2023, 18.09.45 EET James Almer a écrit :
>> On 12/17/2023 6:13 AM, Rémi Denis-Courmont wrote:
>> > ---
>> > 
>> >   tests/checkasm/lpc.c | 47 ++++++++++++++++++++++++++++++++++++++++++--
>> >   1 file changed, 45 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/tests/checkasm/lpc.c b/tests/checkasm/lpc.c
>> > index 592e34c03d..9b33f8a3b0 100644
>> > --- a/tests/checkasm/lpc.c
>> > +++ b/tests/checkasm/lpc.c
>> > @@ -57,10 +57,46 @@ static void test_window(int len)
>> > 
>> >       bench_new(src, len, dst1);
>> > 
>> >   }
>> > 
>> > +static void test_compute_autocorr(ptrdiff_t len, int lag)
>> > +{
>> > +    LOCAL_ALIGNED(32, double, src, [5000 + 2 + MAX_LPC_ORDER]);
>> > +    LOCAL_ALIGNED(16, double, dst0, [MAX_LPC_ORDER + 1]);
>> > +    LOCAL_ALIGNED(16, double, dst1, [MAX_LPC_ORDER + 1]);
>> > +
>> > +    declare_func(void, const double *in, ptrdiff_t len, int lag, double
>> > *out); +
>> > +    av_assert0(lag >= 0 && lag <= MAX_LPC_ORDER);
>> > +
>> > +    for (int i = 0; i < MAX_LPC_ORDER; i++)
>> > +        src[i] = 0.;
>> > +
>> > +    src += MAX_LPC_ORDER;
>> > +
>> > +    for (ptrdiff_t i = 0; i < len; i++) {
>> > +        src[i] = (double)rnd() / (double)UINT_MAX;
>> > +    }
>> > +
>> > +    call_ref(src, len, lag, dst0);
>> > +    call_new(src, len, lag, dst1);
>> > +
>> > +    for (size_t i = 0; i < lag; i++) {
>> > +        if (!double_near_abs_eps(dst0[i], dst1[i], EPS)) {
>> 
>> checkasm: using random seed 2504816888
>> SSE2:
>>   - lpc.apply_welch_window_even [OK]
>>   - lpc.apply_welch_window_odd  [OK]
>> 0:  770.224646270451 -  770.382378714191 = -0.15773244374
>>     autocorr_10_sse2 (lpc.c:86)
>>   - lpc.compute_autocorr_10     [FAILED]
>> 0:  807.574416481743 -  807.732148925482 = -0.157732443739
>>     autocorr_30_sse2 (lpc.c:86)
>>   - lpc.compute_autocorr_30     [FAILED]
>> 0:  787.329053288888 -  787.486785732628 = -0.15773244374
>>     autocorr_32_sse2 (lpc.c:86)
>>   - lpc.compute_autocorr_32     [FAILED]
>> 
>> checkasm: using random seed 827008587
>> SSE2:
>>   - lpc.apply_welch_window_even [OK]
>>   - lpc.apply_welch_window_odd  [OK]
>>   - lpc.compute_autocorr_10     [OK]
>>   - lpc.compute_autocorr_30     [OK]
>>   - lpc.compute_autocorr_32     [OK]
>> 
>> Some seeds work, others don't. So i guess EPS is too small
>
> Rounding errors would not cause a constant gap across the different test cases. 
> This is most likely an off-by-one in the x86 code. I don't know if this is a 
> bug in the x86 code, or the test case being a little loose with input 
> parameters, and I have neither time, nor motivation not to mention skills to 
> figure that out, so there will be no test cases for this function form me 
> afterall.

FWIW, we've had these situations elsewhere before as well, in swscale, 
where the existing x86 assembly mismatches the C code in nontrivial ways, 
and we have new assembly (aarch64 in that case) that is missing a test 
(even if one was written) due to this.

First I considered if we should collect these extra checkasm tests in some 
branch somewhere, so they aren't lost, as they are useful when working on 
assembly on other architectures.

But rather than having the code rot, forgotten in a stray branch 
somewhere, I wonder if we should just go ahead and merge it with an #if 
!ARCH_X86 or something, together with a notable FIXME comment.

That would keep the test coverage for new asm implementations, avoid code 
rot, and leave the opportunity to sort things out easily available for 
whoever wants to dissect the old existing x86 assembly implementations.

That's clearly not ideal, but would pragmatically be better than to just 
not merge the new checkasm test at all. What do others think?

// Martin