[FFmpeg-devel,PATCHv4] checkasm/lpc: test compute_autocorr

Also restrict length to even values, matching real uses.
This test is disabled, known broken, on x86.
---
 tests/checkasm/lpc.c | 57 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 3 deletions(-)

Rémi Denis-Courmont:
> Also restrict length to even values, matching real uses.

It seems to me that the flac encoder can use odd values (namely if the
user set an odd frame_size option or if it gets fed an odd number of
samples in which case the last frame will have an odd number of samples).

> This test is disabled, known broken, on x86.
> ---
>  tests/checkasm/lpc.c | 57 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/checkasm/lpc.c b/tests/checkasm/lpc.c
> index 592e34c03d..cb15e8245b 100644
> --- a/tests/checkasm/lpc.c
> +++ b/tests/checkasm/lpc.c
> @@ -16,6 +16,7 @@
>   * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>   */
>  
> +#include "libavutil/avassert.h"
>  #include "libavutil/mem_internal.h"
>  
>  #include "libavcodec/lpc.h"
> @@ -30,7 +31,7 @@
>          }                                                                 \
>      } while (0)
>  
> -#define EPS 0.005
> +#define EPS 0.0001
>  
>  static void test_window(int len)
>  {
> @@ -57,10 +58,51 @@ static void test_window(int len)
>      bench_new(src, len, dst1);
>  }
>  
> +#if !ARCH_X86
> +static void test_compute_autocorr(ptrdiff_t len, int lag)
> +{
> +    const double eps = EPS * (double)len;
> +    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 (int i = 0; i < 5000 + 2; 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)) {
> +            fprintf(stderr, "%zu: %- .12f - %- .12f = % .12g\n",
> +                    i, dst0[i], dst1[i], dst0[i] - dst1[i]);
> +            fail();
> +            break;
> +        }
> +    }
> +
> +    bench_new(src, 4608, lag, dst1);
> +}
> +#endif
> +
>  void checkasm_check_lpc(void)
>  {
>      LPCContext ctx;
> -    int len = rnd() % 5000;
> +    int len = 2000 + (rnd() % 1500) * 2;
> +#if !ARCH_X86
> +    static const int lags[] = { 8, 12, };
> +#endif
> +
>      ff_lpc_init(&ctx, 32, 16, FF_LPC_TYPE_DEFAULT);
>  
>      if (check_func(ctx.lpc_apply_welch_window, "apply_welch_window_even")) {
> @@ -72,6 +114,15 @@ void checkasm_check_lpc(void)
>          test_window(len | 1);
>      }
>      report("apply_welch_window_odd");
> -
>      ff_lpc_end(&ctx);
> +
> +#if !ARCH_X86
> +    for (size_t i = 0; i < FF_ARRAY_ELEMS(lags); i++) {
> +        ff_lpc_init(&ctx, len, lags[i], FF_LPC_TYPE_DEFAULT);
> +        if (check_func(ctx.lpc_compute_autocorr, "autocorr_%d", lags[i]))
> +            test_compute_autocorr(len, lags[i]);
> +        ff_lpc_end(&ctx);
> +    }
> +    report("compute_autocorr");
> +#endif
>  }

Le keskiviikkona 29. toukokuuta 2024, 22.55.13 EEST Andreas Rheinhardt a écrit 
:
> It seems to me that the flac encoder can use odd values (namely if the
> user set an odd frame_size option or if it gets fed an odd number of
> samples in which case the last frame will have an odd number of samples).

As noted earlier the C code seems to assume that len is even, and fixing it is 
outside the scope of this patch.

Rémi Denis-Courmont:
> Le keskiviikkona 29. toukokuuta 2024, 22.55.13 EEST Andreas Rheinhardt a écrit 
> :
>> It seems to me that the flac encoder can use odd values (namely if the
>> user set an odd frame_size option or if it gets fed an odd number of
>> samples in which case the last frame will have an odd number of samples).
> 
> As noted earlier the C code seems to assume that len is even, and fixing it is 
> outside the scope of this patch.
> 

