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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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

_______________________________________________
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".