[FFmpeg-devel] [PATCH] avutil/tests: Improved code coverage for random_seed

[Hendrik Leppkes]

On Wed, Dec 28, 2016 at 7:57 PM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Wed, Dec 28, 2016 at 07:12:25PM +0100, Hendrik Leppkes wrote:
>> On Wed, Dec 28, 2016 at 7:08 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>> > On Fri, Dec 23, 2016 at 1:12 AM, Thomas Turner <thomastdt at googlemail.com> wrote:
>> >> Signed-off-by: Thomas Turner <thomastdt at googlemail.com>
>> >> ---
>> >>  libavutil/tests/random_seed.c | 34 +++++++++++++++++++++-------------
>> >>  tests/ref/fate/random_seed    |  1 +
>> >>  2 files changed, 22 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/libavutil/tests/random_seed.c b/libavutil/tests/random_seed.c
>> >> index ebe9b3e..fcd68bc 100644
>> >> --- a/libavutil/tests/random_seed.c
>> >> +++ b/libavutil/tests/random_seed.c
>> >> @@ -23,24 +23,32 @@
>> >>
>> >>  #undef printf
>> >>  #define N 256
>> >> +#define F 2
>> >>  #include <stdio.h>
>> >>
>> >> +typedef uint32_t (*random_seed_ptr_t)(void);
>> >> +
>> >>  int main(void)
>> >>  {
>> >> -    int i, j, retry;
>> >> +    int i, j, rsf, retry;
>> >>      uint32_t seeds[N];
>> >> +    random_seed_ptr_t random_seed[F] = {av_get_random_seed, get_generic_seed};
>> >>
>> >> -    for (retry=0; retry<3; retry++){
>> >> -        for (i=0; i<N; i++){
>> >> -            seeds[i] = av_get_random_seed();
>> >> -            for (j=0; j<i; j++)
>> >> -                if (seeds[j] == seeds[i])
>> >> -                    goto retry;
>> >> +    for (rsf=0; rsf<F; ++rsf){
>> >> +        for (retry=0; retry<3; retry++){
>> >> +            for (i=0; i<N; i++){
>> >> +                seeds[i] = random_seed[rsf]();
>> >> +                for (j=0; j<i; j++)
>> >> +                    if (seeds[j] == seeds[i])
>> >> +                        goto retry;
>> >> +            }
>> >> +            printf("seeds OK\n");
>> >> +            goto next;
>> >> +            retry:;
>> >>          }
>> >> -        printf("seeds OK\n");
>> >> -        return 0;
>> >> -        retry:;
>> >> +        printf("rsf %d: FAIL at %d with %X\n", rsf, j, seeds[j]);
>> >> +        return 1;
>> >> +        next:;
>> >>      }
>> >> -    printf("FAIL at %d with %X\n", j, seeds[j]);
>> >> -    return 1;
>> >> -}
>> >> +    return 0;
>> >> + }
>> >> \ No newline at end of file
>> >> diff --git a/tests/ref/fate/random_seed b/tests/ref/fate/random_seed
>> >> index 2b5b3af..ef0eef2 100644
>> >> --- a/tests/ref/fate/random_seed
>> >> +++ b/tests/ref/fate/random_seed
>> >> @@ -1 +1,2 @@
>> >>  seeds OK
>> >> +seeds OK
>> >> --
>> >> 1.9.1
>> >
>> > The new test sporadically fails on msvc x86_64 for some reason. What
>> > does it actually mean when it fails, ie. what does this thing test?
>> >
>>
>> Specifically, it always fails with rsf 1, which seems to be
>> get_generic_seed  - but windows has a special crypto seed provider,
>> get_generic_seed is never used. Making fate fail for some inaccuracy
>> in the clock in code thats never used is a bit annoying.
>
> Thats like saying that as long as asm works it doesnt matter if the C
> code doesnt work.

get_generic_seed is literally dead code in such a build, so its not
the same thing. Which platforms do even realistically use it at all?
On top of that, its behavior depends on platform specifics, ie. the
behavior of the clock function.

>
> get_generic_seed() should work on any platform, ideally.
> why does it fail on windows ?
> can you take a look, its probably not very hard to improve it, the
> function is also quite short.
>

I do not have the time right now to debug random sporadic failures,
since I'm going out of country in a few days for a couple weeks.
The MSDN documents the clock function as follows:

The clock function tells how much wall-clock time the calling process
has used. Note that this is not strictly conformant with ISO C99,
which specifies net CPU time as the return value. To obtain CPU time,
use the Win32 GetProcessTimes function.

So if the function is super fast, its certainly possible the values
just don't increment, since its wall-clock based. But its generally
not a problem, since its just never used.

- Hendrik