I never said you should fix it, but you should not claim in your commit
message that the actual code is not used with odd lengths.

- Andreas

On 5/29/2024 4:42 PM, Rémi Denis-Courmont wrote:
> Also restrict length to even values, matching real uses.
> This test is disabled, known broken, on x86.
> ---
>   tests/checkasm/lpc.c | 57 +++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/checkasm/lpc.c b/tests/checkasm/lpc.c
> index 592e34c03d..cb15e8245b 100644
> --- a/tests/checkasm/lpc.c
> +++ b/tests/checkasm/lpc.c
> @@ -16,6 +16,7 @@
>    * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>    */
>   
> +#include "libavutil/avassert.h"
>   #include "libavutil/mem_internal.h"
>   
>   #include "libavcodec/lpc.h"
> @@ -30,7 +31,7 @@
>           }                                                                 \
>       } while (0)
>   
> -#define EPS 0.005
> +#define EPS 0.0001
>   
>   static void test_window(int len)
>   {
> @@ -57,10 +58,51 @@ static void test_window(int len)
>       bench_new(src, len, dst1);
>   }
>   
> +#if !ARCH_X86
> +static void test_compute_autocorr(ptrdiff_t len, int lag)
> +{
> +    const double eps = EPS * (double)len;
> +    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 (int i = 0; i < 5000 + 2; 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)) {
> +            fprintf(stderr, "%zu: %- .12f - %- .12f = % .12g\n",
> +                    i, dst0[i], dst1[i], dst0[i] - dst1[i]);
> +            fail();
> +            break;
> +        }
> +    }
> +
> +    bench_new(src, 4608, lag, dst1);
> +}
> +#endif
> +
>   void checkasm_check_lpc(void)
>   {
>       LPCContext ctx;
> -    int len = rnd() % 5000;
> +    int len = 2000 + (rnd() % 1500) * 2;

Instead of changing how len is generated, which will break known 
existing results for specific seeds in other tests, alter the value when 
passing it to test_compute_autocorr(), like 
apply_welch_window_{even,odd}() do.

> +#if !ARCH_X86
> +    static const int lags[] = { 8, 12, };
> +#endif
> +
>       ff_lpc_init(&ctx, 32, 16, FF_LPC_TYPE_DEFAULT);
>   
>       if (check_func(ctx.lpc_apply_welch_window, "apply_welch_window_even")) {
> @@ -72,6 +114,15 @@ void checkasm_check_lpc(void)
>           test_window(len | 1);
>       }
>       report("apply_welch_window_odd");
> -
>       ff_lpc_end(&ctx);
> +
> +#if !ARCH_X86
> +    for (size_t i = 0; i < FF_ARRAY_ELEMS(lags); i++) {
> +        ff_lpc_init(&ctx, len, lags[i], FF_LPC_TYPE_DEFAULT);
> +        if (check_func(ctx.lpc_compute_autocorr, "autocorr_%d", lags[i]))
> +            test_compute_autocorr(len, lags[i]);
> +        ff_lpc_end(&ctx);
> +    }
> +    report("compute_autocorr");
> +#endif
>   }

Le keskiviikkona 29. toukokuuta 2024, 23.52.20 EEST James Almer a écrit :
> On 5/29/2024 4:42 PM, Rém
> >   void checkasm_check_lpc(void)
> >   {
> >   
> >       LPCContext ctx;
> > 
> > -    int len = rnd() % 5000;
> > +    int len = 2000 + (rnd() % 1500) * 2;
> 
> Instead of changing how len is generated, which will break known
> existing results for specific seeds in other tests, alter the value when
> passing it to test_compute_autocorr(), like
> apply_welch_window_{even,odd}() do.

Existing benchmarks are unstable and unreliable because of the varying length. 
This was supposed to be contained by the 2000 minimum, but that means the 
benchmarks are invalidated regardless of the parity change.