[FFmpeg-devel] avutil/timestamp: keep microsecond precision in av_ts_make_time_string

av_ts_make_time_string() used "%.6g" format in the past, 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.

So let's change the used format to "%.6f" instead, we have plenty of space in
the string buffer, we can somewhat imitate existing behaviour of %g by trimming
ending zeroes and the potential decimal point, which can be any non-numeric
character. In order not to trim "inf" as well, we assume that the decimal
point does not contain the letter "f".

We also no longer use scientific representation to make parsing and printing
easier, because the theoretical maximum of INT64_MAX*INT32_MAX still fits into
the string buffer in normal form.

Since the additional processing yields more code, inlineing this function no
longer make sense, so this commit also changes the API to actually export the
function instead of having it inlinable in the header.

Thanks for Allan Cady for bringing up this issue.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 doc/APIchanges                               |  3 ++
 libavutil/Makefile                           |  1 +
 libavutil/timestamp.c                        | 33 ++++++++++++++++++++
 libavutil/timestamp.h                        |  8 +----
 libavutil/version.h                          |  2 +-
 tests/ref/fate/filter-metadata-scdet         | 12 +++----
 tests/ref/fate/filter-metadata-silencedetect |  2 +-
 7 files changed, 46 insertions(+), 15 deletions(-)
 create mode 100644 libavutil/timestamp.c

On Mon, 18 Mar 2024, Marton Balint wrote:

> av_ts_make_time_string() used "%.6g" format in the past, 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.
>
> So let's change the used format to "%.6f" instead, we have plenty of space in
> the string buffer, we can somewhat imitate existing behaviour of %g by trimming
> ending zeroes and the potential decimal point, which can be any non-numeric
> character. In order not to trim "inf" as well, we assume that the decimal
> point does not contain the letter "f".
>
> We also no longer use scientific representation to make parsing and printing
> easier, because the theoretical maximum of INT64_MAX*INT32_MAX still fits into
> the string buffer in normal form.
>
> Since the additional processing yields more code, inlineing this function no
> longer make sense, so this commit also changes the API to actually export the
> function instead of having it inlinable in the header.
>
> Thanks for Allan Cady for bringing up this issue.

Ping for this. Considering the API and behaviour change, I prefer we apply 
this before branching 7.0.

Thanks,
Marton

>
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
> doc/APIchanges                               |  3 ++
> libavutil/Makefile                           |  1 +
> libavutil/timestamp.c                        | 33 ++++++++++++++++++++
> libavutil/timestamp.h                        |  8 +----
> libavutil/version.h                          |  2 +-
> tests/ref/fate/filter-metadata-scdet         | 12 +++----
> tests/ref/fate/filter-metadata-silencedetect |  2 +-
> 7 files changed, 46 insertions(+), 15 deletions(-)
> create mode 100644 libavutil/timestamp.c
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index a44c8e4f10..1afde062a5 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
>
> API changes, most recent first:
>
> +2024-03-xx - xxxxxxxxxx - lavu 59.3.100 - timestamp.h
> +  av_ts_make_time_string() is no longer an inline function. It is now exported.
> +
> 2024-03-xx - xxxxxxxxxx - lavu 59.2.100 - channel_layout.h
>   Add AV_CHANNEL_LAYOUT_RETYPE_FLAG_CANONICAL.
>
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index e7709b97d0..5e75aa1855 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 0000000000..06fb47e94c
> --- /dev/null
> +++ b/libavutil/timestamp.c
> @@ -0,0 +1,33 @@
> +/*
> + * 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
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "timestamp.h"
> +
> +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 {
> +        int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.6f", av_q2d(*tb) * ts);
> +        last = FFMIN(last, AV_TS_MAX_STRING_SIZE - 1) - 1;
> +        for (; last && buf[last] == '0'; last--);
> +        for (; last && buf[last] != 'f' && (buf[last] < '0' || buf[0] > '9'); last--);
> +        buf[last + 1] = '\0';
> +    }
> +    return buf;
> +}
> diff --git a/libavutil/timestamp.h b/libavutil/timestamp.h
> index 2b37781eba..a02d873060 100644
> --- a/libavutil/timestamp.h
> +++ b/libavutil/timestamp.h
> @@ -62,13 +62,7 @@ static inline char *av_ts_make_string(char *buf, int64_t ts)
>  * @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,
> -                                           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);
> -    return buf;
> -}
> +char *av_ts_make_time_string(char *buf, int64_t ts, const AVRational *tb);
>
> /**
>  * Convenience macro, the return value should be used only directly in
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 57cad02ec0..5027b025be 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>  */
>
> #define LIBAVUTIL_VERSION_MAJOR  59
> -#define LIBAVUTIL_VERSION_MINOR   2
> +#define LIBAVUTIL_VERSION_MINOR   3
> #define LIBAVUTIL_VERSION_MICRO 100
>
> #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
> 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
> -- 
> 2.35.3
>
> _______________________________________________
> 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".
>

