From patchwork Sun May 31 04:38:38 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Allan Cady X-Patchwork-Id: 20026 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id C8C17448A9F for ; Sun, 31 May 2020 07:39:01 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id A3C05688187; Sun, 31 May 2020 07:39:01 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from sonic309-15.consmr.mail.bf2.yahoo.com (sonic309-15.consmr.mail.bf2.yahoo.com [74.6.129.125]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 4E47668806E for ; Sun, 31 May 2020 07:38:55 +0300 (EEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1590899933; bh=q5+ahc1T6Eki0FNjHSslaaShBVUH5ON2r/E1afZHPtg=; h=Date:From:To:Subject:References:From:Subject; b=n7qpVRYREXHbsZsSrswcUAiWOPQWrEphRUjmRcxMt/8oJa034Bpn5qc9lEb6SjaSrS15lfpjYy32+3Zeo8nMwqAf4CSNudI5J4cMhrDGSJZ+wMxyfO/S9r0EWX8LIsACu/Eou9K3OpHA0rvtq5lm/76DG7v3g1zwhrYr5JjpTm1h5CF5JJiqsMfhYHy5ZpyLAgHeWFEcOpzf+j5dXQzPcN7YQjyQtmi2d6iU1JGxWSaAGDQqjPV2ngDyhS3f/fkJEts0nAsRLUQE3RxRxoXRlE82JTgBWU8GpHTeu9PXqkkrImkXZnKdOWCq7pHuWrFz7cfw+L4K8GfTpwoelidfLg== X-YMail-OSG: KOjbIW4VM1k2yBaDc3lVRHxiHSVjCsMn.Z4cZuYDnL4r_XoFGTgJGYtSgW0LcQJ d_3IxWhHxf6bkl8v.zRPeXuvJMYWujxdWnglyxX2waiXytwGeth2_nszEFzDOuCjSNdmf_uM6s.S LaWPGwYAneWDd0edekd3SfjKUBR3qGzaVgO6UWxfrhjFrVv7YxaPULK.BDiIi1aSxBpaHYhZmTW5 .PcG0wnqd4WRi1MuXqsh8oNU2rl5Ox_aKN7s2GpnvbQo1H5FOPDsEAyGuo35ajGpQNK9_p.Nppc6 KOXsv9P3FEpWp.tzMXLpz6NHWTYCAMGHFJKF3EoFT3BOflEZ1zyxmdB7n.NMp8vNROdjHsyO_5Bt qmEb5M0las4Y14t2eDMD_fuUOq6viKlGlW1gttKbm.X5_K6odPRIw_hwcEhRMmWBg71l9GOi8Kjo EblLhcipQlQHADSBH3EYASLKUMVb2uY_YEsSW59VlUjL1y_KJstufTl3ZltMlLaGkvs7wq2AycfT BhWIwl1dIgcXYDJuUaTGpLmJ7aaeqyINk4sGnkqTTtTkY1ysNh.rbjiwdQxjLyNmcrH6EPuqK_A. njQbGd8vwZ3QNfveb9zyVrRZWsEhDGcxYiwbduZCwYMA_s1YgJ536iZWFPOlZvrPhlIPmGWRJl.u AHeQcLIs8695WEGEeb3KiWrNPb6k1ZQr9.0MoytM.pdYNBe9uwMspD6LkIpANCZlb5v3MjTFfBd3 nLD.O4SOkU4.FjDIEza7j5vJkv3t9M2KuPAgmZOX9EKU3bQYR80yap7Ai3E6qC70U7Gw.H44xWbG vNDX0Kg5u6wj4IFPgO3s.F7sgES5djp.Wxns93DhqWkwstwO89Qqwf3cXEY.PA7LiY2hmSPalFqa rHYgUmQGEdOobJg47V1r7bWkDkgVWNtztasxVaBcnnLRfSsGZQHcNzRO2dA7HlOOyFke51ZaIX.V kJ7NUQAMeeFpfsEvt8jFcZAhazQ.KfRMA3.QtRZe9i6..KYEkvTE3.D6cg7xFTD_MQxJeh_atBhY 8mocOy0eSY64HQahgWf_urBzPTnCKCulBDxmbMTfTycxN7tetCEhV3dOWOc4nHrkCgiyOblj8kUl SRYgtDQOC8naQn2Vc24TL9fDFE7uB3JDJJLAptXYVmiZmrkpRTZzezTI1_6Vdg9RtTeli9XbgA9q JnOXRX7OHZMV0qNQn4EOHOaILB8BFbVSoDArCgQk_gE5BV1Se0EK791SNM8ovRn7oqrOMxl3fIGn qHUXe9gwlGYfBMUznI7BBV5acD4qquRVf3EaKX7wHVXIE_W5tuLsdgvlx2HCMd7ro3bgqS3CEOYC RFZJAH7cicl047GNXeS5DsAwWTnej11mL1MJx.BASlM9o92foaJU1 Received: from sonic.gate.mail.ne1.yahoo.com by sonic309.consmr.mail.bf2.yahoo.com with HTTP; Sun, 31 May 2020 04:38:53 +0000 Date: Sun, 31 May 2020 04:38:38 +0000 (UTC) From: Allan Cady To: FFmpeg Development Discussions and Patches Message-ID: <1149006136.243626.1590899918044@mail.yahoo.com> MIME-Version: 1.0 References: <1149006136.243626.1590899918044.ref@mail.yahoo.com> X-Mailer: WebService/1.1.16037 YMailNorrin Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.122 Safari/537.36 X-Content-Filtered-By: Mailman/MimeDel 2.1.20 Subject: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" I prepared this patch a year ago but never followed through to get it accepted. Thought I would give it another try. I've been using a local copy with these changes, and for me it makes the difference between silencedetect being usable or not. Brief description: The existing implementation of the silencedetect filter uses the format specifier "%.6g", which keeps a fixed-width, and so loses precision as the time offsets get larger. I propose changing to "%.6f", which keeps six decimal digits precision no matter how large the offset. Further explanation in the patch comments. New patch attached, and here is the last correspondence I had with y'all last year. https://ffmpeg.org/pipermail/ffmpeg-devel/2019-April/242233.html Subject: [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files 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(-) 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