[FFmpeg-devel] [PATCH v2] Use monotonic clock for measuring rel time.
Nicolas George
george at nsup.org
Fri Apr 25 11:14:18 CEST 2014
Le quintidi 5 floréal, 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 it is
> not affected by discontinuous jumps in the system time
>
> Scale down the modification from the previous version of the patch
> to only use monotonic clock when relative time is required.
>
> The first version was erroneously modifying avpackets ts coming from
> avdevices which could have adverse effects to current ffmpeg users.
>
> If this patch is accepted, I will look into offering the flexibility to let
> the user choose between rt and monotonic clock for avdevices packets.
Thanks.
> It is very easy to experience the issues that this patch attempt to address
> by rewinding back in the past the system time while ffmpeg is running.
>
> this will break the ffmpeg report printing (ffmepg.c:print_report()) and
> the the rate emulator functionality (-re) without the patch.
>
> Signed-off-by: Olivier Langlois <olivier at trillion01.com>
> Subject: Re: [FFmpeg-devel] [PATCH v2] Use monotonic clock for measuring rel
> time.
Since your patch also adds the extra API, I feel the summary of the commit
message should be driven by it:
# lavu: add av_gettime_monotonic() and use it.
#
# Also add similar av_usleep_monotonic().
#
# further explanations
> ---
> 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/v4l2.c | 10 ----------
> libavformat/avio.c | 4 ++--
> libavformat/network.c | 4 ++--
> libavformat/sapenc.c | 2 +-
> libavutil/time.c | 32 +++++++++++++++++++++++++++++++-
> libavutil/time.h | 20 ++++++++++++++++++++
> tools/aviocat.c | 6 +++---
> 14 files changed, 89 insertions(+), 47 deletions(-)
>
> diff --git a/cmdutils_opencl.c b/cmdutils_opencl.c
> diff --git a/ffmpeg.c b/ffmpeg.c
> diff --git a/ffplay.c b/ffplay.c
> diff --git a/libavcodec/dct-test.c b/libavcodec/dct-test.c
> diff --git a/libavcodec/fft-test.c b/libavcodec/fft-test.c
> diff --git a/libavcodec/motion-test.c b/libavcodec/motion-test.c
> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
> diff --git a/libavformat/avio.c b/libavformat/avio.c
> diff --git a/libavformat/network.c b/libavformat/network.c
> diff --git a/libavformat/sapenc.c b/libavformat/sapenc.c
> diff --git a/tools/aviocat.c b/tools/aviocat.c
These changes look all valid to me. But please wait some more for the answer
of the maintainer of each file, especially Marton for ffplay.
> 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.
> struct timeval tv;
> gettimeofday(&tv, NULL);
> return (int64_t)tv.tv_sec * 1000000 + tv.tv_usec;
> @@ -53,6 +61,17 @@ int64_t av_gettime(void)
> #endif
> }
>
> +int64_t av_gettime_monotonic(void)
> +{
> +#if HAVE_CLOCK_GETTIME && defined(CLOCK_MONOTONIC)
> + struct timespec ts;
> + clock_gettime(CLOCK_MONOTONIC, &ts);
> + return (int64_t)ts.tv_sec * 1000000 + ts.tv_nsec / 1000;
> +#else
> + return av_gettime();
> +#endif
> +}
> +
> int av_usleep(unsigned usec)
> {
> #if HAVE_NANOSLEEP
> @@ -68,3 +87,14 @@ int av_usleep(unsigned usec)
> return AVERROR(ENOSYS);
> #endif
> }
> +
> +int av_usleep_monotonic(int64_t usec)
> +{
> +#if HAVE_CLOCK_NANOSLEEP && defined(CLOCK_MONOTONIC)
> + struct timespec ts = { usec / 1000000, usec % 1000000 * 1000 };
> + while (clock_nanosleep(CLOCK_MONOTONIC, 0, &ts, &ts) < 0 && errno == EINTR);
> + return 0;
> +#else
> + return av_usleep((unsigned)usec);
> +#endif
> +}
> 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
}
> +
> +/**
> + * 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/20140425/ed8b4f2b/attachment.asc>
More information about the ffmpeg-devel
mailing list