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

[Nicolas George]

Le primidi 11 germinal, an CCXXII, Olivier Langlois a écrit :
> Whenever time is requested to measure relative time, the monotonic clock,
> when available, is superior to the system realtime clock because:

Hi. Thanks for the patch. There is merit to your proposal, but I see several
problems that need discussing before going that way.

> It is not affected by discontinuous jumps in the system time
> 
> Concretely that means that successive timestamps will never go backward and
> time difference between 2 events will never be negative because of clock
> setting between the 2 events.

On a properly configured system, the wall time clock should not go back:
AFAIK, we do not know how to transport anything through a wormhole. But yes,
of course in practice it happens.

> Hence, this is an improvement to ffmpeg by using a more stable time source.

You can say it is an improvement only if you discussed the drawbacks. The
monotonic clock is not stable across reboots or between different systems.
For that reason, people may rightfully prefer the wall time clock for some
usages that may include synchronizing network streams and keeping recorded
samples related to real time.

For that reason, I believe that it should be made an option, at least
whenever it makes sense.

> Signed-off-by: Olivier Langlois <olivier at trillion01.com>
> ---
>  cmdutils_opencl.c            |  4 ++--
>  configure                    |  2 ++
>  ffmpeg.c                     | 10 +++++-----
>  ffplay.c                     | 26 +++++++++++++-------------
>  libavcodec/dct-test.c        |  8 ++++----
>  libavcodec/fft-test.c        |  4 ++--
>  libavcodec/motion-test.c     |  4 ++--
>  libavdevice/alsa-audio-dec.c |  2 +-
>  libavdevice/bktr.c           |  6 +++---
>  libavdevice/fbdev_dec.c      |  9 +++------
>  libavdevice/openal-dec.c     |  2 +-
>  libavdevice/oss_audio.c      |  2 +-
>  libavdevice/sndio_dec.c      |  2 +-
>  libavdevice/v4l.c            | 11 ++++-------
>  libavdevice/v4l2.c           | 10 ----------
>  libavdevice/x11grab.c        |  9 +++------
>  libavformat/avio.c           |  4 ++--
>  libavformat/network.c        |  4 ++--
>  libavformat/sapenc.c         |  2 +-
>  libavformat/utils.c          |  2 +-
>  libavutil/time.c             | 32 +++++++++++++++++++++++++++++++-
>  libavutil/time.h             | 20 ++++++++++++++++++++
>  tools/aviocat.c              |  6 +++---
>  23 files changed, 107 insertions(+), 74 deletions(-)

It would be more convenient for review to split the patch. At least separate
the commits introducing new APIs from commits changing the devices.

> diff --git a/configure b/configure
> index face2d2..f1e4071 100755
> --- a/configure
> +++ b/configure
> @@ -1633,6 +1633,7 @@ SYSTEM_FUNCS="
>      access
>      aligned_malloc
>      clock_gettime
> +    clock_nanosleep
>      closesocket
>      CommandLineToArgvW
>      CryptGenRandom
> @@ -4432,6 +4433,7 @@ check_func  ${malloc_prefix}posix_memalign      && enable posix_memalign
>  
>  check_func  access

>  check_func  clock_gettime || { check_func clock_gettime -lrt && add_extralibs -lrt; }
> +check_func  clock_nanosleep || { check_func clock_nanosleep && add_extralibs -lrt; }

These lines are subtly different, is it normal?

>  check_func  fcntl
>  check_func  fork
>  check_func  gethrtime

> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -811,7 +811,7 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
>  
>          /* TODO: audio: time filter; video: frame reordering (pts != dts) */
>          if (s->use_wallclock_as_timestamps)
> -            pkt->dts = pkt->pts = av_rescale_q(av_gettime(), AV_TIME_BASE_Q, st->time_base);
> +            pkt->dts = pkt->pts = av_rescale_q(av_gettime_monotonic(), AV_TIME_BASE_Q, st->time_base);
>  
>          if (!pktl && st->request_probe <= 0)
>              return ret;

This one looks wrong: use_wallclock_as_timestamps clearly means the wall
time clock, not the monotonic clock.

> --- 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().

That means av_gettime_monotonic() is not really guaranteed to be monotonic.
You can do nothing about obsolete systems that lack a monotonic clock, of
course, but sometimes, API users will want to know exactly what kind of time
they get.

It could be something like that:

typedef int64_t (AVGetTimeFunction *)(void);

AVGetTimeFunction av_gettime_get_wall(void);
AVGetTimeFunction av_gettime_get_monotonic(void);
AVGetTimeFunction av_gettime_get_monotonic_if_possible(void);

I did not give it much thought, though.

> + */
> +int64_t av_gettime_monotonic(void);
> +
> +/**
> + * Sleep for a period of time by using the monotic clock.
> + * If a monotonic clock is not available on the targeted platform,
> + * the implementation fallsback on using av_usleep. Although the duration
> + * is expressed in microseconds, the actual delay may be rounded to the
> + * precision of the system timer.
> + *
> + * @param  usec Number of microseconds to sleep.
> + * @return zero on success or (negative) error code.
> + */
> +int av_usleep_monotonic(int64_t usec);
> +
> +/**
>   * Sleep for a period of time.  Although the duration is expressed in
>   * microseconds, the actual delay may be rounded to the precision of the
>   * system timer.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140331/0695731b/attachment.asc>