[FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files

[FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files

[Allan Cady via ffmpeg-devel]

When the silencedetect audio filter is run against long files, the
output timestamps gradually lose precision as the scan proceeds further
into the file. This is because the output format specifier ("%.6g" in
libavutil/timestamp.h) limits the total field width to six significant
digits. As the offset into the file increases, digits drop off the end,
until eventually, for offsets greater than 100000 seconds (about 28
hours), fractions of a second disappear altogether, and the timestamps
are logged as whole seconds.

This patch changes the format to "%.6f" for silencedetect, which will
give microsecond precision for all timestamps regardless of offset.

libavutil/timestamp.h exposes a macro, av_ts2timestr, as the public
interface. This macro was used by silencedetect.c, as well as other
source files. In order to fix the issue for silencedetect without
affecting other files and tests, I have added a new macro,
av_ts2timestr_fixed_precision, which uses the new format specifier.
The original av_ts_make_time_string remains, with the original
behavior.

timestamp.h also exposes a function, av_ts_make_time_string, which
was called only from av_ts2timestr. After this patch, both of the
macros now use a new function, av_ts_make_time_string_format, which
takes a format specifier as an argument, so the function
av_ts_make_time_string is no longer used. I've left it in place, but
flagged it for deprecation with FF_API_AV_MAKETIMESTRING.

The test reference file filter-metadata-silencedetect has been updated
to match the new functionality.

Signed-off-by: Allan Cady <allancady@yahoo.com>
---
 libavfilter/af_silencedetect.c               | 12 +++----
 libavutil/timestamp.h                        | 34 +++++++++++++++++---
 libavutil/version.h                          |  1 +
 tests/ref/fate/filter-metadata-silencedetect |  2 +-
 4 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/libavfilter/af_silencedetect.c b/libavfilter/af_silencedetect.c
index 845c65bfed..f1a8096540 100644
--- a/libavfilter/af_silencedetect.c
+++ b/libavfilter/af_silencedetect.c
@@ -86,11 +86,11 @@ static av_always_inline void update(SilenceDetectContext *s, AVFrame *insamples,
                 s->start[channel] = insamples->pts + av_rescale_q(current_sample / s->channels + 1 - nb_samples_notify * s->independent_channels / s->channels,
                         (AVRational){ 1, s->last_sample_rate }, time_base);
                 set_meta(insamples, s->mono ? channel + 1 : 0, "silence_start",
-                        av_ts2timestr(s->start[channel], &time_base));
+                        av_ts2timestr_fixed_precision(s->start[channel], &time_base));
                 if (s->mono)
                     av_log(s, AV_LOG_INFO, "channel: %d | ", channel);
                 av_log(s, AV_LOG_INFO, "silence_start: %s\n",
-                        av_ts2timestr(s->start[channel], &time_base));
+                        av_ts2timestr_fixed_precision(s->start[channel], &time_base));
             }
         }
     } else {
@@ -101,15 +101,15 @@ static av_always_inline void update(SilenceDetectContext *s, AVFrame *insamples,
             int64_t duration_ts = end_pts - s->start[channel];
             if (insamples) {
                 set_meta(insamples, s->mono ? channel + 1 : 0, "silence_end",
-                        av_ts2timestr(end_pts, &time_base));
+                        av_ts2timestr_fixed_precision(end_pts, &time_base));
                 set_meta(insamples, s->mono ? channel + 1 : 0, "silence_duration",
-                        av_ts2timestr(duration_ts, &time_base));
+                        av_ts2timestr_fixed_precision(duration_ts, &time_base));
             }
             if (s->mono)
                 av_log(s, AV_LOG_INFO, "channel: %d | ", channel);
             av_log(s, AV_LOG_INFO, "silence_end: %s | silence_duration: %s\n",
-                    av_ts2timestr(end_pts, &time_base),
-                    av_ts2timestr(duration_ts, &time_base));
+                    av_ts2timestr_fixed_precision(end_pts, &time_base),
+                    av_ts2timestr_fixed_precision(duration_ts, &time_base));
         }
         s->nb_null_samples[channel] = 0;
         s->start[channel] = INT64_MIN;
diff --git a/libavutil/timestamp.h b/libavutil/timestamp.h
index 9ae64da8a1..b483b5e12d 100644
--- a/libavutil/timestamp.h
+++ b/libavutil/timestamp.h
@@ -31,6 +31,8 @@
 #endif
 
 #define AV_TS_MAX_STRING_SIZE 32
+#define AV_TS_FMT_DEFAULT "%.6g"
+#define AV_TS_FMT_FIXED_PRECISION_6 "%.6f"
 
 /**
  * Fill the provided buffer with a string containing a timestamp
@@ -53,9 +55,14 @@ static inline char *av_ts_make_string(char *buf, int64_t ts)
  */
 #define av_ts2str(ts) av_ts_make_string((char[AV_TS_MAX_STRING_SIZE]){0}, ts)
 
+#if FF_API_AV_MAKETIMESTRING
 /**
+ * This function is probably deprecated. It was originally called by
+ * av_ts_make_time_string defined below, which now uses av_ts_make_time_string_format.
+ * instead. Nothing external references this function directly.
+ *
  * Fill the provided buffer with a string containing a timestamp time
- * representation.
+ * representation, using the default format AV_TS_FMT_DEFAULT.
  *
  * @param buf a buffer with size in bytes of at least AV_TS_MAX_STRING_SIZE
  * @param ts the timestamp to represent
@@ -65,14 +72,33 @@ 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, 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);
+    else                      snprintf(buf, AV_TS_MAX_STRING_SIZE, AV_TS_FMT_DEFAULT, av_q2d(*tb) * ts);
     return buf;
 }
+#endif
 
 /**
- * Convenience macro, the return value should be used only directly in
+ * Fill the provided buffer with a string containing a timestamp time
+ * representation.
+ *
+ * @param buf a buffer with size in bytes of at least AV_TS_MAX_STRING_SIZE
+ * @param ts the timestamp to represent
+ * @param tb the timebase of the timestamp
+ * @param format format string for timestamp output, e.g. AV_TS_FMT_DEFAULT.
+ * @return the buffer in input
+ */
+static inline char *av_ts_make_time_string_format(char *buf, int64_t ts, AVRational *tb, const char *format)
+{
+    if (ts == AV_NOPTS_VALUE) snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
+    else                      snprintf(buf, AV_TS_MAX_STRING_SIZE, format, av_q2d(*tb) * ts);
+    return buf;
+}
+
+/**
+ * Convenience macros, the return values should be used only directly in
  * function arguments but never stand-alone.
  */