Marton Balint:
> av_ts_make_time_string() used "%.6g" format in the past, 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.
> 
> So let's change the used format to "%.6f" instead, we have plenty of space in
> the string buffer, we can somewhat imitate existing behaviour of %g by trimming
> ending zeroes and the potential decimal point, which can be any non-numeric
> character. In order not to trim "inf" as well, we assume that the decimal
> point does not contain the letter "f".
> 
> We also no longer use scientific representation to make parsing and printing
> easier, because the theoretical maximum of INT64_MAX*INT32_MAX still fits into
> the string buffer in normal form.
> 
> Since the additional processing yields more code, inlineing this function no
> longer make sense, so this commit also changes the API to actually export the
> function instead of having it inlinable in the header.
> 
> Thanks for Allan Cady for bringing up this issue.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  doc/APIchanges                               |  3 ++
>  libavutil/Makefile                           |  1 +
>  libavutil/timestamp.c                        | 33 ++++++++++++++++++++
>  libavutil/timestamp.h                        |  8 +----
>  libavutil/version.h                          |  2 +-
>  tests/ref/fate/filter-metadata-scdet         | 12 +++----
>  tests/ref/fate/filter-metadata-silencedetect |  2 +-
>  7 files changed, 46 insertions(+), 15 deletions(-)
>  create mode 100644 libavutil/timestamp.c
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index a44c8e4f10..1afde062a5 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
>  
>  API changes, most recent first:
>  
> +2024-03-xx - xxxxxxxxxx - lavu 59.3.100 - timestamp.h
> +  av_ts_make_time_string() is no longer an inline function. It is now exported.
> +
>  2024-03-xx - xxxxxxxxxx - lavu 59.2.100 - channel_layout.h
>    Add AV_CHANNEL_LAYOUT_RETYPE_FLAG_CANONICAL.
>  
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index e7709b97d0..5e75aa1855 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 0000000000..06fb47e94c
> --- /dev/null
> +++ b/libavutil/timestamp.c
> @@ -0,0 +1,33 @@
> +/*
> + * 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
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "timestamp.h"
> +
> +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 {
> +        int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.6f", av_q2d(*tb) * ts);
> +        last = FFMIN(last, AV_TS_MAX_STRING_SIZE - 1) - 1;
> +        for (; last && buf[last] == '0'; last--);
> +        for (; last && buf[last] != 'f' && (buf[last] < '0' || buf[0] > '9'); last--);
> +        buf[last + 1] = '\0';
> +    }
> +    return buf;
> +}

As has already been said before: Simply using %.6f will discard
significant digits for small timestamps. E.g. 10^-7 will simply print
0.000000, which will then be converted to "0" by your code. The old code
printed 1.

Furthermore, there are more problems:
"A double argument representing an infinity is converted in one of the
styles [-]inf or [-]infinity — which style is implementation-defined."
Your code would trim infinity to "inf". It would be even worse with nan:
"A double argument representing a NaN is converted in one of the styles
[-]nan or [-]nan(n-char-sequence) — which style, and the meaning of
any n-char-sequence, is implementation-defined."
Furthermore, there is no guarantee that the "decimal-point character" is
actually a single character (it's a char* in struct lcov after all).

If I am not mistaken, then NANs and infinities can't happen here unless
the timebase is malformed (denominator zero); and in this case one could
use the NOPTS codepath.

- Andreas

Andreas Rheinhardt:
> Marton Balint:
>> av_ts_make_time_string() used "%.6g" format in the past, 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.
>>
>> So let's change the used format to "%.6f" instead, we have plenty of space in
>> the string buffer, we can somewhat imitate existing behaviour of %g by trimming
>> ending zeroes and the potential decimal point, which can be any non-numeric
>> character. In order not to trim "inf" as well, we assume that the decimal
>> point does not contain the letter "f".
>>
>> We also no longer use scientific representation to make parsing and printing
>> easier, because the theoretical maximum of INT64_MAX*INT32_MAX still fits into
>> the string buffer in normal form.
>>
>> Since the additional processing yields more code, inlineing this function no
>> longer make sense, so this commit also changes the API to actually export the
>> function instead of having it inlinable in the header.
>>
>> Thanks for Allan Cady for bringing up this issue.
>>
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  doc/APIchanges                               |  3 ++
>>  libavutil/Makefile                           |  1 +
>>  libavutil/timestamp.c                        | 33 ++++++++++++++++++++
>>  libavutil/timestamp.h                        |  8 +----
>>  libavutil/version.h                          |  2 +-
>>  tests/ref/fate/filter-metadata-scdet         | 12 +++----
>>  tests/ref/fate/filter-metadata-silencedetect |  2 +-
>>  7 files changed, 46 insertions(+), 15 deletions(-)
>>  create mode 100644 libavutil/timestamp.c
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index a44c8e4f10..1afde062a5 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
>>  
>>  API changes, most recent first:
>>  
>> +2024-03-xx - xxxxxxxxxx - lavu 59.3.100 - timestamp.h
>> +  av_ts_make_time_string() is no longer an inline function. It is now exported.
>> +
>>  2024-03-xx - xxxxxxxxxx - lavu 59.2.100 - channel_layout.h
>>    Add AV_CHANNEL_LAYOUT_RETYPE_FLAG_CANONICAL.
>>  
>> diff --git a/libavutil/Makefile b/libavutil/Makefile
>> index e7709b97d0..5e75aa1855 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 0000000000..06fb47e94c
>> --- /dev/null
>> +++ b/libavutil/timestamp.c
>> @@ -0,0 +1,33 @@
>> +/*
>> + * 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
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +
>> +#include "timestamp.h"
>> +
>> +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 {
>> +        int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.6f", av_q2d(*tb) * ts);
>> +        last = FFMIN(last, AV_TS_MAX_STRING_SIZE - 1) - 1;
>> +        for (; last && buf[last] == '0'; last--);
>> +        for (; last && buf[last] != 'f' && (buf[last] < '0' || buf[0] > '9'); last--);
>> +        buf[last + 1] = '\0';
>> +    }
>> +    return buf;
>> +}
> 
> As has already been said before: Simply using %.6f will discard
> significant digits for small timestamps. E.g. 10^-7 will simply print
> 0.000000, which will then be converted to "0" by your code. The old code
> printed 1.
> 
> Furthermore, there are more problems:
> "A double argument representing an infinity is converted in one of the
> styles [-]inf or [-]infinity — which style is implementation-defined."
> Your code would trim infinity to "inf". It would be even worse with nan:
> "A double argument representing a NaN is converted in one of the styles
> [-]nan or [-]nan(n-char-sequence) — which style, and the meaning of
> any n-char-sequence, is implementation-defined."
> Furthermore, there is no guarantee that the "decimal-point character" is
> actually a single character (it's a char* in struct lcov after all).
> 
> If I am not mistaken, then NANs and infinities can't happen here unless
> the timebase is malformed (denominator zero); and in this case one could
> use the NOPTS codepath.
> 

While just at it: As I already mentioned, this function has a design
defect: It passes the timebase by pointer and not just by value. So if
we add a non-inlined version of this, it should be a new function that
receives the timebase by value and av_ts_make_time_string() should
instead call this new function like
static inline char *av_ts_make_time_string(char *buf, int64_t ts, const
AVRational *tb)
{
    return new_func(buf, ts, *tb);
}

- Andreas

On Wed, 20 Mar 2024, Andreas Rheinhardt wrote:

> Andreas Rheinhardt:
>> Marton Balint:
>>> av_ts_make_time_string() used "%.6g" format in the past, 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.
>>>
>>> So let's change the used format to "%.6f" instead, we have plenty of space in
>>> the string buffer, we can somewhat imitate existing behaviour of %g by trimming
>>> ending zeroes and the potential decimal point, which can be any non-numeric
>>> character. In order not to trim "inf" as well, we assume that the decimal
>>> point does not contain the letter "f".
>>>
>>> We also no longer use scientific representation to make parsing and printing
>>> easier, because the theoretical maximum of INT64_MAX*INT32_MAX still fits into
>>> the string buffer in normal form.
>>>
>>> Since the additional processing yields more code, inlineing this function no
>>> longer make sense, so this commit also changes the API to actually export the
>>> function instead of having it inlinable in the header.
>>>
>>> Thanks for Allan Cady for bringing up this issue.
>>>
>>> Signed-off-by: Marton Balint <cus@passwd.hu>
>>> ---
>>>  doc/APIchanges                               |  3 ++
>>>  libavutil/Makefile                           |  1 +
>>>  libavutil/timestamp.c                        | 33 ++++++++++++++++++++
>>>  libavutil/timestamp.h                        |  8 +----
>>>  libavutil/version.h                          |  2 +-
>>>  tests/ref/fate/filter-metadata-scdet         | 12 +++----
>>>  tests/ref/fate/filter-metadata-silencedetect |  2 +-
>>>  7 files changed, 46 insertions(+), 15 deletions(-)
>>>  create mode 100644 libavutil/timestamp.c
>>>
>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>> index a44c8e4f10..1afde062a5 100644
>>> --- a/doc/APIchanges
>>> +++ b/doc/APIchanges
>>> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
>>>
>>>  API changes, most recent first:
>>> 
>>> +2024-03-xx - xxxxxxxxxx - lavu 59.3.100 - timestamp.h
>>> +  av_ts_make_time_string() is no longer an inline function. It is now exported.
>>> +
>>>  2024-03-xx - xxxxxxxxxx - lavu 59.2.100 - channel_layout.h
>>>    Add AV_CHANNEL_LAYOUT_RETYPE_FLAG_CANONICAL.
>>> 
>>> diff --git a/libavutil/Makefile b/libavutil/Makefile
>>> index e7709b97d0..5e75aa1855 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 0000000000..06fb47e94c
>>> --- /dev/null
>>> +++ b/libavutil/timestamp.c
>>> @@ -0,0 +1,33 @@
>>> +/*
>>> + * 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
>>> + * License along with FFmpeg; if not, write to the Free Software
>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>> + */
>>> +
>>> +#include "timestamp.h"
>>> +
>>> +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 {
>>> +        int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.6f", av_q2d(*tb) * ts);
>>> +        last = FFMIN(last, AV_TS_MAX_STRING_SIZE - 1) - 1;
>>> +        for (; last && buf[last] == '0'; last--);
>>> +        for (; last && buf[last] != 'f' && (buf[last] < '0' || buf[0] > '9'); last--);
>>> +        buf[last + 1] = '\0';
>>> +    }
>>> +    return buf;
>>> +}
>> 
>> As has already been said before: Simply using %.6f will discard
>> significant digits for small timestamps. E.g. 10^-7 will simply print
>> 0.000000, which will then be converted to "0" by your code. The old code
>> printed 1.

