[FFmpeg-devel] [PATCH] change av_ts_make_time_string precision

[Allan Cady]

On Monday, March 11, 2024 at 12:11:45 PM PDT, Marton Balint <cus at passwd.hu> wrote: 
> On Mon, 11 Mar 2024, Andreas Rheinhardt wrote:
> Allan Cady via ffmpeg-devel:
>> From: "Allan Cady" <allancady at 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 at 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.