-#define av_ts2timestr(ts, tb) av_ts_make_time_string((char[AV_TS_MAX_STRING_SIZE]){0}, ts, tb)
+#define av_ts2timestr(ts, tb) av_ts_make_time_string_format((char[AV_TS_MAX_STRING_SIZE]){0}, ts, tb, AV_TS_FMT_DEFAULT)
+#define av_ts2timestr_fixed_precision(ts, tb) av_ts_make_time_string_format((char[AV_TS_MAX_STRING_SIZE]){0}, ts, tb, AV_TS_FMT_FIXED_PRECISION_6)
 
 #endif /* AVUTIL_TIMESTAMP_H */
diff --git a/libavutil/version.h b/libavutil/version.h
index 9f45af93df..a2daeda772 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -119,6 +119,7 @@
 #define FF_API_FRAME_KEY                (LIBAVUTIL_VERSION_MAJOR < 59)
 #define FF_API_PALETTE_HAS_CHANGED      (LIBAVUTIL_VERSION_MAJOR < 59)
 #define FF_API_VULKAN_CONTIGUOUS_MEMORY (LIBAVUTIL_VERSION_MAJOR < 59)
+#define FF_API_AV_MAKETIMESTRING        (LIBAVUTIL_VERSION_MAJOR < 59)
 
 /**
  * @}
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.25.1

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

Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files

[Marton Balint]

On Tue, 20 Feb 2024, Allan Cady via ffmpeg-devel wrote:

> When the silencedetect audio filter is run against long files, the
> output timestamps gradually lose precision as the scan proceeds further
> into the file. This is because the output format specifier ("%.6g" in
> libavutil/timestamp.h) limits the total field width to six significant
> digits. As the offset into the file increases, digits drop off the end,
> until eventually, for offsets greater than 100000 seconds (about 28
> hours), fractions of a second disappear altogether, and the timestamps
> are logged as whole seconds.
>
> This patch changes the format to "%.6f" for silencedetect, which will
> give microsecond precision for all timestamps regardless of offset.
>
> libavutil/timestamp.h exposes a macro, av_ts2timestr, as the public
> interface. This macro was used by silencedetect.c, as well as other
> source files. In order to fix the issue for silencedetect without
> affecting other files and tests, I have added a new macro,
> av_ts2timestr_fixed_precision, which uses the new format specifier.
> The original av_ts_make_time_string remains, with the original
> behavior.

I'd rather just to fix av_ts_make_string to not limit the number of 
significant digits. Something like:

1) Print the number in decimal notation with at most 6 fractional digits. 
2) Use less fractional digits if the first format would not fit into 
AV_TS_MAX_STRING_SIZE.
3) Use scientific notation if the second format would not fit into 
AV_TS_MAX_STRINT_SIZE.

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files

[Allan Cady via ffmpeg-devel]

I had a similar thought, as all timestamps would have the same issue.

This is my first contribution here, and I don't know the code very well, so I was being cautious. I'm open to expanding the scope, but I'm sure I would need some help doing it right and not breaking things.

For starters, I'm curious why there are two functions & macros:

av_ts2str/av_ts_make_string (which used "%" format specifier)
av_ts2timestr/av_ts_make_time_string (which used "%6g")

Do you know the rationale for that? I see that only av_ts2timestr is used in silencedetect.c.

And are you suggesting we should fold those two functions into one?

I did notice something in the output from silencedetect. After I made my change, I see the output is now:


frame:92404 pts:53224175 pts_time:2413.79
lavfi.silence_start=2411.120272
frame:92411 pts:53228207 pts_time:2413.98
lavfi.silence_end=2413.992744
lavfi.silence_duration=2.872472


I see that the pts_time values still have the original formatting. I don't know what pts_time is, or where those lines are coming from. Seems like maybe those should have fixed precision as well.

Guidance for a noob please? Thanks.

(P.S. Can you tell me, when I reply to the list (as opposed to patch submission using git send-email), how should I address the email? Obviously it should go to ffmpeg-devel@ffmpeg.org, but should I include you as a recipient, or as a cc:, or only to the list? or is there some other way it gets directed to you? Any other guidance on how to format email? Thanks.)




On Wednesday, February 21, 2024 at 12:25:23 PM PST, Marton Balint <cus@passwd.hu> wrote: 

On Tue, 20 Feb 2024, Allan Cady via ffmpeg-devel wrote:

> When the silencedetect audio filter is run against long files, the
> output timestamps gradually lose precision as the scan proceeds further
> into the file. This is because the output format specifier ("%.6g" in
> libavutil/timestamp.h) limits the total field width to six significant
> digits. As the offset into the file increases, digits drop off the end,
> until eventually, for offsets greater than 100000 seconds (about 28
> hours), fractions of a second disappear altogether, and the timestamps
> are logged as whole seconds.
>
> This patch changes the format to "%.6f" for silencedetect, which will
> give microsecond precision for all timestamps regardless of offset.
>
> libavutil/timestamp.h exposes a macro, av_ts2timestr, as the public
> interface. This macro was used by silencedetect.c, as well as other
> source files. In order to fix the issue for silencedetect without
> affecting other files and tests, I have added a new macro,
> av_ts2timestr_fixed_precision, which uses the new format specifier.
> The original av_ts_make_time_string remains, with the original
> behavior.

I'd rather just to fix av_ts_make_string to not limit the number of 
significant digits. Something like:

1) Print the number in decimal notation with at most 6 fractional digits. 
2) Use less fractional digits if the first format would not fit into 
AV_TS_MAX_STRING_SIZE.
3) Use scientific notation if the second format would not fit into 
AV_TS_MAX_STRINT_SIZE.

Regards,
Marton

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files

[Marton Balint]

On Thu, 22 Feb 2024, Allan Cady via ffmpeg-devel wrote:

> I had a similar thought, as all timestamps would have the same issue.
>
> This is my first contribution here, and I don't know the code very well, 
> so I was being cautious. I'm open to expanding the scope, but I'm sure I 
> would need some help doing it right and not breaking things.
>
> For starters, I'm curious why there are two functions & macros:
>
> av_ts2str/av_ts_make_string (which used "%" format specifier)

That takes a 64-bit integer timestamp and is actually using "%"PRId64 
because that is the correct (portable) format string for an int64_t 
variable.

> av_ts2timestr/av_ts_make_time_string (which used "%6g")

That takes an integer timestamp and a rational time base. Float timestamps 
(in seconds) is calculated by multiplying the two, that is what is 
printed.

>
> Do you know the rationale for that? I see that only av_ts2timestr is used in silencedetect.c.
>
> And are you suggesting we should fold those two functions into one?

No, they have different purpose. The first prints out a timestamps which 
can be in any time base. The second prints out a timestamp in seconds.

>
> I did notice something in the output from silencedetect. After I made my 
> change, I see the output is now:
>
>
> frame:92404 pts:53224175 pts_time:2413.79
> lavfi.silence_start=2411.120272
> frame:92411 pts:53228207 pts_time:2413.98
> lavfi.silence_end=2413.992744
> lavfi.silence_duration=2.872472
>
>
> I see that the pts_time values still have the original formatting. I 
> don't know what pts_time is, or where those lines are coming from. Seems 
> like maybe those should have fixed precision as well.

Well, that is likely using the same function, but you only fixed 
silencedetect, right?

>
> Guidance for a noob please? Thanks.
>
> (P.S. Can you tell me, when I reply to the list (as opposed to patch 
> submission using git send-email), how should I address the email? 
> Obviously it should go to ffmpeg-devel@ffmpeg.org, but should I include 
> you as a recipient, or as a cc:, or only to the list? or is there some 
> other way it gets directed to you? Any other guidance on how to format 
> email? Thanks.)

I don't think there is a rule, I have seen it happen both with or without 
CC. You don't need to CC me though, as I am a regular on the list, but 
others may have other preference.

Regards,
Marton

PS: Please avoid top positing in you replies.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files

[Allan Cady via ffmpeg-devel]

  On Thursday, February 22, 2024 at 01:16:19 AM PST, Marton Balint <cus@passwd.hu> wrote:
>> For starters, I'm curious why there are two functions & macros:>>>> av_ts2str/av_ts_make_string (which used "%" format specifier)> > That takes a 64-bit integer timestamp and is actually using "%"PRId64 > because that is the correct (portable) format string for an int64_t > variable.> >> av_ts2timestr/av_ts_make_time_string (which used "%6g")> > That takes an integer timestamp and a rational time base. Float timestamps > (in seconds) is calculated by multiplying the two, that is what is > printed.> >>>> Do you know the rationale for that? I see that only av_ts2timestr is used in silencedetect.c.>>>> And are you suggesting we should fold those two functions into one?> > No, they have different purpose. The first prints out a timestamps which > can be in any time base. The second prints out a timestamp in seconds.
Understood, I think I'm up to speed now. av_ts2str prints a 64-bit integer timestamp;av_ts2timestr prints a floating point timestamp in seconds with the timebase applied.
In your previous email, you said:
> I'd rather just to fix av_ts_make_string to not limit the number of > significant digits. Something like:>> 1) Print the number in decimal notation with at most 6 fractional digits. > 2) Use less fractional digits if the first format would not fit into > AV_TS_MAX_STRING_SIZE.> 3) Use scientific notation if the second format would not fit into > AV_TS_MAX_STRINT_SIZE.
I think you probably meant to say "I'd rather just to fixav_ts_make_time_string" (not av_ts_make_string)?Since it's av_ts_make_time_string() that's formatting floating point.
So it makes sense to me to make the change to av_ts_make_time_string()for all timestamps, as you suggest. As for how specifically to format them,
I'm fine with whatever you think is best, and I'm happy to work on this, but theimplementation has me a bit stumped for the moment, and I may need somehelp with it. My C language skills are rusty to say the least.
It also occurs to me to wonder, would this warrant a formal problem ticket?
I haven't looked into how that works for ffmpeg.

Thanks for your help.


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files

[Allan Cady via ffmpeg-devel]

[Apologies for the awful mess in the previous email. Trying again with straight text.]

On Thursday, February 22, 2024 at 01:16:19 AM PST, Marton Balint <cus@passwd.hu> wrote:

>> For starters, I'm curious why there are two functions & macros:
>>
>> av_ts2str/av_ts_make_string (which used "%" format specifier)
> 
> That takes a 64-bit integer timestamp and is actually using "%"PRId64 
> because that is the correct (portable) format string for an int64_t 
> variable.
> 
>> av_ts2timestr/av_ts_make_time_string (which used "%6g")
> 
> That takes an integer timestamp and a rational time base. Float timestamps 
> (in seconds) is calculated by multiplying the two, that is what is 
> printed.
> 
>>
>> Do you know the rationale for that? I see that only av_ts2timestr is used in silencedetect.c.
>>
>> And are you suggesting we should fold those two functions into one?
> 
> No, they have different purpose. The first prints out a timestamps which 
> can be in any time base. The second prints out a timestamp in seconds.


Understood, I think I'm up to speed now. av_ts2str prints a 64-bit integer timestamp;
av_ts2timestr prints a floating point timestamp in seconds with the timebase applied.


In your previous email, you said:


> I'd rather just to fix av_ts_make_string to not limit the number of 
> significant digits. Something like:
>
> 1) Print the number in decimal notation with at most 6 fractional digits. 
> 2) Use less fractional digits if the first format would not fit into 
> AV_TS_MAX_STRING_SIZE.
> 3) Use scientific notation if the second format would not fit into 
> AV_TS_MAX_STRINT_SIZE.


I think you probably meant to say "I'd rather just to fix
av_ts_make_time_string" (not av_ts_make_string)?
Since it's av_ts_make_time_string() that's formatting floating point.


So it makes sense to me to make the change to av_ts_make_time_string()
for all timestamps, as you suggest. As for how specifically to format them,
I'm fine with whatever you think is best, and I'm happy to work on this, but the
implementation has me a bit stumped for the moment, and I may need some
help with it. My C language skills are rusty to say the least.

It also occurs to me to wonder, would this warrant a formal problem ticket?
I haven't looked into how that works for ffmpeg.

Thanks for your help.




_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files

[Marton Balint]

On Fri, 23 Feb 2024, Allan Cady via ffmpeg-devel wrote:

> [Apologies for the awful mess in the previous email. Trying again with straight text.]
>
> On Thursday, February 22, 2024 at 01:16:19 AM PST, Marton Balint <cus@passwd.hu> wrote:
>
>>> For starters, I'm curious why there are two functions & macros:
>>>
>>> av_ts2str/av_ts_make_string (which used "%" format specifier)
>>  
>> That takes a 64-bit integer timestamp and is actually using "%"PRId64 
>> because that is the correct (portable) format string for an int64_t 
>> variable.
>>  
>>> av_ts2timestr/av_ts_make_time_string (which used "%6g")
>>  
>> That takes an integer timestamp and a rational time base. Float timestamps 
>> (in seconds) is calculated by multiplying the two, that is what is 
>> printed.
>>  
>>>
>>> Do you know the rationale for that? I see that only av_ts2timestr is used in silencedetect.c.
>>>
>>> And are you suggesting we should fold those two functions into one?
>>  
>> No, they have different purpose. The first prints out a timestamps which 
>> can be in any time base. The second prints out a timestamp in seconds.
>
>
> Understood, I think I'm up to speed now. av_ts2str prints a 64-bit integer timestamp;
> av_ts2timestr prints a floating point timestamp in seconds with the timebase applied.
>
>
> In your previous email, you said:
>
>
>> I'd rather just to fix av_ts_make_string to not limit the number of 
>> significant digits. Something like:
>>
>> 1) Print the number in decimal notation with at most 6 fractional digits. 
>> 2) Use less fractional digits if the first format would not fit into 
>> AV_TS_MAX_STRING_SIZE.
>> 3) Use scientific notation if the second format would not fit into 
>> AV_TS_MAX_STRINT_SIZE.
>
>
> I think you probably meant to say "I'd rather just to fix
> av_ts_make_time_string" (not av_ts_make_string)?
> Since it's av_ts_make_time_string() that's formatting floating point.

Yes, indeed.

>
> So it makes sense to me to make the change to av_ts_make_time_string()
> for all timestamps, as you suggest. As for how specifically to format them,
> I'm fine with whatever you think is best, and I'm happy to work on this, but the
> implementation has me a bit stumped for the moment, and I may need some
> help with it. My C language skills are rusty to say the least.

The simplest way is to try snprintf() with 6 fractional digits, and check 
the return value to see how long the string would become.
Based on this you can either accept snprintf result and truncate 
trailing zeros and dots, or try snprintf() with less fractional digits and 
truncate, or fall back to scientific format.

>
> It also occurs to me to wonder, would this warrant a formal problem ticket?
> I haven't looked into how that works for ffmpeg.

Opening a ticket for the problem is not required, but if your patch 
fixes an existing ticket, then you should mention the ticket number
in the commit message.

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] When the silencedetect filter is run against long files, the output timestamps gradually lose precision as the scan proceeds further into the file. This is because the output is formatted (in libavutil/timestamp.h) as "%.6g", which limits the total field length. Eventually, for offsets greater than 100000 seconds (about 28 hours), fractions of a second disappear altogether, and the timestamps are logged as whole seconds.

[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;
 }
 
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.34.1

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

Re: [FFmpeg-devel] [PATCH] When the silencedetect filter is run against long files, the output timestamps gradually lose precision as the scan proceeds further into the file. This is because the output is formatted (in libavutil/timestamp.h) as "%.6g", which limits the total field length. Eventually, for offsets greater than 100000 seconds (about 28 hours), fractions of a second disappear altogether, and the timestamps are logged as whole seconds.

[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.

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

Re: [FFmpeg-devel] [PATCH] When the silencedetect filter is run against long files, the output timestamps gradually lose precision as the scan proceeds further into the file. This is because the output is formatted (in libavutil/timestamp.h) as "%.6g", which limits the total field length. Eventually, for offsets greater than 100000 seconds (about 28 hours), fractions of a second disappear altogether, and the timestamps are logged as whole seconds.

[Andreas Rheinhardt]

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

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

Re: [FFmpeg-devel] [PATCH] When the silencedetect filter is run against long files, the output timestamps gradually lose precision as the scan proceeds further into the file. This is because the output is formatted (in libavutil/timestamp.h) as "%.6g", which limits the total field length. Eventually, for offsets greater than 100000 seconds (about 28 hours), fractions of a second disappear altogether, and the timestamps are logged as whole seconds.

[epirat07]

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

Re: [FFmpeg-devel] [libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files]

[Allan Cady via ffmpeg-devel]

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

Re: [FFmpeg-devel] [PATCH] change av_ts_make_time_string precision

[Marton Balint]

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

Re: [FFmpeg-devel] [PATCH] change av_ts_make_time_string precision

[Allan Cady via ffmpeg-devel]

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

Re: [FFmpeg-devel] [PATCH] change av_ts_make_time_string precision

[Marton Balint]

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

Re: [FFmpeg-devel] [PATCH] change av_ts_make_time_string precision

[Allan Cady via ffmpeg-devel]

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.

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

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

[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;
+}
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".

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

[Marton Balint]

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

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

[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.

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

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

[Andreas Rheinhardt]

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

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

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

[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?

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

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

[Marton Balint]

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

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

[Andreas Rheinhardt]

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

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

[FFmpeg-devel] [PATCH v2 1/2] avutil/timestamp: introduce av_ts_make_time_string2 for better precision

[Marton Balint]

av_ts_make_time_string() used "%.6g" format, but this format was losing
precision even when the timestamp to be printed was not that large. For example
for 3 hours (10800) seconds, only 1 decimal digit was printed, which made this
format inaccurate when it was used in e.g. the silencedetect filter. Other
detection filters printing timestamps had similar issues. Also time base
parameter of the function was *AVRational instead of AVRational.

Resolve these problems by introducing a new function, av_ts_make_time_string2().

We change the used format to "%.*f", use a precision of 6, except when printing
values near 0, in which case we calculate the precision dynamically to aim for
a similar precision in normal form as with %.6g.  No longer using scientific
representation can make parsing the timestamp easier for the users, we can
safely do this because the theoretical maximum of INT64_MAX*INT32_MAX still
fits into the string buffer in normal form.

We somewhat imitate %g by trimming ending zeroes and the potential decimal
point characters. In order not to trim "inf" as well, we assume that the
decimal point string does not contain the letter "f". Note that depending on
printf %f implementation, we might trim "infinity" to "inf".

Thanks for Allan Cady for bringing up this issue.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 doc/APIchanges        |  5 +++++
 libavutil/Makefile    |  1 +
 libavutil/timestamp.c | 36 ++++++++++++++++++++++++++++++++++++
 libavutil/timestamp.h |  8 ++++++++
 libavutil/version.h   |  2 +-
 5 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 libavutil/timestamp.c

diff --git a/doc/APIchanges b/doc/APIchanges
index 2796b4d0c2..9c6fa381e1 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -2,6 +2,11 @@ The last version increases of all libraries were on 2024-03-07
 
 API changes, most recent first:
 
+2024-03-xx - xxxxxxxxxx - lavu 59.5.100 - timestamp.h
+  Add av_ts_make_time_string2() for better timestamp precision, the new
+  function accepts AVRational as time base instead of *AVRational, and is not
+  an inline function like its predecessor.
+
 2024-03-xx - xxxxxxxxxx - lavc 61.3.100 - jni.h
   Add av_jni_set_android_app_ctx() and av_jni_get_android_app_ctx().
 
diff --git a/libavutil/Makefile b/libavutil/Makefile
index 008fcfcd9c..6e6fa8d800 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -174,6 +174,7 @@ OBJS = adler32.o                                                        \
        threadmessage.o                                                  \
        time.o                                                           \
        timecode.o                                                       \
+       timestamp.o                                                      \
        tree.o                                                           \
        twofish.o                                                        \
        utils.o                                                          \
diff --git a/libavutil/timestamp.c b/libavutil/timestamp.c
new file mode 100644
index 0000000000..2a3e3012a4
--- /dev/null
+++ b/libavutil/timestamp.c
@@ -0,0 +1,36 @@
+/*
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * 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_string2(char *buf, int64_t ts, AVRational tb)
+{
+    if (ts == AV_NOPTS_VALUE) {
+        snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
+    } else {
+        double val = av_q2d(tb) * ts;
+        double log = floor(log10(fabs(val)));
+        int precision = (isfinite(log) && log < 0) ? -log + 5 : 6;
+        int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.*f", precision, val);
+        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..7e6da894df 100644
--- a/libavutil/timestamp.h
+++ b/libavutil/timestamp.h
@@ -62,6 +62,14 @@ static inline char *av_ts_make_string(char *buf, int64_t ts)
  * @param tb the timebase of the timestamp
  * @return the buffer in input
  */
