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

[Hendrik Leppkes]

On Wed, Dec 28, 2016 at 8:05 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> 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.
>

What I forgot to add, CLOCKS_PER_SEC is also only 1000, so only
millisecond precision, which probably plays into here.

- Hendrik