Admittedly we will lose precision < 10^-6 by using %.6f, but I don't 
think this will cause any real problems, so I'd rather just leave this as 
is. Or you prefer some special handling of small values? E.g. using %.9f 
or something?

>> 
>> Furthermore, there are more problems:
>> "A double argument representing an infinity is converted in one of the
>> styles [-]inf or [-]infinity — which style is implementation-defined."
>> Your code would trim infinity to "inf". It would be even worse with nan:
>> "A double argument representing a NaN is converted in one of the styles
>> [-]nan or [-]nan(n-char-sequence) — which style, and the meaning of
>> any n-char-sequence, is implementation-defined."
>> Furthermore, there is no guarantee that the "decimal-point character" is
>> actually a single character (it's a char* in struct lcov after all).
>> 
>> If I am not mistaken, then NANs and infinities can't happen here unless
>> the timebase is malformed (denominator zero); and in this case one could
>> use the NOPTS codepath.

So the NAN case can't happen. The multiple character decimal separator is 
not a problem because we are trimming multiple chars. The inf instead of 
infinity case should not be a problem either, inf is parseable the 
same way as infinity, strtod() supports both. But if you prefer, I 
can make the code look for 'y' as well to stop trimming, I don't think it 
really matters. I am not sure about returning NOPTS for infinity.

> While just at it: As I already mentioned, this function has a design
> defect: It passes the timebase by pointer and not just by value. So if
> we add a non-inlined version of this, it should be a new function that
> receives the timebase by value and av_ts_make_time_string() should
> instead call this new function like
> static inline char *av_ts_make_time_string(char *buf, int64_t ts, const
> AVRational *tb)
> {
>    return new_func(buf, ts, *tb);
> }

I am uneasy about adding another public function signature for this 
purpose, because that will still have the flaw of using a locale 
dependant decimal point.

Regards,
Marton

On Wed, 20 Mar 2024, Marton Balint wrote:

>
>
> On Wed, 20 Mar 2024, Andreas Rheinhardt wrote:
>
>>  Andreas Rheinhardt:
>>>  Marton Balint:
>>>>  av_ts_make_time_string() used "%.6g" format in the past, 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.
>>>>
>>>>  So let's change the used format to "%.6f" instead, we have plenty of
>>>>  space in
>>>>  the string buffer, we can somewhat imitate existing behaviour of %g by
>>>>  trimming
>>>>  ending zeroes and the potential decimal point, which can be any
>>>>  non-numeric
>>>>  character. In order not to trim "inf" as well, we assume that the
>>>>  decimal
>>>>  point does not contain the letter "f".
>>>>
>>>>  We also no longer use scientific representation to make parsing and
>>>>  printing
>>>>  easier, because the theoretical maximum of INT64_MAX*INT32_MAX still
>>>>  fits into
>>>>  the string buffer in normal form.
>>>>
>>>>  Since the additional processing yields more code, inlineing this
>>>>  function no
>>>>  longer make sense, so this commit also changes the API to actually
>>>>  export the
>>>>  function instead of having it inlinable in the header.
>>>>
>>>>  Thanks for Allan Cady for bringing up this issue.
>>>>
>>>>  Signed-off-by: Marton Balint <cus@passwd.hu>
>>>>  ---
>>>>   doc/APIchanges                               |  3 ++
>>>>   libavutil/Makefile                           |  1 +
>>>>   libavutil/timestamp.c                        | 33 ++++++++++++++++++++
>>>>   libavutil/timestamp.h                        |  8 +----
>>>>   libavutil/version.h                          |  2 +-
>>>>   tests/ref/fate/filter-metadata-scdet         | 12 +++----
>>>>   tests/ref/fate/filter-metadata-silencedetect |  2 +-
>>>>   7 files changed, 46 insertions(+), 15 deletions(-)
>>>>   create mode 100644 libavutil/timestamp.c
>>>>
>>>>  diff --git a/doc/APIchanges b/doc/APIchanges
>>>>  index a44c8e4f10..1afde062a5 100644
>>>>  --- a/doc/APIchanges
>>>>  +++ b/doc/APIchanges
>>>> @@  -2,6 +2,9 @@ The last version increases of all libraries were on 
>>>> @@  2024-03-07
>>>>
>>>>   API changes, most recent first:
>>>>
>>>>  +2024-03-xx - xxxxxxxxxx - lavu 59.3.100 - timestamp.h
>>>>  +  av_ts_make_time_string() is no longer an inline function. It is now
>>>>  exported.
>>>>  +
>>>>   2024-03-xx - xxxxxxxxxx - lavu 59.2.100 - channel_layout.h
>>>>     Add AV_CHANNEL_LAYOUT_RETYPE_FLAG_CANONICAL.
>>>>
>>>>  diff --git a/libavutil/Makefile b/libavutil/Makefile
>>>>  index e7709b97d0..5e75aa1855 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 0000000000..06fb47e94c
>>>>  --- /dev/null
>>>>  +++ b/libavutil/timestamp.c
>>>> @@  -0,0 +1,33 @@
>>>>  +/*
>>>>  + * 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
>>>>  + * License along with FFmpeg; if not, write to the Free Software
>>>>  + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>>>>  02110-1301 USA
>>>>  + */
>>>>  +
>>>>  +#include "timestamp.h"
>>>>  +
>>>>  +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 {
>>>>  +        int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.6f",
>>>>  av_q2d(*tb) * ts);
>>>>  +        last = FFMIN(last, AV_TS_MAX_STRING_SIZE - 1) - 1;
>>>>  +        for (; last && buf[last] == '0'; last--);
>>>>  +        for (; last && buf[last] != 'f' && (buf[last] < '0' || buf[0] >
>>>>  '9'); last--);
>>>>  +        buf[last + 1] = '\0';
>>>>  +    }
>>>>  +    return buf;
>>>>  +}
>>>
>>>  As has already been said before: Simply using %.6f will discard
>>>  significant digits for small timestamps. E.g. 10^-7 will simply print
>>>  0.000000, which will then be converted to "0" by your code. The old code
>>>  printed 1.
>
> Admittedly we will lose precision < 10^-6 by using %.6f, but I don't think 
> this will cause any real problems, so I'd rather just leave this as is. Or 
> you prefer some special handling of small values? E.g. using %.9f or 
> something?
>
>>>
>>>  Furthermore, there are more problems:
>>>  "A double argument representing an infinity is converted in one of the
>>>  styles [-]inf or [-]infinity — which style is implementation-defined."
>>>  Your code would trim infinity to "inf". It would be even worse with nan:
>>>  "A double argument representing a NaN is converted in one of the styles
>>>  [-]nan or [-]nan(n-char-sequence) — which style, and the meaning of
>>>  any n-char-sequence, is implementation-defined."
>>>  Furthermore, there is no guarantee that the "decimal-point character" is
>>>  actually a single character (it's a char* in struct lcov after all).
>>>
>>>  If I am not mistaken, then NANs and infinities can't happen here unless
>>>  the timebase is malformed (denominator zero); and in this case one could
>>>  use the NOPTS codepath.
>
> So the NAN case can't happen. The multiple character decimal separator is not 
> a problem because we are trimming multiple chars. The inf instead of infinity 
> case should not be a problem either, inf is parseable the same way as 
> infinity, strtod() supports both. But if you prefer, I can make the code look 
> for 'y' as well to stop trimming, I don't think it really matters. I am not 
> sure about returning NOPTS for infinity.
>
>>  While just at it: As I already mentioned, this function has a design
>>  defect: It passes the timebase by pointer and not just by value. So if
>>  we add a non-inlined version of this, it should be a new function that
>>  receives the timebase by value and av_ts_make_time_string() should
>>  instead call this new function like
>>  static inline char *av_ts_make_time_string(char *buf, int64_t ts, const
>>  AVRational *tb)
>>  {
>>     return new_func(buf, ts, *tb);
>>  }
>
> I am uneasy about adding another public function signature for this purpose, 
> because that will still have the flaw of using a locale dependant decimal 
> point.

Andreas, please make it clear what is needed for this patch to become 
acceptable to you, which parts you insist on changing, which parts are you 
OK with as is.

Thanks,
Marton

Marton Balint:
> 
> 
> On Wed, 20 Mar 2024, Andreas Rheinhardt wrote:
> 
>> Andreas Rheinhardt:
>>> Marton Balint:
>>>> av_ts_make_time_string() used "%.6g" format in the past, 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.
>>>>
>>>> So let's change the used format to "%.6f" instead, we have plenty of
>>>> space in
>>>> the string buffer, we can somewhat imitate existing behaviour of %g
>>>> by trimming
>>>> ending zeroes and the potential decimal point, which can be any
>>>> non-numeric
>>>> character. In order not to trim "inf" as well, we assume that the
>>>> decimal
>>>> point does not contain the letter "f".
>>>>
>>>> We also no longer use scientific representation to make parsing and
>>>> printing
>>>> easier, because the theoretical maximum of INT64_MAX*INT32_MAX still
>>>> fits into
>>>> the string buffer in normal form.
>>>>
>>>> Since the additional processing yields more code, inlineing this
>>>> function no
>>>> longer make sense, so this commit also changes the API to actually
>>>> export the
>>>> function instead of having it inlinable in the header.
>>>>
>>>> Thanks for Allan Cady for bringing up this issue.
>>>>
>>>> Signed-off-by: Marton Balint <cus@passwd.hu>
>>>> ---
>>>>  doc/APIchanges                               |  3 ++
>>>>  libavutil/Makefile                           |  1 +
>>>>  libavutil/timestamp.c                        | 33 ++++++++++++++++++++
>>>>  libavutil/timestamp.h                        |  8 +----
>>>>  libavutil/version.h                          |  2 +-
>>>>  tests/ref/fate/filter-metadata-scdet         | 12 +++----
>>>>  tests/ref/fate/filter-metadata-silencedetect |  2 +-
>>>>  7 files changed, 46 insertions(+), 15 deletions(-)
>>>>  create mode 100644 libavutil/timestamp.c
>>>>
>>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>>> index a44c8e4f10..1afde062a5 100644
>>>> --- a/doc/APIchanges
>>>> +++ b/doc/APIchanges
>>>> @@ -2,6 +2,9 @@ The last version increases of all libraries were on
>>>> 2024-03-07
>>>>
>>>>  API changes, most recent first:
>>>>
>>>> +2024-03-xx - xxxxxxxxxx - lavu 59.3.100 - timestamp.h
>>>> +  av_ts_make_time_string() is no longer an inline function. It is
>>>> now exported.
>>>> +
>>>>  2024-03-xx - xxxxxxxxxx - lavu 59.2.100 - channel_layout.h
>>>>    Add AV_CHANNEL_LAYOUT_RETYPE_FLAG_CANONICAL.
>>>>
>>>> diff --git a/libavutil/Makefile b/libavutil/Makefile
>>>> index e7709b97d0..5e75aa1855 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 0000000000..06fb47e94c
>>>> --- /dev/null
>>>> +++ b/libavutil/timestamp.c
>>>> @@ -0,0 +1,33 @@
>>>> +/*
>>>> + * 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
>>>> + * License along with FFmpeg; if not, write to the Free Software
>>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>>>> 02110-1301 USA
>>>> + */
>>>> +
>>>> +#include "timestamp.h"
>>>> +
>>>> +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 {
>>>> +        int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.6f",
>>>> av_q2d(*tb) * ts);
>>>> +        last = FFMIN(last, AV_TS_MAX_STRING_SIZE - 1) - 1;
>>>> +        for (; last && buf[last] == '0'; last--);
>>>> +        for (; last && buf[last] != 'f' && (buf[last] < '0' ||
>>>> buf[0] > '9'); last--);
>>>> +        buf[last + 1] = '\0';
>>>> +    }
>>>> +    return buf;
>>>> +}
>>>
>>> As has already been said before: Simply using %.6f will discard
>>> significant digits for small timestamps. E.g. 10^-7 will simply print
>>> 0.000000, which will then be converted to "0" by your code. The old code
>>> printed 1.
> 
> Admittedly we will lose precision < 10^-6 by using %.6f, but I don't
> think this will cause any real problems, so I'd rather just leave this
> as is. Or you prefer some special handling of small values? E.g. using
> %.9f or something?
> 