+char *av_ts_make_time_string2(char *buf, int64_t ts, AVRational tb);
+
+/**
+ * Fill the provided buffer with a string containing a timestamp
+ * representation.
+ *
+ * @see av_ts_make_time_string2
+ */
 static inline char *av_ts_make_time_string(char *buf, int64_t ts,
                                            const AVRational *tb)
 {
diff --git a/libavutil/version.h b/libavutil/version.h
index 882003f719..8a1ecd4451 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  59
-#define LIBAVUTIL_VERSION_MINOR   4
+#define LIBAVUTIL_VERSION_MINOR   5
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
-- 
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".

[FFmpeg-devel] [PATCH v2 2/2] avutil/timestamp: change precision of av_ts_make_time_string()

[Marton Balint]

By calling the av_ts_make_time_string2() from the function we can fix the
precision issue.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavutil/timestamp.h                        |  4 +---
 tests/ref/fate/filter-metadata-scdet         | 12 ++++++------
 tests/ref/fate/filter-metadata-silencedetect |  2 +-
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/libavutil/timestamp.h b/libavutil/timestamp.h
index 7e6da894df..fa53a46b98 100644
--- a/libavutil/timestamp.h
+++ b/libavutil/timestamp.h
@@ -73,9 +73,7 @@ char *av_ts_make_time_string2(char *buf, int64_t ts, AVRational tb);
 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;
+    return av_ts_make_time_string2(buf, ts, *tb);
 }
 
 /**
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".

Re: [FFmpeg-devel] [PATCH v2 1/2] avutil/timestamp: introduce av_ts_make_time_string2 for better precision

[Marton Balint]

On Sat, 23 Mar 2024, Marton Balint wrote:

> av_ts_make_time_string() used "%.6g" format, but this format was losing
> precision even when the timestamp to be printed was not that large. For example
> for 3 hours (10800) seconds, only 1 decimal digit was printed, which made this
> format inaccurate when it was used in e.g. the silencedetect filter. Other
> detection filters printing timestamps had similar issues. Also time base
> parameter of the function was *AVRational instead of AVRational.
>
> Resolve these problems by introducing a new function, av_ts_make_time_string2().
>
> We change the used format to "%.*f", use a precision of 6, except when printing
> values near 0, in which case we calculate the precision dynamically to aim for
> a similar precision in normal form as with %.6g.  No longer using scientific
> representation can make parsing the timestamp easier for the users, we can
> safely do this because the theoretical maximum of INT64_MAX*INT32_MAX still
> fits into the string buffer in normal form.
>
> We somewhat imitate %g by trimming ending zeroes and the potential decimal
> point characters. In order not to trim "inf" as well, we assume that the
> decimal point string does not contain the letter "f". Note that depending on
> printf %f implementation, we might trim "infinity" to "inf".
>
> Thanks for Allan Cady for bringing up this issue.

Will apply the series, so it can make it into 7.0.

Regards,
Marton

>
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
> doc/APIchanges        |  5 +++++
> libavutil/Makefile    |  1 +
> libavutil/timestamp.c | 36 ++++++++++++++++++++++++++++++++++++
> libavutil/timestamp.h |  8 ++++++++
> libavutil/version.h   |  2 +-
> 5 files changed, 51 insertions(+), 1 deletion(-)
> create mode 100644 libavutil/timestamp.c
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 2796b4d0c2..9c6fa381e1 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,11 @@ The last version increases of all libraries were on 2024-03-07
>
> API changes, most recent first:
>
> +2024-03-xx - xxxxxxxxxx - lavu 59.5.100 - timestamp.h
> +  Add av_ts_make_time_string2() for better timestamp precision, the new
> +  function accepts AVRational as time base instead of *AVRational, and is not
> +  an inline function like its predecessor.
> +
> 2024-03-xx - xxxxxxxxxx - lavc 61.3.100 - jni.h
>   Add av_jni_set_android_app_ctx() and av_jni_get_android_app_ctx().
>
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index 008fcfcd9c..6e6fa8d800 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -174,6 +174,7 @@ OBJS = adler32.o                                                        \
>        threadmessage.o                                                  \
>        time.o                                                           \
>        timecode.o                                                       \
> +       timestamp.o                                                      \
>        tree.o                                                           \
>        twofish.o                                                        \
>        utils.o                                                          \
> diff --git a/libavutil/timestamp.c b/libavutil/timestamp.c
> new file mode 100644
> index 0000000000..2a3e3012a4
> --- /dev/null
> +++ b/libavutil/timestamp.c
> @@ -0,0 +1,36 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * 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_string2(char *buf, int64_t ts, AVRational tb)
> +{
> +    if (ts == AV_NOPTS_VALUE) {
> +        snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
> +    } else {
> +        double val = av_q2d(tb) * ts;
> +        double log = floor(log10(fabs(val)));
> +        int precision = (isfinite(log) && log < 0) ? -log + 5 : 6;
> +        int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.*f", precision, val);
> +        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..7e6da894df 100644
> --- a/libavutil/timestamp.h
> +++ b/libavutil/timestamp.h
> @@ -62,6 +62,14 @@ static inline char *av_ts_make_string(char *buf, int64_t ts)
>  * @param tb the timebase of the timestamp
>  * @return the buffer in input
>  */
> +char *av_ts_make_time_string2(char *buf, int64_t ts, AVRational tb);
> +
> +/**
> + * Fill the provided buffer with a string containing a timestamp
> + * representation.
> + *
> + * @see av_ts_make_time_string2
> + */
> static inline char *av_ts_make_time_string(char *buf, int64_t ts,
>                                            const AVRational *tb)
> {
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 882003f719..8a1ecd4451 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>  */
>
> #define LIBAVUTIL_VERSION_MAJOR  59
> -#define LIBAVUTIL_VERSION_MINOR   4
> +#define LIBAVUTIL_VERSION_MINOR   5
> #define LIBAVUTIL_VERSION_MICRO 100
>
> #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
> -- 
> 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".
>
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2 1/2] avutil/timestamp: introduce av_ts_make_time_string2 for better precision

