diff mbox series

[FFmpeg-devel] libavutil/timestamp.h: Fix loss of precision in silencedetect timestamps for long files

Message ID 990375690.258086.1591479138605@mail.yahoo.com
State New
Headers show
Series [FFmpeg-devel] libavutil/timestamp.h: Fix loss of precision in silencedetect timestamps for long files | expand

Checks

Context Check Description
andriy/default pending
andriy/configure warning Failed to apply patch

Commit Message

Allan Cady June 6, 2020, 9:32 p.m. UTC 
[Repeat submission. I really don't know my way around git tools... this time I'm using "git imap-send" in hopes this will better please the ffmpeg submission gods. Thank you for your patience.]

When the silencedetect filter is run against long files, the output
timestamps gradually lose precision as the scan proceeds further into
the file. This is because the output is formatted (in
libavutil/timestamp.h) as "%.6g", which limits the total field
length. Eventually, for offsets greater than 100000 seconds (about 28
hours), fractions of a second disappear altogether, and the
timestamps are logged as whole seconds.

I propose changing the format to "%.6f",
which will give microsecond precision for all timestamps, regardless of offset.

The timestamp string length is limited to 32 characters
(AV_TS_MAX_STRING_SIZE), so this should still be plenty long enough
with the increased length (up to 10^25 seconds).

My interest is in fixing this problem for silencedetect, which
formats the timestamps by calling the macro av_ts2timestr, defined in
timestamp.h. Since av_ts2timestr is also used in many other places (I
count 21 c files), I have created a new macro, av_ts2timestr_format,
with a format string added as a parameter, and left the original
macro interface as is for other usages, to limit the scope of this
change. The same or similar change could be made for other cases
where better precision is desired.
---
libavfilter/af_silencedetect.c              | 14 +++++-----
libavutil/timestamp.h                        | 27 +++++++++++++++++---
tests/ref/fate/filter-metadata-silencedetect |  2 +-
3 files changed, 32 insertions(+), 11 deletions(-)

Comments

Michael Niedermayer June 7, 2020, 8:03 p.m. UTC | #1 
On Sat, Jun 06, 2020 at 09:32:18PM +0000, Allan Cady wrote:
> [Repeat submission. I really don't know my way around git tools... this time I'm using "git imap-send" in hopes this will better please the ffmpeg submission gods. Thank you for your patience.]
> 
> When the silencedetect filter is run against long files, the output
> timestamps gradually lose precision as the scan proceeds further into
> the file. This is because the output is formatted (in
> libavutil/timestamp.h) as "%.6g", which limits the total field
> length. Eventually, for offsets greater than 100000 seconds (about 28
> hours), fractions of a second disappear altogether, and the
> timestamps are logged as whole seconds.
> 
> I propose changing the format to "%.6f",
> which will give microsecond precision for all timestamps, regardless of offset.
> 
> The timestamp string length is limited to 32 characters
> (AV_TS_MAX_STRING_SIZE), so this should still be plenty long enough
> with the increased length (up to 10^25 seconds).
> 
> My interest is in fixing this problem for silencedetect, which
> formats the timestamps by calling the macro av_ts2timestr, defined in
> timestamp.h. Since av_ts2timestr is also used in many other places (I
> count 21 c files), I have created a new macro, av_ts2timestr_format,
> with a format string added as a parameter, and left the original
> macro interface as is for other usages, to limit the scope of this
> change. The same or similar change could be made for other cases
> where better precision is desired.
> ---
> libavfilter/af_silencedetect.c              | 14 +++++-----
> libavutil/timestamp.h                        | 27 +++++++++++++++++---
> tests/ref/fate/filter-metadata-silencedetect |  2 +-
> 3 files changed, 32 insertions(+), 11 deletions(-)

doesnt apply, this patch seems messed up in transit
Applying: libavutil/timestamp.h: Fix loss of precision in silencedetect timestamps for long files
error: corrupt patch at line 12
error: could not build fake ancestor
Patch failed at 0001 libavutil/timestamp.h: Fix loss of precision in silencedetect timestamps for long files
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

[...]
diff mbox series

Patch

diff --git a/libavfilter/af_silencedetect.c b/libavfilter/af_silencedetect.c
index ff3b219e77..13c6f6f498 100644
--- a/libavfilter/af_silencedetect.c
+++ b/libavfilter/af_silencedetect.c
@@ -32,6 +32,8 @@ 
#include "avfilter.h"
#include "internal.h"

