[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_ts2timest
[FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in silencedetect timestamps for long files
[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 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;
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
[FFmpeg-devel] Patch request to timestamp.h for silencedetect filter
Noob here. I would like to suggest a simple change to libavutil\timestamp.h,
that for me makes the difference between silencedetect being very useful, or
not useful at all. If there is a better way to submit this, I'm happy to jump
through proper hoops... this seems like a good place to start the process.
REQUESTED PATCH
The requested patch is to the function av_ts_make_time_string in
ffmpeg_sources\ffmpeg\libavutil\timestamp.h:
Compare: (<) ffmpeg_sources\ffmpeg\libavutil\timestamp.h.orig (2617 bytes)
with: (>) ffmpeg_sources\ffmpeg\libavutil\timestamp.h (2617 bytes)
68c68
< else snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.6g",
av_q2d(*tb) * ts);
---
> else snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.3f",
>av_q2d(*tb) * ts);
The original function:
/**
* Fill the provided buffer with a string containing a timestamp time
* representation.
*
* @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
* @return the buffer in input
*/
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);
return buf;
}
PROBLEM DESCRIPTION
The problem is with the format of the timestamp output, which is hardcoded in
timestamp.h. The issue has been, for long audio files, as ffmpeg scans further
into the file, the magnitude of the time offsets grows, and the output
gradually loses decimal places off the right end, until eventually it's in
whole seconds, which is not enough precision to be useful for me. Current code
formats the output as "%.6g". Changing it to either "%.3f" or just "%f",
ensures at least millisecond precision, which would serve my purposes.
EXAMPLE
For a sample scan on a file that's about 35 hours (126000 seconds) length:
$ /usr/bin/ffmpeg -i input.mp3 -filter_complex
silencedetect=n=-30dB:d=3.5,ametadata=mode=print:file=silence-out.txt -f null -
NOTE: One might think, 35 hours is an unusually long audio file. That's true,
but in fact it's the need to work with long files that brought me to ffmpeg for
this. For smaller files, I often use Audacity, which works fine, but it's
problematic to have to load entire large files into an Audacity project, so I
went looking for a command line option. So anyway...
The output goes from this near the beginning:
frame:83085 pts:47856431 pts_time:2170.36
lavfi.silence_start=2166.86
frame:83139 pts:47887535 pts_time:2171.77
lavfi.silence_end=2171.77
lavfi.silence_duration=4.91061
to this somewhere in the middle:
frame:2450348 pts:1411399919 pts_time:64009.1
lavfi.silence_start=64005.6
frame:2450371 pts:1411413167 pts_time:64009.7
lavfi.silence_end=64009.7
lavfi.silence_duration=4.10082
to eventually this after it passes 10 seconds:
frame:4738029 pts:2729104175 pts_time:123769
lavfi.silence_start=123765
frame:4738055 pts:2729119151 pts_time:123770
lavfi.silence_end=123770
lavfi.silence_duration=4.17918
As you can see, the start and end are eventually limited to whole-number
precision, which is not sufficient for my needs. And it doesn't make sense
logically to reduce precision for large numbers anyway; I can't think of a use
where this would be desirable.
After making the above patch and building the code myself, I now get this
output near the end:
frame:4738029 pts:2729104175 pts_time:123768.897
lavfi.silence_start=123765.411
frame:4738055 pts:2729119151 pts_time:123769.576
lavfi.silence_end=123769.584
lavfi.silence_duration=4.173
This gives me the output I want.
I haven't attempted to find if this routine is used elsewhere or if it might
have undesirable results. If there is further effort you would like me to make
before you will consider the change, please let me know and I can see what I
can do.
Thank you,
Allan Cady
Seattle WA
___
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] Is it possible to run just a single FATE test?
On 4/4/2019 5:52 PM, Carl Eugen Hoyos wrote: 2019-04-05 2:45 GMT+02:00, Allan Cady via ffmpeg-devel Try for example: $ make SAMPLES=fate-suite fate-xtea $ make SAMPLES=fate-suite fate-h264 $ make fate-acodec-flac $ make fate-vsynth1 and also $ make SAMPLES=fate-suite fate-list Excellent, that works. Thank you. ___ 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] Is it possible to run just a single FATE test?
Is it possible to run just a single FATE test against local code while doing development, rather than running the entire test suite, to speed up the iteration cycle? I expect it is, but I don't see how explained in the documentation. Would appreciate a clue if this is possible. Thanks. ___ 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] Fix loss of precision for silencedetect on large files
Trying a desktop mail client instead of webmail. On 3/27/2019 11:19 AM, Michael Niedermayer wrote: this breaks make fate also if fate is updated it should be ensured it still checks enough precission and that it does produce the same results (fate passes) on all relevant platforms. This change may bring non significant differences into significance thanks [...] OK so I spent some more time familiarizing myself with the code and tests, and it's clear that making the change the way I did would probably break dozens of tests, because the function I changed, av_ts_make_time_string, is referenced (by its convenience macro av_ts2timestr) in many files (I count 21) and dozens of lines of code, in libavfilter, fftools, and libavformat. As an alternative, I tried making a modified & renamed copy of av_ts_make_time_string/av_ts2timestr, and referencing that copy in af_silencedetect.c, leaving other references to the original function unchanged. In this case, FATE fails just a single test, filter-metadata-silencedetect. I then modified the reference output file (tests/ref/fate/filter-metadata-silencedetect) to match the changed output, and all tests then pass. In looking at other uses of av_ts2timestr, it's clear av_ts2timestr was intended for general purpose use. So at this point I could use some guidance from the wizards as to the best way to proceed. Not knowing any of the history or thinking that went into the design, I don't know if it makes the best sense to use the "%.6g" format for all those other timestamps, but I feel strongly that it's a poor choice for silencedetect (and probably other *detect filters), for the reason I explained originally. Is there possibly another way to get the result I want without modifying the code? I can imagine maybe a format specifier as a filter parameter -- is there any precedent for this? Open to suggestions. Thanks all. Allan ___ 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] Fix loss of precision for silencedetect on large files
When the silencedetect filter is run against very large 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 integers. This is insufficient precision for my purposes. I propose changing the format to "%.3f", which will give millisecond precision for all timestamps regardless of offset. Patch file is attached. EXAMPLE For a sample scan on a file that's about 35 hours (126000 seconds) length: $ ffmpeg -i input.mp3 -filter_complex silencedetect=n=-30dB:d=3,ametadata=mode=print:file=silence-out.txt -f null - The output looks like this near the beginning: frame:83085 pts:47856431 pts_time:2170.36 lavfi.silence_start=2166.86 frame:83139 pts:47887535 pts_time:2171.77 lavfi.silence_end=2171.77 lavfi.silence_duration=4.91061 Further on we get this: frame:2450348 pts:1411399919 pts_time:64009.1 lavfi.silence_start=64005.6 frame:2450371 pts:1411413167 pts_time:64009.7 lavfi.silence_end=64009.7 lavfi.silence_duration=4.10082 Eventually after it passes 10 seconds: frame:4738029 pts:2729104175 pts_time:123769 lavfi.silence_start=123765 frame:4738055 pts:2729119151 pts_time:123770 lavfi.silence_end=123770 lavfi.silence_duration=4.17918 The start and end times are now in whole integers (seconds). After making this patch and building the code myself, I now get this output near the end: frame:4738029 pts:2729104175 pts_time:123768.897 lavfi.silence_start=123765.411 frame:4738055 pts:2729119151 pts_time:123769.576 lavfi.silence_end=123769.584 lavfi.silence_duration=4.173 This gives me the output I want. Thank you, Allan Cady Seattle WA 0001-Fix-loss-of-precision-for-silencedetect-on-large-fil.patch Description: Binary data ___ 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] Fix loss of precision for silencedetecton large files
On Tue, Mar 26, 2019 at 10:07:10PM +, Allan Cady via ffmpeg-devel wrote: > When the silencedetect filter is run against very large 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 integers. This is insufficient precision for > my purposes. > > I propose changing the format to "%.3f", which will give millisecond > precision for all timestamps regardless of offset. > > Patch file is attached. > > EXAMPLE > > For a sample scan on a file that's about 35 hours (126000 seconds) length: > > $ ffmpeg -i input.mp3 -filter_complex > silencedetect=n=-30dB:d=3,ametadata=mode=print:file=silence-out.txt -f null - > > The output looks like this near the beginning: > > frame:83085 pts:47856431 pts_time:2170.36 > lavfi.silence_start=2166.86 > frame:83139 pts:47887535 pts_time:2171.77 > lavfi.silence_end=2171.77 > lavfi.silence_duration=4.91061 > > Further on we get this: > > frame:2450348 pts:1411399919 pts_time:64009.1 > lavfi.silence_start=64005.6 > frame:2450371 pts:1411413167 pts_time:64009.7 > lavfi.silence_end=64009.7 > lavfi.silence_duration=4.10082 > > Eventually after it passes 10 seconds: > > frame:4738029 pts:2729104175 pts_time:123769 > lavfi.silence_start=123765 > frame:4738055 pts:2729119151 pts_time:123770 > lavfi.silence_end=123770 > lavfi.silence_duration=4.17918 > > The start and end times are now in whole integers (seconds). > > After making this patch and building the code myself, I now get this > output near the end: > > frame:4738029 pts:2729104175 pts_time:123768.897 > lavfi.silence_start=123765.411 > frame:4738055 pts:2729119151 pts_time:123769.576 > lavfi.silence_end=123769.584 > lavfi.silence_duration=4.173 > > This gives me the output I want. > > Thank you, > > Allan Cady > Seattle WA > timestamp.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > a997e2f7d02d14a3761cf389b8096e55f3670fb5 > 0001-Fix-loss-of-precision-for-silencedetect-on-large-fil.patch > From 59b82d49516926173ab03944a73fd7fc9e5d7bcc Mon Sep 17 00:00:00 2001 > From: Allan Cady > Date: Tue, 26 Mar 2019 14:11:03 -0700 > Subject: [PATCH] Fix loss of precision for silencedetect on large files On Wednesday, March 27, 2019, 11:19:23 AM PDT, Michael Niedermayer wrote: this breaks make fate also if fate is updated it should be ensured it still checks enough precission and that it does produce the same results (fate passes) on all relevant platforms. This change may bring non significant differences into significance thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Thank you Michael for moving this forward. I've been able to reproduce the failure for myself and I'm starting to look into how big a deal it will be. First glance it looks like there are pre-built reference output files in the tests that may need to be re-generated, and that may be all that's required. I'll continue digging to try to understand what's going on. I'm brand new here and not very experienced with open source collaboration... any hints how to proceed or corrections if I break protocol would be appreciated. So far so good though it seems. ___ 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] Fix loss of precision for silencedetecton large files
On Wednesday, March 27, 2019, 11:19:23 AM PDT, Michael Niedermayer wrote: > this breaks make fate> > also if fate is updated it should be ensured it still checks enough precission> and that it does produce the same results (fate passes) on all relevant> platforms. This change may bring non significant differences into significance> > thanks OK so I spent some more time familiarizing myself with the code and tests, and it's clear that making the change the way I did would probably break dozens of tests, because the function I changed, av_ts_make_time_string, is referenced (by its convenience macro av_ts2timestr) in many files (I count 21) and dozens of lines of code, in libavfilter, fftools, and libavformat. As an alternative, I tried making a modified & renamed copy of av_ts_make_time_string/av_ts2timestr, and referencing that copy in af_silencedetect.c, leaving other references to the original function unchanged. In this case, FATE fails just a single test, filter-metadata-silencedetect. I then modified the reference output file (tests/ref/fate/filter-metadata-silencedetect) to match the changed output, and all tests then pass. So at this point I could use some guidance from the wizards as to the best way to proceed. Not knowing any of the history or thinking that went into the design, I don't know if it makes the best sense to use the "%.6g" format for all those other timestamps, but I feel strongly that it's a poor choice for silencedetect (and probably other *detect filters), for the reason I explained originally. Is there possibly another way to get the result I want without modifying the code? Or I can imagine maybe a format specifier as a filter parameter -- is there any precedent for this? Open to suggestions. Thanks all. Allan ___ 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] Fix loss of precision for silencedetect on large files
Gck! Sorry guys about the formatting. Yahoo Mail sucks for anything slightly out of the norm... Let's try this again... if it doesn't work better, not sure what to do: On Wednesday, March 27, 2019, 11:19:23 AM PDT, Michael Niedermayer wrote: > this breaks make fate> > also if fate is updated it should be ensured it still checks enough precission> and that it does produce the same results (fate passes) on all relevant> platforms. This change may bring non significant differences into significance> > thanks OK so I spent some more time familiarizing myself with the code and tests, and it's clear that making the change the way I did would probably break dozens of tests, because the function I changed, av_ts_make_time_string, is referenced (by its convenience macro av_ts2timestr) in many files (I count 21) and dozens of lines of code, in libavfilter, fftools, and libavformat. As an alternative, I tried making a modified & renamed copy of av_ts_make_time_string/av_ts2timestr, and referencing that copy in af_silencedetect.c, leaving other references to the original function unchanged. In this case, FATE fails just a single test, filter-metadata-silencedetect. I then modified the reference output file (tests/ref/fate/filter-metadata-silencedetect) to match the changed output, and all tests then pass. In looking at other uses of av_ts2timestr, it's clear av_ts2timestr was intended for general purpose use. So at this point I could use some guidance from the wizards as to the best way to proceed. Not knowing any of the history or thinking that went into the design, I don't know if it makes the best sense to use the "%.6g" format for all those other timestamps, but I feel strongly that it's a poor choice for silencedetect (and probably other *detect filters), for the reason I explained originally. Is there possibly another way to get the result I want without modifying the code? I can imagine maybe a format specifier as a filter parameter -- is there any precedent for this? Open to suggestions. Thanks all. Allan ___ 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
[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_
[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 b
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
[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] [libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files]
> On Monday, March 11, 2024 at 12:50:11 PM PDT, wrote:
> On 11 Mar 2024, at 15:26, Andreas Rheinhardt wrote:
>> Andreas Rheinhardt:
>>> Allan Cady via ffmpeg-devel:
>>>> From: "Allan Cady"
>>>>
>>>> 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)
>>>>
Re: [FFmpeg-devel] [PATCH] change av_ts_make_time_string precision
On Monday, March 11, 2024 at 12:11:45 PM PDT, Marton Balint
wrote:
> On Mon, 11 Mar 2024, Andreas Rheinhardt wrote:
> Allan Cady via ffmpeg-devel:
>> From: "Allan Cady"
>>
>> 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;
>> +
>> +
[FFmpeg-devel] Relative paths vs. filenames in #includes for project files.
To test the patch I've been working on, I wrote a small standalone C program, which I had saved in the project root. The file I'm patching is libavutil/timestamp.h. At first I had duplicated a bunch of definitions out of other include files (e.g. struct AVRational, in libavutil/rational.h) so I could run my test program in isolation, but then I decided to see if I could make it work by including the project headers, especially by including libavutil/timestamp.h. But when I tried to compile and run it that way (I'm using Visual Studio Code with the Code Runner Extension), I got a bunch of errors that it couldn't find several files that timestamp.h depends on. I eventually figured out this was because some of the #includes in various files in libavutil are like this: #include "avutil.h" and others are like this: #include "libavutil/avconfig.h" In both of those cases, both the including and included files are in libavutil. I changed the #includes in two files, by removing the "libavutil/", and then my program compiled. I also tested making the whole project, and it still compiled and ran fine as well. So my question is, would it make sense to remove the path from as many of those #includes as possible, so they are filename-only? Given my limited experience, it seems like it might not be a problem, but I don't understand fully how the project manages source dependencies. Just thought I'd toss this out for comment. ___ 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] 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
From: "Allan Cady"
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
-pts=5800|tag:lavfi.scd.score=55.819|tag:lavfi.scd.mafd=55.819|tag:lavfi.scd.time=9.7
+pts=5800|tag:lavfi.scd.score=55.819|tag:lavfi.scd.mafd=55.819|tag:lavfi.scd.time=9.67
pts=6720|tag:lavfi.scd.score=18.5
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-scde
Re: [FFmpeg-devel] [PATCH] change av_ts_make_time_string precision
I've been meaning to ask -- what version of C are we using? I know it's at least 99, because of the compound literal (had to look that up). ___ 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] change av_ts_make_time_string precision
On Tuesday, March 12, 2024 at 02:24:47 PM PDT, Marton Balint
wrote:
> On Tue, 12 Mar 2024, Allan Cady via ffmpeg-devel wrote:
>> On Monday, March 11, 2024 at 12:11:45 PM PDT, Marton Balint
>> wrote:
>>> On Mon, 11 Mar 2024, Andreas Rheinhardt wrote:
>>> Allan Cady via ffmpeg-devel:
>>>> From: "Allan Cady"
>>>>
>>>> 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
>>
Re: [FFmpeg-devel] [PATCH] change av_ts_make_time_string precision
On Sunday, March 17, 2024 at 04:43:31 PM PDT, Marton Balint
wrote:
> On Wed, 13 Mar 2024, Allan Cady via ffmpeg-devel wrote:>> On Tuesday, March
> 12, 2024 at 02:24:47 PM PDT, Marton Balint wrote:>>> On Tue,
> 12 Mar 2024, Allan Cady via ffmpeg-devel wrote:>>>> On Monday, March 11, 2024
> at 12:11:45 PM PDT, Marton Balint wrote:>>>>> On Mon, 11 Mar
> 2024, Andreas Rheinhardt wrote:>>>>> Allan Cady via ffmpeg-devel:>>>>>> From:
> "Allan Cady" >>>>>>> > [...]> >>>>>>> One thing to
> notice is that you will not need to use the scientific>>> representation at
> all, because maximum value this function prints is the>>> product of an INT32
> and an INT64, and that is 96 bit, which is at most 29>>> chars. Adding the
> optional sign and the decimal point, that is still only>>> 31. So we can be
> sure that by using %.6f, the first character of>>> the decimal point is going
> to be present in the output.>>>>>> I had done some similar calculation and
> came to a similar conclusion. >>>>>> Which is great,>>> because that means we
> only have to>>> - do a single snprintf("%.6f")>>> - calculate last char
> position by subtracting 1 from the minimum of>>> AV_TS_MAX_STRING_SIZE-1 and
> the result of the snprintf() call.>>> - decrement string length while last
> char is '0' to remove trailing 0s>>> - decrement string length while last
> char is non-digit to remove decimal>>> point (which can be a multiple chars
> for some locales).>>> - update last+1 char to \0.>>> Ot is it still too
> complex to keep it inline?>>>>>> I'll give your suggestion a spin tomorrow.
> Thanks.>>> > In the end I posted a patch myself, sorry if you were working on
> it too,> it just looked a bit too complex for a new developer, since it
> touched> API/build system/etc... I hope you don't mind.> > Regards,> > Marton
No problem at all. I was just getting ready to resubmit, but I'm happyto see
you beat me to the punch. And I'm delighted that I won't need tokeep dragging
around my own customized copy of ffmpeg going forward.
Any idea how soon this will show up on the master branch?
___
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] Maintaining email threads for patch submissions using git send-email
Could someone please have a look at an issue I'm having in resubmitting a patch, trying to get the resubmission email to appear as a reply on an existing thread? In order to conform to submission guidelines in ffmpeg-devel, I'm using git format-patch and git send-email, using the --in-reply-to option, but it's not giving the expected results. I had started a thread in this mailing list for a patch I've been working on. I had gotten some comments to my initial submission, worked on addressing them, and last night I submitted it again. I wanted the resubmission to appear as a reply on the earlier thread, to preserve that conversation. After configuring git to use the same email account that I had used for the earlier submissions and comments, I used the following git command: git send-email --to=ffmpeg-devel@ffmpeg.org --in-reply-to="e4d3f14f-ac69-9424-804e-ee5025059...@passwd.hu" I got the message ID from the following line in the message I wanted to reply to: Message-ID: But when I submitted the patch, it showed up in the list as a reply on a completely different, unrelated thread. So I came back and fixed a couple other issues in the patch (I had messed up the subject line), double-checked that I had copied the Message-ID, and entered this command (adding angle brackets this time to the ID, just in case that might matter). git send-email --to=ffmpeg-devel@ffmpeg.org --in-reply-to="" Still, the message went to the same incorrect thread. Is there possibly something else I need to add to the send-email command to get it to attach to the older thread? Thanks, Allan ___ 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 v2 1/2] avutil/timestamp: introduce av_ts_make_time_string2 for better precision
> On Sat, 23 Mar 2024, Marton Balint wrote: >> av_ts_make_time_string() used "%.6g" format, but this format was losing >> precision even when the timestamp to be printed was not that large. For >> example >> for 3 hours (10800) seconds, only 1 decimal digit was printed, which made >> this >> format inaccurate when it was used in e.g. the silencedetect filter. Other >> detection filters printing timestamps had similar issues. Also time base >> parameter of the function was *AVRational instead of AVRational. >> >> Resolve these problems by introducing a new function, >> av_ts_make_time_string2(). >> >> We change the used format to "%.*f", use a precision of 6, except when >> printing >> values near 0, in which case we calculate the precision dynamically to aim >> for >> a similar precision in normal form as with %.6g. No longer using scientific >> representation can make parsing the timestamp easier for the users, we can >> safely do this because the theoretical maximum of INT64_MAX*INT32_MAX still >> fits into the string buffer in normal form. >> >> We somewhat imitate %g by trimming ending zeroes and the potential decimal >> point characters. In order not to trim "inf" as well, we assume that the >> decimal point string does not contain the letter "f". Note that depending on >> printf %f implementation, we might trim "infinity" to "inf". >> >> Thanks for Allan Cady for bringing up this issue. > Will apply the series, so it can make it into 7.0. > Regards, > Marton This looks great. I ran the patch against my test program with a couple dozen vectors of inputs, and it looks perfect to me. Thanks to both of you Marton and Andreas for seeing this through. I really appreciate it. Allan >> >> Signed-off-by: Marton Balint >> --- >> doc/APIchanges | 5 + >> libavutil/Makefile | 1 + >> libavutil/timestamp.c | 36 >> libavutil/timestamp.h | 8 >> libavutil/version.h | 2 +- >> 5 files changed, 51 insertions(+), 1 deletion(-) >> create mode 100644 libavutil/timestamp.c >> >> diff --git a/doc/APIchanges b/doc/APIchanges >> index 2796b4d0c2..9c6fa381e1 100644 >> --- a/doc/APIchanges >> +++ b/doc/APIchanges >> @@ -2,6 +2,11 @@ The last version increases of all libraries were on >> 2024-03-07 >> >> API changes, most recent first: >> >> +2024-03-xx - xx - lavu 59.5.100 - timestamp.h >> + Add av_ts_make_time_string2() for better timestamp precision, the new >> + function accepts AVRational as time base instead of *AVRational, and is >> not >> + an inline function like its predecessor. >> + >> 2024-03-xx - xx - lavc 61.3.100 - jni.h >> Add av_jni_set_android_app_ctx() and av_jni_get_android_app_ctx(). >> >> diff --git a/libavutil/Makefile b/libavutil/Makefile >> index 008fcfcd9c..6e6fa8d800 100644 >> --- a/libavutil/Makefile >> +++ b/libavutil/Makefile >> @@ -174,6 +174,7 @@ OBJS = adler32.o >> \ >> threadmessage.o \ >> time.o \ >> timecode.o \ >> + timestamp.o \ >> tree.o \ >> twofish.o \ >> utils.o \ >> diff --git a/libavutil/timestamp.c b/libavutil/timestamp.c >> new file mode 100644 >> index 00..2a3e3012a4 >> --- /dev/null >> +++ b/libavutil/timestamp.c >> @@ -0,0 +1,36 @@ >> +/* >> + * This file is part of FFmpeg. >> + * >> + * FFmpeg is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * FFmpeg is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Lesser General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >>