[Allan Cady via ffmpeg-devel]

> On Sat, 23 Mar 2024, Marton Balint wrote:


>> av_ts_make_time_string() used "%.6g" format, but this format was losing
>> precision even when the timestamp to be printed was not that large. For example
>> for 3 hours (10800) seconds, only 1 decimal digit was printed, which made this
>> format inaccurate when it was used in e.g. the silencedetect filter. Other
>> detection filters printing timestamps had similar issues. Also time base
>> parameter of the function was *AVRational instead of AVRational.
>>
>> Resolve these problems by introducing a new function, av_ts_make_time_string2().
>>
>> We change the used format to "%.*f", use a precision of 6, except when printing
>> values near 0, in which case we calculate the precision dynamically to aim for
>> a similar precision in normal form as with %.6g.  No longer using scientific
>> representation can make parsing the timestamp easier for the users, we can
>> safely do this because the theoretical maximum of INT64_MAX*INT32_MAX still
>> fits into the string buffer in normal form.
>>
>> We somewhat imitate %g by trimming ending zeroes and the potential decimal
>> point characters. In order not to trim "inf" as well, we assume that the
>> decimal point string does not contain the letter "f". Note that depending on
>> printf %f implementation, we might trim "infinity" to "inf".
>>
>> Thanks for Allan Cady for bringing up this issue.