+const char TS_FMT_SILENCEDETECT[] = "%.6f";
+
typedef struct SilenceDetectContext {
    const AVClass *class;
    double noise;              ///< noise amplitude ratio
@@ -87,11 +89,11 @@  static av_always_inline void update(SilenceDetectContext *s, AVFrame *insamples,
                s->start[channel] = insamples->pts + av_rescale_q(current_sample / s->channels + 1 - nb_samples_notify * s->independent_channels / s->channels,
                        (AVRational){ 1, s->last_sample_rate }, time_base);
                set_meta(insamples, s->mono ? channel + 1 : 0, "silence_start",
-                        av_ts2timestr(s->start[channel], &time_base));
+                        av_ts2timestr_fmt(s->start[channel], &time_base, TS_FMT_SILENCEDETECT));
                if (s->mono)
                    av_log(s, AV_LOG_INFO, "channel: %d | ", channel);
                av_log(s, AV_LOG_INFO, "silence_start: %s\n",
-                        av_ts2timestr(s->start[channel], &time_base));
+                        av_ts2timestr_fmt(s->start[channel], &time_base, TS_FMT_SILENCEDETECT));
            }
        }
    } else {
@@ -102,15 +104,15 @@  static av_always_inline void update(SilenceDetectContext *s, AVFrame *insamples,
            int64_t duration_ts = end_pts - s->start[channel];
            if (insamples) {
                set_meta(insamples, s->mono ? channel + 1 : 0, "silence_end",
-                        av_ts2timestr(end_pts, &time_base));
+                        av_ts2timestr_fmt(end_pts, &time_base, TS_FMT_SILENCEDETECT));
                set_meta(insamples, s->mono ? channel + 1 : 0, "silence_duration",
-                        av_ts2timestr(duration_ts, &time_base));
+                        av_ts2timestr_fmt(duration_ts, &time_base, TS_FMT_SILENCEDETECT));
            }
            if (s->mono)
                av_log(s, AV_LOG_INFO, "channel: %d | ", channel);
            av_log(s, AV_LOG_INFO, "silence_end: %s | silence_duration: %s\n",
-                    av_ts2timestr(end_pts, &time_base),
-                    av_ts2timestr(duration_ts, &time_base));
+                    av_ts2timestr_fmt(end_pts, &time_base, TS_FMT_SILENCEDETECT),
+                    av_ts2timestr_fmt(duration_ts, &time_base, TS_FMT_SILENCEDETECT));
        }
        s->nb_null_samples[channel] = 0;
        s->start[channel] = INT64_MIN;
diff --git a/libavutil/timestamp.h b/libavutil/timestamp.h
index e082f01b40..590c8642d8 100644
--- a/libavutil/timestamp.h
+++ b/libavutil/timestamp.h
@@ -32,6 +32,8 @@ 

#define AV_TS_MAX_STRING_SIZE 32

+#define AV_TS_FMT_DEFAULT "%.6g"
+
/**
  * Fill the provided buffer with a string containing a timestamp
  * representation.
@@ -53,6 +55,23 @@  static inline char *av_ts_make_string(char *buf, int64_t ts)
  */
#define av_ts2str(ts) av_ts_make_string((char[AV_TS_MAX_STRING_SIZE]){0}, ts)

+/**
+ * Fill the provided buffer with a string containing a timestamp time
+ * representation, allowing format specification.
+ *
+ * @param buf a buffer with size in bytes of at least AV_TS_MAX_STRING_SIZE
+ * @param ts the timestamp to represent
+ * @param tb the timebase of the timestamp
+ * @param format string for timestamp output, e.g. AV_TS_FMT_DEFAULT.
+ * @return the buffer in input
+ */
+static inline char *av_ts_make_time_string_format(char *buf, int64_t ts, AVRational *tb, const char *format)
+{
+    if (ts == AV_NOPTS_VALUE) snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
+    else                      snprintf(buf, AV_TS_MAX_STRING_SIZE, format, av_q2d(*tb) * ts);
+    return buf;
+}
+
/**
  * Fill the provided buffer with a string containing a timestamp time
  * representation.
@@ -64,15 +83,15 @@  static inline char *av_ts_make_string(char *buf, int64_t ts)
  */
static inline char *av_ts_make_time_string(char *buf, int64_t ts, AVRational *tb)
{
-    if (ts == AV_NOPTS_VALUE) snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
-    else                      snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.6g", av_q2d(*tb) * ts);
+    av_ts_make_time_string_format(buf, ts, tb, AV_TS_FMT_DEFAULT);
    return buf;
}

/**
- * Convenience macro, the return value should be used only directly in
+ * Convenience macros, the return value should be used only directly in
  * function arguments but never stand-alone.
  */
-#define av_ts2timestr(ts, tb) av_ts_make_time_string((char[AV_TS_MAX_STRING_SIZE]){0}, ts, tb)
+#define av_ts2timestr_fmt(ts, tb, fmt) av_ts_make_time_string_format((char[AV_TS_MAX_STRING_SIZE]){0}, ts, tb, fmt)
+#define av_ts2timestr(ts, tb) av_ts_make_time_string_format((char[AV_TS_MAX_STRING_SIZE]){0}, ts, tb, AV_TS_FMT_DEFAULT)

#endif /* AVUTIL_TIMESTAMP_H */
diff --git a/tests/ref/fate/filter-metadata-silencedetect b/tests/ref/fate/filter-metadata-silencedetect
index 16a9d07692..5d8063cb71 100644
--- a/tests/ref/fate/filter-metadata-silencedetect
+++ b/tests/ref/fate/filter-metadata-silencedetect
@@ -1,5 +1,5 @@ 
pkt_pts=0|tag:lavfi.silence_duration=0.523107|tag:lavfi.silence_end=0.690023|tag:lavfi.silence_start=0.736417
-pkt_pts=46080|tag:lavfi.silence_start=1.27626|tag:lavfi.silence_end=1.80751|tag:lavfi.silence_duration=0.531247
+pkt_pts=46080|tag:lavfi.silence_start=1.276259|tag:lavfi.silence_end=1.807506|tag:lavfi.silence_duration=0.531247
pkt_pts=92160
pkt_pts=138240
pkt_pts=184320