[FFmpeg-devel] [PATCH v2] Use monotonic clock for measuring rel time.

[LANGLOIS Olivier PIS -EXT]

> -----Original Message-----
> From: ffmpeg-devel-bounces at ffmpeg.org [mailto:ffmpeg-devel-
> bounces at ffmpeg.org] On Behalf Of Nicolas George
> Sent: Friday, April 25, 2014 5:14 AM
> To: FFmpeg development discussions and patches
> Cc: Olivier Langlois
> Subject: Re: [FFmpeg-devel] [PATCH v2] Use monotonic clock for measuring
> rel time.
> > diff --git a/libavutil/time.c b/libavutil/time.c index
> > 5a00e70..82f0abc 100644
> > --- a/libavutil/time.c
> > +++ b/libavutil/time.c
> > @@ -38,7 +38,15 @@
> >
> >  int64_t av_gettime(void)
> >  {
>
> > -#if HAVE_GETTIMEOFDAY
> > +#if HAVE_CLOCK_GETTIME
> > +    /*
> > +     * POSIX.1-2008 marks gettimeofday() as obsolete,
> > +     * recommending the use of clock_gettime(2) instead.
> > +     */
> > +    struct timespec ts;
> > +    clock_gettime(CLOCK_REALTIME, &ts);
> > +    return (int64_t)ts.tv_sec * 1000000 + ts.tv_nsec / 1000; #elif
> > +HAVE_GETTIMEOFDAY
>
> Is there any actual benefit to that beyond following POSIX exactly? AFAIK,
> systems supporting clock_gettime() are a strict subset of systems supporting
> gettimeofday(). All I can see here is more code complexity and an extra
> multiplication.
>

There is not a big benefit. The only one is that gettimeofday() man page indicate that the function is obsolete and that applications should use clock_gettime() instead.
Concerning the extra multiplication, it is not really extra as, on Linux at least, it is just postponed as the kernel will need to convert its internal system time from timespec to timeval:

http://lxr.free-electrons.com/source/kernel/time/timekeeping.c#L479

> > diff --git a/libavutil/time.h b/libavutil/time.h index
> > 90eb436..8e87717 100644
> > --- a/libavutil/time.h
> > +++ b/libavutil/time.h
> > @@ -29,6 +29,26 @@
> >  int64_t av_gettime(void);
> >
> >  /**
>
> > + * Get the current time in microseconds from the monotonic clock
> > + * if available. If a monotonic clock  is not available on the
> > + * targeted platform, the implementation fallsback on using
> > + * av_gettime().
> > + */
> > +int64_t av_gettime_monotonic(void);
>
> I do not remember if I evoked that problem: with this API, applications can
> not know if the monotonic clock is really supported, which makes this
> function mostly unusable in some cases, and potentially dangerous due to its
> misleading name.
>
> (For the last point, possible scenario:
>
>       int index = (end_time - start_time) / interval;
>       //if (index < 0) return 0; the clock is monotonic, no need to check
>       if (index > max) index = max;
>       frobnicate(array[index]);
>
> If the av_gettime_monotonic() is not monotonic, you get an out-of-bounds
> array access, possibly overwriting random addresses and opening security
> holes.
> )
>
> Maybe call it av_gettime_relative() (less misleading; just make sure
> "monotonic" appears in the doxy), and also add:
>
> int av_gettime_relative_is_monotonic(void)
> {
> #if HAVE_CLOCK_GETTIME && defined(CLOCK_MONOTONIC)
>     return 1;
> #else
>     return 0;
> #endif
> }
>
Good point. I agree to it. I will implement it and return back here with the result.

Thank you very much for your feedback and I apologize again for having not reach the end of your email earlier and having pulled the trigger too fast!

Greetings,
Olivier

Please ignore the confidentiality notice below.
It is automatically added without my consent.



________________________________
CONFIDENTIALITY : This e-mail and any attachments are confidential and may be privileged. If you are not a named recipient, please notify the sender immediately and do not disclose the contents to another person, use it for any purpose or store or copy the information in any medium.