> Will apply the series, so it can make it into 7.0.


> Regards,
> Marton


This looks great. I ran the patch against my test program with a couple
dozen vectors of inputs, and it looks perfect to me.


Thanks to both of you Marton and Andreas for seeing this through. I
really appreciate it.


Allan


>>
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>> doc/APIchanges        |  5 +++++
>> libavutil/Makefile    |  1 +
>> libavutil/timestamp.c | 36 ++++++++++++++++++++++++++++++++++++
>> libavutil/timestamp.h |  8 ++++++++
>> libavutil/version.h  |  2 +-
>> 5 files changed, 51 insertions(+), 1 deletion(-)
>> create mode 100644 libavutil/timestamp.c
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 2796b4d0c2..9c6fa381e1 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -2,6 +2,11 @@ The last version increases of all libraries were on 2024-03-07
>>
>> API changes, most recent first:
>>
>> +2024-03-xx - xxxxxxxxxx - lavu 59.5.100 - timestamp.h
>> +  Add av_ts_make_time_string2() for better timestamp precision, the new
>> +  function accepts AVRational as time base instead of *AVRational, and is not
>> +  an inline function like its predecessor.
>> +
>> 2024-03-xx - xxxxxxxxxx - lavc 61.3.100 - jni.h
>>  Add av_jni_set_android_app_ctx() and av_jni_get_android_app_ctx().
>>
>> diff --git a/libavutil/Makefile b/libavutil/Makefile
>> index 008fcfcd9c..6e6fa8d800 100644
>> --- a/libavutil/Makefile
>> +++ b/libavutil/Makefile
>> @@ -174,6 +174,7 @@ OBJS = adler32.o                                                        \
>>        threadmessage.o                                                  \
>>        time.o                                                          \
>>        timecode.o                                                      \
>> +      timestamp.o                                                      \
>>        tree.o                                                          \
>>        twofish.o                                                        \
>>        utils.o                                                          \
>> diff --git a/libavutil/timestamp.c b/libavutil/timestamp.c
>> new file mode 100644
>> index 0000000000..2a3e3012a4
>> --- /dev/null
>> +++ b/libavutil/timestamp.c
>> @@ -0,0 +1,36 @@
>> +/*
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * 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_string2(char *buf, int64_t ts, AVRational tb)
>> +{
>> +    if (ts == AV_NOPTS_VALUE) {
>> +        snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
>> +    } else {
>> +        double val = av_q2d(tb) * ts;
>> +        double log = floor(log10(fabs(val)));
>> +        int precision = (isfinite(log) && log < 0) ? -log + 5 : 6;
>> +        int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.*f", precision, val);
>> +        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..7e6da894df 100644
>> --- a/libavutil/timestamp.h
>> +++ b/libavutil/timestamp.h
>> @@ -62,6 +62,14 @@ static inline char *av_ts_make_string(char *buf, int64_t ts)
>>  * @param tb the timebase of the timestamp
>>  * @return the buffer in input
>>  */
>> +char *av_ts_make_time_string2(char *buf, int64_t ts, AVRational tb);
>> +
>> +/**
>> + * Fill the provided buffer with a string containing a timestamp
>> + * representation.
>> + *
>> + * @see av_ts_make_time_string2
>> + */
>> static inline char *av_ts_make_time_string(char *buf, int64_t ts,
>>                                            const AVRational *tb)
>> {
>> diff --git a/libavutil/version.h b/libavutil/version.h
>> index 882003f719..8a1ecd4451 100644
>> --- a/libavutil/version.h
>> +++ b/libavutil/version.h
>> @@ -79,7 +79,7 @@
>>  */
>>
>> #define LIBAVUTIL_VERSION_MAJOR  59
>> -#define LIBAVUTIL_VERSION_MINOR  4
>> +#define LIBAVUTIL_VERSION_MINOR  5
>> #define LIBAVUTIL_VERSION_MICRO 100
>>
>> #define LIBAVUTIL_VERSION_INT  AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>> --
>> 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".


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

Re: [FFmpeg-devel] [PATCH v2 1/2] avutil/timestamp: introduce av_ts_make_time_string2 for better precision

[Hendrik Leppkes]

On Sat, Mar 23, 2024 at 12:14 PM Marton Balint <cus@passwd.hu> wrote:
> +char *av_ts_make_time_string2(char *buf, int64_t ts, AVRational tb)
> +{
> +    if (ts == AV_NOPTS_VALUE) {
> +        snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
> +    } else {
> +        double val = av_q2d(tb) * ts;
> +        double log = floor(log10(fabs(val)));

This causes a floating point exception on some systems (div by zero)
if val ends up zero. Can we introduce a check to avoid the FPE?
log10(0) seems to be implementation defined, and may raise a FPE,
which we should avoid.

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

[FFmpeg-devel] [PATCH] avutil/timestamp: avoid possible FPE when 0 is passed to av_ts_make_time_string2()

[Marton Balint]

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavutil/timestamp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavutil/timestamp.c b/libavutil/timestamp.c
index 2a3e3012a4..6c231a517d 100644
--- a/libavutil/timestamp.c
+++ b/libavutil/timestamp.c
@@ -24,7 +24,7 @@ char *av_ts_make_time_string2(char *buf, int64_t ts, AVRational tb)
         snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
     } else {
         double val = av_q2d(tb) * ts;
-        double log = floor(log10(fabs(val)));
+        double log = (fpclassify(val) == FP_ZERO ? -INFINITY : floor(log10(fabs(val))));
         int precision = (isfinite(log) && log < 0) ? -log + 5 : 6;
         int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.*f", precision, val);
         last = FFMIN(last, AV_TS_MAX_STRING_SIZE - 1) - 1;
-- 
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".

Re: [FFmpeg-devel] [PATCH] avutil/timestamp: avoid possible FPE when 0 is passed to av_ts_make_time_string2()

[Rémi Denis-Courmont]

Le 17 juin 2024 22:33:01 GMT+02:00, Marton Balint <cus@passwd.hu> a écrit :
>Signed-off-by: Marton Balint <cus@passwd.hu>
>---
> libavutil/timestamp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/libavutil/timestamp.c b/libavutil/timestamp.c
>index 2a3e3012a4..6c231a517d 100644
>--- a/libavutil/timestamp.c
>+++ b/libavutil/timestamp.c
>@@ -24,7 +24,7 @@ char *av_ts_make_time_string2(char *buf, int64_t ts, AVRational tb)
>         snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
>     } else {
>         double val = av_q2d(tb) * ts;
>-        double log = floor(log10(fabs(val)));
>+        double log = (fpclassify(val) == FP_ZERO ? -INFINITY : floor(log10(fabs(val))));
>         int precision = (isfinite(log) && log < 0) ? -log + 5 : 6;

Can the log be infinite anymore?

>         int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.*f", precision, val);
>         last = FFMIN(last, AV_TS_MAX_STRING_SIZE - 1) - 1;
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avutil/timestamp: avoid possible FPE when 0 is passed to av_ts_make_time_string2()

[Marton Balint]

On Tue, 18 Jun 2024, Rémi Denis-Courmont wrote:

>
>
> Le 17 juin 2024 22:33:01 GMT+02:00, Marton Balint <cus@passwd.hu> a écrit :
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>> libavutil/timestamp.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavutil/timestamp.c b/libavutil/timestamp.c
>> index 2a3e3012a4..6c231a517d 100644
>> --- a/libavutil/timestamp.c
>> +++ b/libavutil/timestamp.c
>> @@ -24,7 +24,7 @@ char *av_ts_make_time_string2(char *buf, int64_t ts, AVRational tb)
>>         snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
>>     } else {
>>         double val = av_q2d(tb) * ts;
>> -        double log = floor(log10(fabs(val)));
>> +        double log = (fpclassify(val) == FP_ZERO ? -INFINITY : floor(log10(fabs(val))));
>>         int precision = (isfinite(log) && log < 0) ? -log + 5 : 6;
>
> Can the log be infinite anymore?

Yes, in case of zero value we explicitly set it to -INFINITY, but implicit 
infinity or NaN is also possible, because av_q2d can return both.

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avutil/timestamp: avoid possible FPE when 0 is passed to av_ts_make_time_string2()

[Marton Balint]

On Tue, 18 Jun 2024, Marton Balint wrote:

>
>
> On Tue, 18 Jun 2024, Rémi Denis-Courmont wrote:
>
>> 
>>
>>  Le 17 juin 2024 22:33:01 GMT+02:00, Marton Balint <cus@passwd.hu> a
>>  écrit :
>>>  Signed-off-by: Marton Balint <cus@passwd.hu>
>>>  ---
>>>  libavutil/timestamp.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>>  diff --git a/libavutil/timestamp.c b/libavutil/timestamp.c
>>>  index 2a3e3012a4..6c231a517d 100644
>>>  --- a/libavutil/timestamp.c
>>>  +++ b/libavutil/timestamp.c
>>>  @@ -24,7 +24,7 @@ char *av_ts_make_time_string2(char *buf, int64_t ts,
>>>  AVRational tb)
>>>          snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
>>>      } else {
>>>          double val = av_q2d(tb) * ts;
>>>  -        double log = floor(log10(fabs(val)));
>>>  +        double log = (fpclassify(val) == FP_ZERO ? -INFINITY :
>>>  floor(log10(fabs(val))));
>>>          int precision = (isfinite(log) && log < 0) ? -log + 5 : 6;
>>
>>  Can the log be infinite anymore?
>
> Yes, in case of zero value we explicitly set it to -INFINITY, but implicit 
> infinity or NaN is also possible, because av_q2d can return both.

Will apply.

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] change av_ts_make_time_string precision

[Marton Balint]

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

Re: [FFmpeg-devel] [PATCH] change av_ts_make_time_string precision

[Allan Cady via ffmpeg-devel]

 
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?

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

Re: [FFmpeg-devel] [PATCH] change av_ts_make_time_string precision

[Allan Cady via ffmpeg-devel]

I've been meaning to ask -- what version of C are we using? I know it's at least 99,
because of the compound literal (had to look that up).
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files

[Allan Cady via 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 the total field
length. Eventually, for offsets greater than 100000 seconds (about 28
hours), fractions of a second disappear altogether, and the
timestamps are logged as whole seconds.

I propose changing the format to "%.6f", which will
give microsecond precision for all timestamps, regardless of
offset. Trailing zeros can be trimmed from the fraction, without
losing precision. If the length of the fixed-precision formatted
timestamp exceeds the length of the allocated buffer
(AV_TS_MAX_STRING_SIZE, currently 32, less one for the
terminating null), then we can fall back to scientific notation, though
this seems almost certain to never occur, because 32 characters would
allow a maximum timestamp value of (32 - 1 - 6 - 1) = 24 characters.
By my calculation, 10^24 seconds is about six orders of magnitude
greater than the age of the universe.

The fix proposed here follows the following logic:

1) Try formatting the number of seconds using "%.6f". This will
always result in a string with six decimal digits in the fraction,
possibly including trailing zeros. (e.g. "897234.73200").

2) Check if that string would overflow the buffer. If it would, then
format it using scientific notation ("%.8g").

3) If the original fixed-point format fits, then trim any trailing
zeros and decimal point, and return that result.

