Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files
Greetings Martin et al, I've been trying to resubmit this patch based on your earlier suggestions. Most of the tools in the toolchain for this are new to me, so it's been awkward going. I did finally get the email sent with the new patch, but for some reason I haven't been able to figure out yet, it's going to the wrong thread. Here's the link to the message with the patch. https://ffmpeg.org/pipermail/ffmpeg-devel/2024-March/323170.html Looking forward to comments and to hopefully getting this approved and into the codebase. Allan On Friday, February 23, 2024 at 02:47:27 PM PST, Marton Balint wrote: On Fri, 23 Feb 2024, Allan Cady via ffmpeg-devel wrote: > [Apologies for the awful mess in the previous email. Trying again with > straight text.] > > On Thursday, February 22, 2024 at 01:16:19 AM PST, Marton Balint > wrote: > >>> For starters, I'm curious why there are two functions & macros: >>> >>> av_ts2str/av_ts_make_string (which used "%" format specifier) >> >> That takes a 64-bit integer timestamp and is actually using "%"PRId64 >> because that is the correct (portable) format string for an int64_t >> variable. >> >>> av_ts2timestr/av_ts_make_time_string (which used "%6g") >> >> That takes an integer timestamp and a rational time base. Float timestamps >> (in seconds) is calculated by multiplying the two, that is what is >> printed. >> >>> >>> Do you know the rationale for that? I see that only av_ts2timestr is used >>> in silencedetect.c. >>> >>> And are you suggesting we should fold those two functions into one? >> >> No, they have different purpose. The first prints out a timestamps which >> can be in any time base. The second prints out a timestamp in seconds. > > > Understood, I think I'm up to speed now. av_ts2str prints a 64-bit integer > timestamp; > av_ts2timestr prints a floating point timestamp in seconds with the timebase > applied. > > > In your previous email, you said: > > >> I'd rather just to fix av_ts_make_string to not limit the number of >> significant digits. Something like: >> >> 1) Print the number in decimal notation with at most 6 fractional digits. >> 2) Use less fractional digits if the first format would not fit into >> AV_TS_MAX_STRING_SIZE. >> 3) Use scientific notation if the second format would not fit into >> AV_TS_MAX_STRINT_SIZE. > > > I think you probably meant to say "I'd rather just to fix > av_ts_make_time_string" (not av_ts_make_string)? > Since it's av_ts_make_time_string() that's formatting floating point. Yes, indeed. > > So it makes sense to me to make the change to av_ts_make_time_string() > for all timestamps, as you suggest. As for how specifically to format them, > I'm fine with whatever you think is best, and I'm happy to work on this, but > the > implementation has me a bit stumped for the moment, and I may need some > help with it. My C language skills are rusty to say the least. The simplest way is to try snprintf() with 6 fractional digits, and check the return value to see how long the string would become. Based on this you can either accept snprintf result and truncate trailing zeros and dots, or try snprintf() with less fractional digits and truncate, or fall back to scientific format. > > It also occurs to me to wonder, would this warrant a formal problem ticket? > I haven't looked into how that works for ffmpeg. Opening a ticket for the problem is not required, but if your patch fixes an existing ticket, then you should mention the ticket number in the commit message. Regards, Marton ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [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 10 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. Trailing zeros can be trimmed from the fraction, without
losing precision. If the length of the fixed-precision formatted
timestamp exceeds the length of the allocated buffer
(AV_TS_MAX_STRING_SIZE, currently 32, less one for the
terminating null), then we can fall back to scientific notation, though
this seems almost certain to never occur, because 32 characters would
allow a maximum timestamp value of (32 - 1 - 6 - 1) = 24 characters.
By my calculation, 10^24 seconds is about six orders of magnitude
greater than the age of the universe.
The fix proposed here follows the following logic:
1) Try formatting the number of seconds using "%.6f". This will
always result in a string with six decimal digits in the fraction,
possibly including trailing zeros. (e.g. "897234.73200").
2) Check if that string would overflow the buffer. If it would, then
format it using scientific notation ("%.8g").
3) If the original fixed-point format fits, then trim any trailing
zeros and decimal point, and return that result.
Making this change broke two fate tests, filter-metadata-scdet,
and filter-metadata-silencedetect. To correct this, I've modified
tests/ref/fate/filter-metadata-scdet and
tests/ref/fate/filter-metadata-silencedetect to match the
new output.
Signed-off-by: Allan Cady
---
libavutil/timestamp.h| 53 +++-
tests/ref/fate/filter-metadata-scdet | 12 ++---
tests/ref/fate/filter-metadata-silencedetect | 2 +-
3 files changed, 58 insertions(+), 9 deletions(-)
diff --git a/libavutil/timestamp.h b/libavutil/timestamp.h
index 2b37781eba..2f04f9bb2b 100644
--- a/libavutil/timestamp.h
+++ b/libavutil/timestamp.h
@@ -25,6 +25,7 @@
#define AVUTIL_TIMESTAMP_H
#include "avutil.h"
+#include
#if defined(__cplusplus) && !defined(__STDC_FORMAT_MACROS) && !defined(PRId64)
#error missing -D__STDC_FORMAT_MACROS / #define __STDC_FORMAT_MACROS
@@ -53,6 +54,32 @@ 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)
+/**
+ * Strip trailing zeros and decimal point from a string. Performed
+ * in-place on input buffer. For local use only by av_ts_make_time_string.
+ *
+ * e.g.:
+ * "752.378000" -> "752.378"
+ *"4.0" -> "4"
+ * "97300" -> "97300"
+ */
+static inline void av_ts_strip_trailing_zeros_and_decimal_point(char *str) {
+if (strchr(str, '.'))
+{
+int i;
+for (i = strlen(str) - 1; i >= 0 && str[i] == '0'; i--) {
+str[i] = '\0';
+}
+
+// Remove decimal point if it's the last character
+if (i >= 0 && str[i] == '.') {
+str[i] = '\0';
+}
+
+// String was modified in place; no need for return value.
+}
+}
+
/**
* Fill the provided buffer with a string containing a timestamp time
* representation.
@@ -65,8 +92,30 @@ 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,
const 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);
+if (ts == AV_NOPTS_VALUE)
+{
+snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
+}
+else
+{
+const int max_fraction_digits = 6;
+
+// Convert 64-bit timestamp to double, using rational timebase
+double seconds = av_q2d(*tb) * ts;
+
+int length = snprintf(NULL, 0, "%.*f", max_fraction_digits, seconds);
+if (length <= AV_TS_MAX_STRING_SIZE - 1)
+{
+snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.*f", max_fraction_digits,
seconds);
+av_ts_strip_trailing_zeros_and_decimal_point(buf);
+}
+else
+{
+snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.8g", seconds);
+}
+
+}
+
return buf;
}
diff --git a/tests/ref/fate/filter-metadata-scdet
b/tests/ref/fate/filter-metadata-scdet
index ca5dbaaefc..d385920fcd 100644
--- a/tests/ref/fate/filter-metadata-scdet
+++ b/tests/ref/fate/filter-metadata-scdet
@@ -1,11 +1,11 @@
pts=1620|tag:lavfi.scd.score=59.252|tag:lavfi.scd.mafd=60.175|tag:lavfi.scd.time=2.7
pts=4140|tag:lavfi.scd.score=36.070|tag:lavfi.scd.mafd=44.209|tag:lavfi.scd.time=6.9
Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files
On Fri, 23 Feb 2024, Allan Cady via ffmpeg-devel wrote: [Apologies for the awful mess in the previous email. Trying again with straight text.] On Thursday, February 22, 2024 at 01:16:19 AM PST, Marton Balint wrote: For starters, I'm curious why there are two functions & macros: av_ts2str/av_ts_make_string (which used "%" format specifier) That takes a 64-bit integer timestamp and is actually using "%"PRId64 because that is the correct (portable) format string for an int64_t variable. av_ts2timestr/av_ts_make_time_string (which used "%6g") That takes an integer timestamp and a rational time base. Float timestamps (in seconds) is calculated by multiplying the two, that is what is printed. Do you know the rationale for that? I see that only av_ts2timestr is used in silencedetect.c. And are you suggesting we should fold those two functions into one? No, they have different purpose. The first prints out a timestamps which can be in any time base. The second prints out a timestamp in seconds. Understood, I think I'm up to speed now. av_ts2str prints a 64-bit integer timestamp; av_ts2timestr prints a floating point timestamp in seconds with the timebase applied. In your previous email, you said: I'd rather just to fix av_ts_make_string to not limit the number of significant digits. Something like: 1) Print the number in decimal notation with at most 6 fractional digits. 2) Use less fractional digits if the first format would not fit into AV_TS_MAX_STRING_SIZE. 3) Use scientific notation if the second format would not fit into AV_TS_MAX_STRINT_SIZE. I think you probably meant to say "I'd rather just to fix av_ts_make_time_string" (not av_ts_make_string)? Since it's av_ts_make_time_string() that's formatting floating point. Yes, indeed. So it makes sense to me to make the change to av_ts_make_time_string() for all timestamps, as you suggest. As for how specifically to format them, I'm fine with whatever you think is best, and I'm happy to work on this, but the implementation has me a bit stumped for the moment, and I may need some help with it. My C language skills are rusty to say the least. The simplest way is to try snprintf() with 6 fractional digits, and check the return value to see how long the string would become. Based on this you can either accept snprintf result and truncate trailing zeros and dots, or try snprintf() with less fractional digits and truncate, or fall back to scientific format. It also occurs to me to wonder, would this warrant a formal problem ticket? I haven't looked into how that works for ffmpeg. Opening a ticket for the problem is not required, but if your patch fixes an existing ticket, then you should mention the ticket number in the commit message. Regards, Marton ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files
[Apologies for the awful mess in the previous email. Trying again with straight text.] On Thursday, February 22, 2024 at 01:16:19 AM PST, Marton Balint wrote: >> For starters, I'm curious why there are two functions & macros: >> >> av_ts2str/av_ts_make_string (which used "%" format specifier) > > That takes a 64-bit integer timestamp and is actually using "%"PRId64 > because that is the correct (portable) format string for an int64_t > variable. > >> av_ts2timestr/av_ts_make_time_string (which used "%6g") > > That takes an integer timestamp and a rational time base. Float timestamps > (in seconds) is calculated by multiplying the two, that is what is > printed. > >> >> Do you know the rationale for that? I see that only av_ts2timestr is used in >> silencedetect.c. >> >> And are you suggesting we should fold those two functions into one? > > No, they have different purpose. The first prints out a timestamps which > can be in any time base. The second prints out a timestamp in seconds. Understood, I think I'm up to speed now. av_ts2str prints a 64-bit integer timestamp; av_ts2timestr prints a floating point timestamp in seconds with the timebase applied. In your previous email, you said: > I'd rather just to fix av_ts_make_string to not limit the number of > significant digits. Something like: > > 1) Print the number in decimal notation with at most 6 fractional digits. > 2) Use less fractional digits if the first format would not fit into > AV_TS_MAX_STRING_SIZE. > 3) Use scientific notation if the second format would not fit into > AV_TS_MAX_STRINT_SIZE. I think you probably meant to say "I'd rather just to fix av_ts_make_time_string" (not av_ts_make_string)? Since it's av_ts_make_time_string() that's formatting floating point. So it makes sense to me to make the change to av_ts_make_time_string() for all timestamps, as you suggest. As for how specifically to format them, I'm fine with whatever you think is best, and I'm happy to work on this, but the implementation has me a bit stumped for the moment, and I may need some help with it. My C language skills are rusty to say the least. It also occurs to me to wonder, would this warrant a formal problem ticket? I haven't looked into how that works for ffmpeg. Thanks for your help. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files
On Thursday, February 22, 2024 at 01:16:19 AM PST, Marton Balint wrote: >> For starters, I'm curious why there are two functions & macros: >> av_ts2str/av_ts_make_string (which used "%" format specifier)> > That takes >> a 64-bit integer timestamp and is actually using "%"PRId64 > because that is >> the correct (portable) format string for an int64_t > variable.> >> >> av_ts2timestr/av_ts_make_time_string (which used "%6g")> > That takes an >> integer timestamp and a rational time base. Float timestamps > (in seconds) >> is calculated by multiplying the two, that is what is > printed.> Do >> you know the rationale for that? I see that only av_ts2timestr is used in >> silencedetect.c. And are you suggesting we should fold those two >> functions into one?> > No, they have different purpose. The first prints out >> a timestamps which > can be in any time base. The second prints out a >> timestamp in seconds. Understood, I think I'm up to speed now. av_ts2str prints a 64-bit integer timestamp;av_ts2timestr prints a floating point timestamp in seconds with the timebase applied. In your previous email, you said: > I'd rather just to fix av_ts_make_string to not limit the number of > > significant digits. Something like:>> 1) Print the number in decimal notation > with at most 6 fractional digits. > 2) Use less fractional digits if the > first format would not fit into > AV_TS_MAX_STRING_SIZE.> 3) Use scientific > notation if the second format would not fit into > AV_TS_MAX_STRINT_SIZE. I think you probably meant to say "I'd rather just to fixav_ts_make_time_string" (not av_ts_make_string)?Since it's av_ts_make_time_string() that's formatting floating point. So it makes sense to me to make the change to av_ts_make_time_string()for all timestamps, as you suggest. As for how specifically to format them, I'm fine with whatever you think is best, and I'm happy to work on this, but theimplementation has me a bit stumped for the moment, and I may need somehelp with it. My C language skills are rusty to say the least. It also occurs to me to wonder, would this warrant a formal problem ticket? I haven't looked into how that works for ffmpeg. Thanks for your help. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files
On Thu, 22 Feb 2024, Allan Cady via ffmpeg-devel wrote: I had a similar thought, as all timestamps would have the same issue. This is my first contribution here, and I don't know the code very well, so I was being cautious. I'm open to expanding the scope, but I'm sure I would need some help doing it right and not breaking things. For starters, I'm curious why there are two functions & macros: av_ts2str/av_ts_make_string (which used "%" format specifier) That takes a 64-bit integer timestamp and is actually using "%"PRId64 because that is the correct (portable) format string for an int64_t variable. av_ts2timestr/av_ts_make_time_string (which used "%6g") That takes an integer timestamp and a rational time base. Float timestamps (in seconds) is calculated by multiplying the two, that is what is printed. Do you know the rationale for that? I see that only av_ts2timestr is used in silencedetect.c. And are you suggesting we should fold those two functions into one? No, they have different purpose. The first prints out a timestamps which can be in any time base. The second prints out a timestamp in seconds. I did notice something in the output from silencedetect. After I made my change, I see the output is now: frame:92404 pts:53224175 pts_time:2413.79 lavfi.silence_start=2411.120272 frame:92411 pts:53228207 pts_time:2413.98 lavfi.silence_end=2413.992744 lavfi.silence_duration=2.872472 I see that the pts_time values still have the original formatting. I don't know what pts_time is, or where those lines are coming from. Seems like maybe those should have fixed precision as well. Well, that is likely using the same function, but you only fixed silencedetect, right? Guidance for a noob please? Thanks. (P.S. Can you tell me, when I reply to the list (as opposed to patch submission using git send-email), how should I address the email? Obviously it should go to ffmpeg-devel@ffmpeg.org, but should I include you as a recipient, or as a cc:, or only to the list? or is there some other way it gets directed to you? Any other guidance on how to format email? Thanks.) I don't think there is a rule, I have seen it happen both with or without CC. You don't need to CC me though, as I am a regular on the list, but others may have other preference. Regards, Marton PS: Please avoid top positing in you replies. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files
I had a similar thought, as all timestamps would have the same issue.
This is my first contribution here, and I don't know the code very well, so I
was being cautious. I'm open to expanding the scope, but I'm sure I would need
some help doing it right and not breaking things.
For starters, I'm curious why there are two functions & macros:
av_ts2str/av_ts_make_string (which used "%" format specifier)
av_ts2timestr/av_ts_make_time_string (which used "%6g")
Do you know the rationale for that? I see that only av_ts2timestr is used in
silencedetect.c.
And are you suggesting we should fold those two functions into one?
I did notice something in the output from silencedetect. After I made my
change, I see the output is now:
frame:92404 pts:53224175 pts_time:2413.79
lavfi.silence_start=2411.120272
frame:92411 pts:53228207 pts_time:2413.98
lavfi.silence_end=2413.992744
lavfi.silence_duration=2.872472
I see that the pts_time values still have the original formatting. I don't know
what pts_time is, or where those lines are coming from. Seems like maybe those
should have fixed precision as well.
Guidance for a noob please? Thanks.
(P.S. Can you tell me, when I reply to the list (as opposed to patch submission
using git send-email), how should I address the email? Obviously it should go
to ffmpeg-devel@ffmpeg.org, but should I include you as a recipient, or as a
cc:, or only to the list? or is there some other way it gets directed to you?
Any other guidance on how to format email? Thanks.)
On Wednesday, February 21, 2024 at 12:25:23 PM PST, Marton Balint
wrote:
On Tue, 20 Feb 2024, Allan Cady via ffmpeg-devel wrote:
> When the silencedetect audio 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 format specifier ("%.6g" in
> libavutil/timestamp.h) limits the total field width to six significant
> digits. As the offset into the file increases, digits drop off the end,
> until eventually, for offsets greater than 10 seconds (about 28
> hours), fractions of a second disappear altogether, and the timestamps
> are logged as whole seconds.
>
> This patch changes the format to "%.6f" for silencedetect, which will
> give microsecond precision for all timestamps regardless of offset.
>
> libavutil/timestamp.h exposes a macro, av_ts2timestr, as the public
> interface. This macro was used by silencedetect.c, as well as other
> source files. In order to fix the issue for silencedetect without
> affecting other files and tests, I have added a new macro,
> av_ts2timestr_fixed_precision, which uses the new format specifier.
> The original av_ts_make_time_string remains, with the original
> behavior.
I'd rather just to fix av_ts_make_string to not limit the number of
significant digits. Something like:
1) Print the number in decimal notation with at most 6 fractional digits.
2) Use less fractional digits if the first format would not fit into
AV_TS_MAX_STRING_SIZE.
3) Use scientific notation if the second format would not fit into
AV_TS_MAX_STRINT_SIZE.
Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files
On Tue, 20 Feb 2024, Allan Cady via ffmpeg-devel wrote:
When the silencedetect audio 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 format specifier ("%.6g" in
libavutil/timestamp.h) limits the total field width to six significant
digits. As the offset into the file increases, digits drop off the end,
until eventually, for offsets greater than 10 seconds (about 28
hours), fractions of a second disappear altogether, and the timestamps
are logged as whole seconds.
This patch changes the format to "%.6f" for silencedetect, which will
give microsecond precision for all timestamps regardless of offset.
libavutil/timestamp.h exposes a macro, av_ts2timestr, as the public
interface. This macro was used by silencedetect.c, as well as other
source files. In order to fix the issue for silencedetect without
affecting other files and tests, I have added a new macro,
av_ts2timestr_fixed_precision, which uses the new format specifier.
The original av_ts_make_time_string remains, with the original
behavior.
I'd rather just to fix av_ts_make_string to not limit the number of
significant digits. Something like:
1) Print the number in decimal notation with at most 6 fractional digits.
2) Use less fractional digits if the first format would not fit into
AV_TS_MAX_STRING_SIZE.
3) Use scientific notation if the second format would not fit into
AV_TS_MAX_STRINT_SIZE.
Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files
When the silencedetect audio 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 format specifier ("%.6g" in
libavutil/timestamp.h) limits the total field width to six significant
digits. As the offset into the file increases, digits drop off the end,
until eventually, for offsets greater than 10 seconds (about 28
hours), fractions of a second disappear altogether, and the timestamps
are logged as whole seconds.
This patch changes the format to "%.6f" for silencedetect, which will
give microsecond precision for all timestamps regardless of offset.
libavutil/timestamp.h exposes a macro, av_ts2timestr, as the public
interface. This macro was used by silencedetect.c, as well as other
source files. In order to fix the issue for silencedetect without
affecting other files and tests, I have added a new macro,
av_ts2timestr_fixed_precision, which uses the new format specifier.
The original av_ts_make_time_string remains, with the original
behavior.
timestamp.h also exposes a function, av_ts_make_time_string, which
was called only from av_ts2timestr. After this patch, both of the
macros now use a new function, av_ts_make_time_string_format, which
takes a format specifier as an argument, so the function
av_ts_make_time_string is no longer used. I've left it in place, but
flagged it for deprecation with FF_API_AV_MAKETIMESTRING.
The test reference file filter-metadata-silencedetect has been updated
to match the new functionality.
Signed-off-by: Allan Cady
---
libavfilter/af_silencedetect.c | 12 +++
libavutil/timestamp.h| 34 +---
libavutil/version.h | 1 +
tests/ref/fate/filter-metadata-silencedetect | 2 +-
4 files changed, 38 insertions(+), 11 deletions(-)
diff --git a/libavfilter/af_silencedetect.c b/libavfilter/af_silencedetect.c
index 845c65bfed..f1a8096540 100644
--- a/libavfilter/af_silencedetect.c
+++ b/libavfilter/af_silencedetect.c
@@ -86,11 +86,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], _base));
+av_ts2timestr_fixed_precision(s->start[channel],
_base));
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], _base));
+av_ts2timestr_fixed_precision(s->start[channel],
_base));
}
}
} else {
@@ -101,15 +101,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, _base));
+av_ts2timestr_fixed_precision(end_pts, _base));
set_meta(insamples, s->mono ? channel + 1 : 0,
"silence_duration",
-av_ts2timestr(duration_ts, _base));
+av_ts2timestr_fixed_precision(duration_ts,
_base));
}
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, _base),
-av_ts2timestr(duration_ts, _base));
+av_ts2timestr_fixed_precision(end_pts, _base),
+av_ts2timestr_fixed_precision(duration_ts, _base));
}
s->nb_null_samples[channel] = 0;
s->start[channel] = INT64_MIN;
diff --git a/libavutil/timestamp.h b/libavutil/timestamp.h
index 9ae64da8a1..b483b5e12d 100644
--- a/libavutil/timestamp.h
+++ b/libavutil/timestamp.h
@@ -31,6 +31,8 @@
#endif
#define AV_TS_MAX_STRING_SIZE 32
+#define AV_TS_FMT_DEFAULT "%.6g"
+#define AV_TS_FMT_FIXED_PRECISION_6 "%.6f"
/**
* Fill the provided buffer with a string containing a timestamp
@@ -53,9 +55,14 @@ 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)
+#if FF_API_AV_MAKETIMESTRING
/**
+ * This function is probably deprecated. It was originally called by
+ * av_ts_make_time_string defined below, which now uses
av_ts_make_time_string_format.
+ * instead. Nothing external references this function directly.
+ *
* Fill the provided buffer with a string
[FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps
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
From 88866601cf6c6931846636fc5fee25dbbfe5ce52 Mon Sep 17 00:00:00 2001
From: Allan Cady
Date: Sat, 30 May 2020 21:06:45 -0700
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 10 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], _base));
+av_ts2timestr_fmt(s->start[channel], _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], _base));
+av_ts2timestr_fmt(s->start[channel], _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, _base));
+av_ts2timestr_fmt(end_pts, _base, TS_FMT_SILENCEDETECT));
set_meta(insamples, s->mono ? channel + 1 : 0, "silence_duration",
-av_ts2timestr(duration_ts, _base));
+av_ts2timestr_fmt(duration_ts, _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, _base),
-av_ts2timestr(duration_ts, _base));
+av_ts2timestr_fmt(end_pts, _base, TS_FMT_SILENCEDETECT),
+av_ts2timestr_fmt(duration_ts, _base, TS_FMT_SILENCEDETECT));
}
s->nb_null_samples[channel] = 0;
Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files
On 4/7/2019 4:47 AM, Allan Cady via ffmpeg-devel wrote:
> [Second try submitting to the list. This patch now passes fate.]
>
> 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 10 seconds (about 28
> hours), fractions of a second disappear altogether, and the
> timestamps are logged as whole seconds. This is insufficient
> precision for my purposes. I propose changing the format to "%.6f",
> which will give microsecond precision (probably overkill but safe)
> 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.
>
> 0001-libavutil-timestamp.h-Fix-loss-of-precision-in-times.patch
>
> From 5492506534bf863cbf1ee8f09a5e59b4ee111226 Mon Sep 17 00:00:00 2001
> From: Allan Cady
> Date: Sun, 7 Apr 2019 00:07:58 -0700
> 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 10 seconds (about 28
> hours), fractions of a second disappear altogether, and the
> timestamps are logged as whole seconds. This is insufficient
> precision for my purposes. I propose changing the format to "%.6f",
> which will give microsecond precision (probably overkill but safe)
> 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| 13 -
> tests/ref/fate/filter-metadata-silencedetect | 2 +-
> 3 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/libavfilter/af_silencedetect.c b/libavfilter/af_silencedetect.c
> index 3a71f39..2da8dbe 100644
> --- a/libavfilter/af_silencedetect.c
> +++ b/libavfilter/af_silencedetect.c
> @@ -32,6 +32,8 @@
> #include "avfilter.h"
> #include "internal.h"
>
> +const char TIMESTAMP_FMT_SILENCEDETECT[] = "%.6f";
> +
> typedef struct SilenceDetectContext {
> const AVClass *class;
> double noise; ///< noise amplitude ratio
> @@ -86,11 +88,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], _base));
> +av_ts2timestr_fmt(s->start[channel], _base,
> TIMESTAMP_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], _base));
> +av_ts2timestr_fmt(s->start[channel], _base,
> TIMESTAMP_FMT_SILENCEDETECT));
> }
> }
> } else {
> @@ -101,15 +103,15 @@ static av_always_inline void
> update(SilenceDetectContext *s, AVFrame *insamples,
> int64_t duration_ts =
[FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files
[Second try submitting to the list. This patch now passes fate.]
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 10 seconds (about 28
hours), fractions of a second disappear altogether, and the
timestamps are logged as whole seconds. This is insufficient
precision for my purposes. I propose changing the format to "%.6f",
which will give microsecond precision (probably overkill but safe)
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.
From 5492506534bf863cbf1ee8f09a5e59b4ee111226 Mon Sep 17 00:00:00 2001
From: Allan Cady
Date: Sun, 7 Apr 2019 00:07:58 -0700
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 10 seconds (about 28
hours), fractions of a second disappear altogether, and the
timestamps are logged as whole seconds. This is insufficient
precision for my purposes. I propose changing the format to "%.6f",
which will give microsecond precision (probably overkill but safe)
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| 13 -
tests/ref/fate/filter-metadata-silencedetect | 2 +-
3 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/libavfilter/af_silencedetect.c b/libavfilter/af_silencedetect.c
index 3a71f39..2da8dbe 100644
--- a/libavfilter/af_silencedetect.c
+++ b/libavfilter/af_silencedetect.c
@@ -32,6 +32,8 @@
#include "avfilter.h"
#include "internal.h"
+const char TIMESTAMP_FMT_SILENCEDETECT[] = "%.6f";
+
typedef struct SilenceDetectContext {
const AVClass *class;
double noise; ///< noise amplitude ratio
@@ -86,11 +88,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], _base));
+av_ts2timestr_fmt(s->start[channel], _base,
TIMESTAMP_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], _base));
+av_ts2timestr_fmt(s->start[channel], _base,
TIMESTAMP_FMT_SILENCEDETECT));
}
}
} else {
@@ -101,15 +103,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, _base));
+av_ts2timestr_fmt(end_pts, _base,
TIMESTAMP_FMT_SILENCEDETECT));