Yeah, special casing small values seems good. Or just keep the original
%.6g for small values.

>>>
>>> Furthermore, there are more problems:
>>> "A double argument representing an infinity is converted in one of the
>>> styles [-]inf or [-]infinity — which style is implementation-defined."
>>> Your code would trim infinity to "inf". It would be even worse with nan:
>>> "A double argument representing a NaN is converted in one of the styles
>>> [-]nan or [-]nan(n-char-sequence) — which style, and the meaning of
>>> any n-char-sequence, is implementation-defined."
>>> Furthermore, there is no guarantee that the "decimal-point character" is
>>> actually a single character (it's a char* in struct lcov after all).
>>>
>>> If I am not mistaken, then NANs and infinities can't happen here unless
>>> the timebase is malformed (denominator zero); and in this case one could
>>> use the NOPTS codepath.
> 
> So the NAN case can't happen. The multiple character decimal separator
> is not a problem because we are trimming multiple chars. The inf instead
> of infinity case should not be a problem either, inf is parseable the
> same way as infinity, strtod() supports both. But if you prefer, I can
> make the code look for 'y' as well to stop trimming, I don't think it
> really matters. I am not sure about returning NOPTS for infinity.
> 
>> While just at it: As I already mentioned, this function has a design
>> defect: It passes the timebase by pointer and not just by value. So if
>> we add a non-inlined version of this, it should be a new function that
>> receives the timebase by value and av_ts_make_time_string() should
>> instead call this new function like
>> static inline char *av_ts_make_time_string(char *buf, int64_t ts, const
>> AVRational *tb)
>> {
>>    return new_func(buf, ts, *tb);
>> }
> 
> I am uneasy about adding another public function signature for this
> purpose, because that will still have the flaw of using a locale
> dependant decimal point.
> 

I do not really get this point here. Just because it has a flaw (I'm not
certain everyone sees the locale dependcy as a flaw) means that continue
another flaw.

- Andreas