Making this change broke two fate tests, filter-metadata-scdet,
and filter-metadata-silencedetect. To correct this, I've modified
tests/ref/fate/filter-metadata-scdet and
tests/ref/fate/filter-metadata-silencedetect to match the
new output.

Signed-off-by: Allan Cady <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;
 }
 
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.34.1

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

Re: [FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files

[Allan Cady via ffmpeg-devel]

Greetings Martin et al,

I've been trying to resubmit this patch based on your earlier suggestions. Most of the tools in the toolchain for this are new to me, so it's been awkward going. 

I did finally get the email sent with the new patch, but for some reason I haven't been able to figure out yet, it's going to the wrong thread. Here's the link to the message with the patch.

https://ffmpeg.org/pipermail/ffmpeg-devel/2024-March/323170.html

Looking forward to comments and to hopefully getting this approved and into the codebase.

Allan







On Friday, February 23, 2024 at 02:47:27 PM PST, Marton Balint <cus@passwd.hu> wrote: 







On Fri, 23 Feb 2024, Allan Cady via ffmpeg-devel wrote:

> [Apologies for the awful mess in the previous email. Trying again with straight text.]
>
> On Thursday, February 22, 2024 at 01:16:19 AM PST, Marton Balint <cus@passwd.hu> wrote:
>
>>> For starters, I'm curious why there are two functions & macros:
>>>
>>> av_ts2str/av_ts_make_string (which used "%" format specifier)
>>  
>> That takes a 64-bit integer timestamp and is actually using "%"PRId64 
>> because that is the correct (portable) format string for an int64_t 
>> variable.
>>  
>>> av_ts2timestr/av_ts_make_time_string (which used "%6g")
>>  
>> That takes an integer timestamp and a rational time base. Float timestamps 
>> (in seconds) is calculated by multiplying the two, that is what is 
>> printed.
>>  
>>>
>>> Do you know the rationale for that? I see that only av_ts2timestr is used in silencedetect.c.
>>>
>>> And are you suggesting we should fold those two functions into one?
>>  
>> No, they have different purpose. The first prints out a timestamps which 
>> can be in any time base. The second prints out a timestamp in seconds.
>
>
> Understood, I think I'm up to speed now. av_ts2str prints a 64-bit integer timestamp;
> av_ts2timestr prints a floating point timestamp in seconds with the timebase applied.
>
>
> In your previous email, you said:
>
>
>> I'd rather just to fix av_ts_make_string to not limit the number of 
>> significant digits. Something like:
>>
>> 1) Print the number in decimal notation with at most 6 fractional digits. 
>> 2) Use less fractional digits if the first format would not fit into 
>> AV_TS_MAX_STRING_SIZE.
>> 3) Use scientific notation if the second format would not fit into 
>> AV_TS_MAX_STRINT_SIZE.
>
>
> I think you probably meant to say "I'd rather just to fix
> av_ts_make_time_string" (not av_ts_make_string)?
> Since it's av_ts_make_time_string() that's formatting floating point.

Yes, indeed.

>
> So it makes sense to me to make the change to av_ts_make_time_string()
> for all timestamps, as you suggest. As for how specifically to format them,
> I'm fine with whatever you think is best, and I'm happy to work on this, but the
> implementation has me a bit stumped for the moment, and I may need some
> help with it. My C language skills are rusty to say the least.

The simplest way is to try snprintf() with 6 fractional digits, and check 
the return value to see how long the string would become.
Based on this you can either accept snprintf result and truncate 
trailing zeros and dots, or try snprintf() with less fractional digits and 
truncate, or fall back to scientific format.

>
> It also occurs to me to wonder, would this warrant a formal problem ticket?
> I haven't looked into how that works for ffmpeg.

Opening a ticket for the problem is not required, but if your patch 
fixes an existing ticket, then you should mention the ticket number
in the commit message.

Regards,

Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".