| Message ID | 20240311025607.3468624-1-allancady@yahoo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] 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 t | expand |
| Context | Check | Description |
|---|---|---|
| andriy/commit_msg_x86 | warning | The first line of the commit message is way too long. |
| yinshiyou/commit_msg_loongarch64 | warning | The first line of the commit message is way too long. |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Allan Cady via ffmpeg-devel: > From: "Allan Cady" <allancady@yahoo.com> > > 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 <allancady@yahoo.com> > --- > 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 <stdbool.h> > > #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; > } > 1. What makes you believe that all users want the new format and that it does not cause undesired behaviour for some (maybe a lot) of them? The number of characters written by the earlier code stayed pretty constant even when the times became big (in this case, it just printed 8 chars if ts>=0), yet your code will really make use of the whole buffer. Granted, we could tell our users that they have no right to complain about this, given that we always had a "right" to use the full buffer, but I consider this a violation of the principle of least surprise. Why don't you just change silencedetect or add another function? 2. For very small timestamps (< 10^-4), the new code will print a lot of useless leading zeros (after the decimal point). In fact, it can be so many that the new code has less precision than the old code, despite using the fill buffer. 2. This is way too much code for an inline function. 3. Anyway, your placement of {} on their own lines does not match the project coding style. - Andreas
Andreas Rheinhardt: > Allan Cady via ffmpeg-devel: >> From: "Allan Cady" <allancady@yahoo.com> >> >> 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 <allancady@yahoo.com> >> --- >> 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 <stdbool.h> >> >> #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; >> } >> > > 1. What makes you believe that all users want the new format and that it > does not cause undesired behaviour for some (maybe a lot) of them? The > number of characters written by the earlier code stayed pretty constant > even when the times became big (in this case, it just printed 8 chars if > ts>=0), yet your code will really make use of the whole buffer. > Granted, we could tell our users that they have no right to complain > about this, given that we always had a "right" to use the full buffer, > but I consider this a violation of the principle of least surprise. Why > don't you just change silencedetect or add another function? > 2. For very small timestamps (< 10^-4), the new code will print a lot of > useless leading zeros (after the decimal point). In fact, it can be so > many that the new code has less precision than the old code, despite > using the fill buffer. > 2. This is way too much code for an inline function. > 3. Anyway, your placement of {} on their own lines does not match the > project coding style. > In addition to this, there is another issue here: Your av_ts_strip_trailing_zeros_and_decimal_point() presumes that the "decimal-point character" is always '.', but this can be changed via setlocale(). - Andreas
On Mon, 11 Mar 2024, Andreas Rheinhardt wrote: > Allan Cady via ffmpeg-devel: >> From: "Allan Cady" <allancady@yahoo.com> >> >> 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 <allancady@yahoo.com> >> --- >> 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 <stdbool.h> >> >> #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; >> } >> > > 1. What makes you believe that all users want the new format and that it > does not cause undesired behaviour for some (maybe a lot) of them? The > number of characters written by the earlier code stayed pretty constant > even when the times became big (in this case, it just printed 8 chars if > ts>=0), yet your code will really make use of the whole buffer. > Granted, we could tell our users that they have no right to complain > about this, given that we always had a "right" to use the full buffer, > but I consider this a violation of the principle of least surprise. Why > don't you just change silencedetect or add another function? I suggested to change av_ts_make_time_string() because this problem affect all detection filters, not only silencedetect. So fixing it in silencedetect only is wrong. The API did not make any promises about the format, and as long as it provides less chars than AV_TS_MAX_STRING_SIZE, and the string is parseable string representation of a floating point number, it is not a break. Sure, it changes behaviour, but that is not unheard of if there is a good reason, and in this case, I believe there is. And I think it is more likely that some code is broken right now because the function loses precision or returns scientific notation for relatively small timestamps. Actually, it _was_ a suprise for me, so IMHO the element of least suprise is that precision will not fade away for reasonably small timestamps. > 2. For very small timestamps (< 10^-4), the new code will print a lot of > useless leading zeros (after the decimal point). In fact, it can be so > many that the new code has less precision than the old code, despite > using the fill buffer. > 2. This is way too much code for an inline function. > 3. Anyway, your placement of {} on their own lines does not match the > project coding style. I am OK with any implementation which keeps at least millisecond precision for timestamps < 10^10. You are right, it should be as simple as possible. Regards, Marton
On 11 Mar 2024, at 15:26, Andreas Rheinhardt wrote: > Andreas Rheinhardt: >> Allan Cady via ffmpeg-devel: >>> From: "Allan Cady" <allancady@yahoo.com> >>> >>> 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 <allancady@yahoo.com> >>> --- >>> 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 <stdbool.h> >>> >>> #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; >>> } >>> >> >> 1. What makes you believe that all users want the new format and that it >> does not cause undesired behaviour for some (maybe a lot) of them? The >> number of characters written by the earlier code stayed pretty constant >> even when the times became big (in this case, it just printed 8 chars if >> ts>=0), yet your code will really make use of the whole buffer. >> Granted, we could tell our users that they have no right to complain >> about this, given that we always had a "right" to use the full buffer, >> but I consider this a violation of the principle of least surprise. Why >> don't you just change silencedetect or add another function? >> 2. For very small timestamps (< 10^-4), the new code will print a lot of >> useless leading zeros (after the decimal point). In fact, it can be so >> many that the new code has less precision than the old code, despite >> using the fill buffer. >> 2. This is way too much code for an inline function. >> 3. Anyway, your placement of {} on their own lines does not match the >> project coding style. >> > > In addition to this, there is another issue here: Your > av_ts_strip_trailing_zeros_and_decimal_point() presumes that the > "decimal-point character" is always '.', but this can be changed via > setlocale(). True, though I would consider this a more general bug. We should be consistent and not generate files that are locale-dependent and then not parseable anymore with a different one… That’s just a huge mess. Also in general FFmpeg is completely broken if you use any locale that does not use . as decimal separator. (This never shows for most users currently as most people use FFmpeg CLI which does not respect the users locale) > > - Andreas > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> On Monday, March 11, 2024 at 12:50:11 PM PDT, <epirat07@gmail.com> wrote: > On 11 Mar 2024, at 15:26, Andreas Rheinhardt wrote: >> Andreas Rheinhardt: >>> Allan Cady via ffmpeg-devel: >>>> From: "Allan Cady" <allancady@yahoo.com> >>>> >>>> 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 <allancady@yahoo.com> >>>> --- >>>> 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 <stdbool.h> >>>> >>>> #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; >>>> } >>>> >>> >>> 1. What makes you believe that all users want the new format and that it >>> does not cause undesired behaviour for some (maybe a lot) of them? I definitely do not know what other users would want. I would think maybe some would like the change, others wouldn't, and most would never know. >>> The >>> number of characters written by the earlier code stayed pretty constant >>> even when the times became big (in this case, it just printed 8 chars if >>> ts>=0), yet your code will really make use of the whole buffer. It's true that my change will increase the potential length of the output beyond 8 significant digits. The issue I was having that brought this up was, I have some very long audio files (up to 50 hours long), which I was wanting to split into smaller pieces. I wrote some scripts that use silencedetect to get the locations of breaks and then split the files at the breaks, but I discovered that for segments near the end of the file, silencedetect was returning whole-number timestamps, which was causing undesirable results for me. Thinking functionally, it seems like timestamps further out in a file ought to have the same precision as those near the beginning. So this seems to me like a minor oversight in the original design, that might warrant fixing. >>> Granted, we could tell our users that they have no right to complain >>> about this, given that we always had a "right" to use the full buffer, >>> but I consider this a violation of the principle of least surprise. I definitely agree with you there. >>> Why don't you just change silencedetect or add another function? I actually started out taking that approach in my submission a few weeks ago. Marton Balint suggested (in a message on 20 Feb) that we make the change in av_ts_make_time_string, so I did that for this submission. I'm open to whatever approach you all consider is best. >>> 2. For very small timestamps (< 10^-4), the new code will print a lot of >>> useless leading zeros (after the decimal point). In fact, it can be so >>> many that the new code has less precision than the old code, despite >>> using the fill buffer. I don't understand. Leading zeros after the decimal point are far from useless -- they are part of the value. Maybe what you're saying is that six digits is more precision than necessary? That may be so. I could personally do fine with just two digits (hundredths), as long as it's consistent through the length of the file. >>> 2. This is way too much code for an inline function. No disagreement there. >>> 3. Anyway, your placement of {} on their own lines does not match the >>> project coding style. I'm happy to conform with project coding style. >> In addition to this, there is another issue here: Your >> av_ts_strip_trailing_zeros_and_decimal_point() presumes that the >> "decimal-point character" is always '.', but this can be changed via >> setlocale(). Excellent point, which I hadn't considered. I have no experience with how locale is handled in C. I would welcome advice on the best way to handle this. > True, though I would consider this a more general bug. We should be > consistent and not generate files that are locale-dependent and then > not parseable anymore with a different one… That’s just a huge mess. > > Also in general FFmpeg is completely broken if you use any locale that > does not use . as decimal separator. (This never shows for most users > currently as most people use FFmpeg CLI which does not respect the > users locale) I'll leave that conversation to the experts here. Thanks for giving my code a look.
On Monday, March 11, 2024 at 12:11:45 PM PDT, Marton Balint <cus@passwd.hu> wrote: > On Mon, 11 Mar 2024, Andreas Rheinhardt wrote: > Allan Cady via ffmpeg-devel: >> From: "Allan Cady" <allancady@yahoo.com> >> >> 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 <allancady@yahoo.com> >> --- >> 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 <stdbool.h> >> >> #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; >> } >> > > 1. What makes you believe that all users want the new format and that it > does not cause undesired behaviour for some (maybe a lot) of them? The > number of characters written by the earlier code stayed pretty constant > even when the times became big (in this case, it just printed 8 chars if > ts>=0), yet your code will really make use of the whole buffer. > Granted, we could tell our users that they have no right to complain > about this, given that we always had a "right" to use the full buffer, > but I consider this a violation of the principle of least surprise. Why > don't you just change silencedetect or add another function? > > I suggested to change av_ts_make_time_string() because this > problem affect all detection filters, not only silencedetect. So fixing > it in silencedetect only is wrong. > > The API did not make any promises about the format, and as long as it > provides less chars than AV_TS_MAX_STRING_SIZE, and the string is > parseable string representation of a floating point number, it is not a > break. > > Sure, it changes behaviour, but that is not unheard of if there is a good > reason, and in this case, I believe there is. And I think it is more > likely that some code is broken right now because the function > loses precision or returns scientific notation for relatively small > timestamps. Actually, it _was_ a suprise for me, so IMHO the element of > least suprise is that precision will not fade away for reasonably small > timestamps. > > 2. For very small timestamps (< 10^-4), the new code will print a lot of > useless leading zeros (after the decimal point). In fact, it can be so > many that the new code has less precision than the old code, despite > using the fill buffer. > 2. This is way too much code for an inline function. > 3. Anyway, your placement of {} on their own lines does not match the > project coding style. > > I am OK with any implementation which keeps at least millisecond > precision for timestamps < 10^10. You are right, it should be as simple as > possible. > Milliseconds would be fine with me. Marton, do you have any other comments on my implementation? I have from Andreas that I should remove the inlines, and move the curly braces to match coding style. I also plan on removing the #include <stdbool.h>, which I added at some point but is no longer needed. And I would be happy to change from %.6f to %.3f. If that sounds good, I'll submit another patch sometime tomorrow. The only other thing I had thought of is to wonder if I should add some additional testing for the new formatting. I did a fair amount of testing on my own, but it would probably be good to have at least some of that in FATE. I had thought about maybe generating an audio file with just a tone and several silence intervals.
On Tue, 12 Mar 2024, Allan Cady via ffmpeg-devel wrote: > On Monday, March 11, 2024 at 12:11:45 PM PDT, Marton Balint <cus@passwd.hu> wrote: >> On Mon, 11 Mar 2024, Andreas Rheinhardt wrote: >> Allan Cady via ffmpeg-devel: >>> From: "Allan Cady" <allancady@yahoo.com> >>> >>> 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 <allancady@yahoo.com> >>> --- >>> 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 <stdbool.h> >>> >>> #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; >>> } >>> >> >> 1. What makes you believe that all users want the new format and that it >> does not cause undesired behaviour for some (maybe a lot) of them? The >> number of characters written by the earlier code stayed pretty constant >> even when the times became big (in this case, it just printed 8 chars if >> ts>=0), yet your code will really make use of the whole buffer. >> Granted, we could tell our users that they have no right to complain >> about this, given that we always had a "right" to use the full buffer, >> but I consider this a violation of the principle of least surprise. Why >> don't you just change silencedetect or add another function? >> >> I suggested to change av_ts_make_time_string() because this >> problem affect all detection filters, not only silencedetect. So fixing >> it in silencedetect only is wrong. >> >> The API did not make any promises about the format, and as long as it >> provides less chars than AV_TS_MAX_STRING_SIZE, and the string is >> parseable string representation of a floating point number, it is not a >> break. >> >> Sure, it changes behaviour, but that is not unheard of if there is a good >> reason, and in this case, I believe there is. And I think it is more >> likely that some code is broken right now because the function >> loses precision or returns scientific notation for relatively small >> timestamps. Actually, it _was_ a suprise for me, so IMHO the element of >> least suprise is that precision will not fade away for reasonably small >> timestamps. >> >> 2. For very small timestamps (< 10^-4), the new code will print a lot of >> useless leading zeros (after the decimal point). In fact, it can be so >> many that the new code has less precision than the old code, despite >> using the fill buffer. >> 2. This is way too much code for an inline function. >> 3. Anyway, your placement of {} on their own lines does not match the >> project coding style. >> >> I am OK with any implementation which keeps at least millisecond >> precision for timestamps < 10^10. You are right, it should be as simple as >> possible. >> > > Milliseconds would be fine with me. > > Marton, do you have any other comments on my implementation? I have > from Andreas that I should remove the inlines, and move the curly > braces to match coding style. I also plan on removing the > #include <stdbool.h>, which I added at some point but is no longer > needed. And I would be happy to change from %.6f to %.3f. TBH I'd rather keep the precision as is. If you want to convert the function to non-inlined, then you will have to move the implementation from the header to an existing or new C file and unconditionally compile it to avutil... Maybe we should give it another go keeping it inlineable by simplifying it a little. > > If that sounds good, I'll submit another patch sometime tomorrow. > > The only other thing I had thought of is to wonder if I should add some > additional testing for the new formatting. I did a fair amount of testing > on my own, but it would probably be good to have at least some of that > in FATE. I had thought about maybe generating an audio file with just > a tone and several silence intervals. 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. 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? Regards, Marton
On Tuesday, March 12, 2024 at 02:24:47 PM PDT, Marton Balint <cus@passwd.hu> 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 <cus@passwd.hu> wrote: >>> On Mon, 11 Mar 2024, Andreas Rheinhardt wrote: >>> Allan Cady via ffmpeg-devel: >>>> From: "Allan Cady" <allancady@yahoo.com> >>>> >>>> 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 <allancady@yahoo.com> >>>> --- >>>> 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 <stdbool.h> >>>> >>>> #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; >>>> } >>>> >>> >>> 1. What makes you believe that all users want the new format and that it >>> does not cause undesired behaviour for some (maybe a lot) of them? The >>> number of characters written by the earlier code stayed pretty constant >>> even when the times became big (in this case, it just printed 8 chars if >>> ts>=0), yet your code will really make use of the whole buffer. >>> Granted, we could tell our users that they have no right to complain >>> about this, given that we always had a "right" to use the full buffer, >>> but I consider this a violation of the principle of least surprise. Why >>> don't you just change silencedetect or add another function? >>> >>> I suggested to change av_ts_make_time_string() because this >>> problem affect all detection filters, not only silencedetect. So fixing >>> it in silencedetect only is wrong. >>> >>> The API did not make any promises about the format, and as long as it >>> provides less chars than AV_TS_MAX_STRING_SIZE, and the string is >>> parseable string representation of a floating point number, it is not a >>> break. >>> >>> Sure, it changes behaviour, but that is not unheard of if there is a good >>> reason, and in this case, I believe there is. And I think it is more >>> likely that some code is broken right now because the function >>> loses precision or returns scientific notation for relatively small >>> timestamps. Actually, it _was_ a suprise for me, so IMHO the element of >>> least suprise is that precision will not fade away for reasonably small >>> timestamps. >>> >>> 2. For very small timestamps (< 10^-4), the new code will print a lot of >>> useless leading zeros (after the decimal point). In fact, it can be so >>> many that the new code has less precision than the old code, despite >>> using the fill buffer. >>> 2. This is way too much code for an inline function. >>> 3. Anyway, your placement of {} on their own lines does not match the >>> project coding style. >>> >>> I am OK with any implementation which keeps at least millisecond >>> precision for timestamps < 10^10. You are right, it should be as simple as >>> possible. >>> >> >> Milliseconds would be fine with me. >> >> Marton, do you have any other comments on my implementation? I have >> from Andreas that I should remove the inlines, and move the curly >> braces to match coding style. I also plan on removing the >> #include <stdbool.h>, which I added at some point but is no longer >> needed. And I would be happy to change from %.6f to %.3f. > TBH I'd rather keep the precision as is. If you want to convert the > function to non-inlined, then you will have to move the implementation > from the header to an existing or new C file and unconditionally compile > it to avutil... Maybe we should give it another go keeping it inlineable > by simplifying it a little. It's been years (decades) since I had to think about inlines -- and I don't remember there being any fixed criteria for them. Is it a language rule that subroutines in header files have to be inlined? Or just a policy? I really don't know the language. Anyway, I'm all for simple. I was fairly uncomfortable with that much complexity, especially in a header file. I don't really like having that extra sub to strip trailing zeros, but I couldn't come up with a better way to do it. >> >> If that sounds good, I'll submit another patch sometime tomorrow. >> >> The only other thing I had thought of is to wonder if I should add some >> additional testing for the new formatting. I did a fair amount of testing >> on my own, but it would probably be good to have at least some of that >> in FATE. I had thought about maybe generating an audio file with just >> a tone and several silence intervals. > > 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.
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).
On Wed, 13 Mar 2024, Allan Cady via ffmpeg-devel wrote: > On Tuesday, March 12, 2024 at 02:24:47 PM PDT, Marton Balint <cus@passwd.hu> 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 <cus@passwd.hu> wrote: >>>> On Mon, 11 Mar 2024, Andreas Rheinhardt wrote: >>>> Allan Cady via ffmpeg-devel: >>>>> From: "Allan Cady" <allancady@yahoo.com> >>>>> [...] > > >> 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
On Sunday, March 17, 2024 at 04:43:31 PM PDT, Marton Balint <cus@passwd.hu> 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 <cus@passwd.hu> 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 <cus@passwd.hu> wrote:>>>>> On Mon, 11 Mar 2024, Andreas Rheinhardt wrote:>>>>> Allan Cady via ffmpeg-devel:>>>>>> From: "Allan Cady" <allancady@yahoo.com>>>>>>>> > [...]> >>>>>>> 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?
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 <stdbool.h> #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.66667 +pts=5800|tag:lavfi.scd.score=55.819|tag:lavfi.scd.mafd=55.819|tag:lavfi.scd.time=9.666667 pts=6720|tag:lavfi.scd.score=18.580|tag:lavfi.scd.mafd=22.505|tag:lavfi.scd.time=11.2 pts=8160|tag:lavfi.scd.score=49.240|tag:lavfi.scd.mafd=49.444|tag:lavfi.scd.time=13.6 -pts=9760|tag:lavfi.scd.score=51.497|tag:lavfi.scd.mafd=51.801|tag:lavfi.scd.time=16.2667 -pts=14080|tag:lavfi.scd.score=34.165|tag:lavfi.scd.mafd=34.337|tag:lavfi.scd.time=23.4667 -pts=15700|tag:lavfi.scd.score=58.310|tag:lavfi.scd.mafd=58.315|tag:lavfi.scd.time=26.1667 -pts=18500|tag:lavfi.scd.score=16.504|tag:lavfi.scd.mafd=19.603|tag:lavfi.scd.time=30.8333 +pts=9760|tag:lavfi.scd.score=51.497|tag:lavfi.scd.mafd=51.801|tag:lavfi.scd.time=16.266667 +pts=14080|tag:lavfi.scd.score=34.165|tag:lavfi.scd.mafd=34.337|tag:lavfi.scd.time=23.466667 +pts=15700|tag:lavfi.scd.score=58.310|tag:lavfi.scd.mafd=58.315|tag:lavfi.scd.time=26.166667 +pts=18500|tag:lavfi.scd.score=16.504|tag:lavfi.scd.mafd=19.603|tag:lavfi.scd.time=30.833333 pts=20040|tag:lavfi.scd.score=13.764|tag:lavfi.scd.mafd=19.060|tag:lavfi.scd.time=33.4 -pts=21760|tag:lavfi.scd.score=64.451|tag:lavfi.scd.mafd=64.551|tag:lavfi.scd.time=36.2667 +pts=21760|tag:lavfi.scd.score=64.451|tag:lavfi.scd.mafd=64.551|tag:lavfi.scd.time=36.266667 diff --git a/tests/ref/fate/filter-metadata-silencedetect b/tests/ref/fate/filter-metadata-silencedetect index bc53fea047..e66ffe5fdd 100644 --- a/tests/ref/fate/filter-metadata-silencedetect +++ b/tests/ref/fate/filter-metadata-silencedetect @@ -1,5 +1,5 @@ pts=0|tag:lavfi.silence_duration=0.523107|tag:lavfi.silence_end=0.690023|tag:lavfi.silence_start=0.736417 -pts=46080|tag:lavfi.silence_start=1.27626|tag:lavfi.silence_end=1.80751|tag:lavfi.silence_duration=0.531247 +pts=46080|tag:lavfi.silence_start=1.276259|tag:lavfi.silence_end=1.807506|tag:lavfi.silence_duration=0.531247 pts=92160 pts=138240 pts=184320