diff mbox

[FFmpeg-devel,v2] Add 2 timestamp print formats

Message ID 917dcaa0-ccc7-a78f-9e93-bee8d2fb1294@CoSoCo.de
State Superseded
Headers show

Commit Message

Ulf Zibis July 1, 2019, 9:51 a.m. UTC 
Am 29.06.19 um 18:12 schrieb Ulf Zibis:
> Hi,
>
> for my developement of another filter I need additional timestamp print
> formats.
>
> So I like to share my efforts. Are you interested to include them to the
> project?
>
> For libavutil/timestamp.h I'm also wondering, why these format functions
> are designated for "inline". As printing is always a little heavy job,
> an explicit function call would not change much for performance IMHO,
> but would save project footprint.
Because of a rounding issue I had to make a new version.

Please review patch v2.

-Ulf

Comments

Michael Niedermayer July 1, 2019, 3:16 p.m. UTC | #1 
On Mon, Jul 01, 2019 at 11:51:30AM +0200, Ulf Zibis wrote:
> 
> Am 29.06.19 um 18:12 schrieb Ulf Zibis:
> > Hi,
> >
> > for my developement of another filter I need additional timestamp print
> > formats.
> >
> > So I like to share my efforts. Are you interested to include them to the
> > project?
> >
> > For libavutil/timestamp.h I'm also wondering, why these format functions
> > are designated for "inline". As printing is always a little heavy job,
> > an explicit function call would not change much for performance IMHO,
> > but would save project footprint.
> Because of a rounding issue I had to make a new version.
> 
> Please review patch v2.
> 
> -Ulf
> 

>  timestamp.h |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 73 insertions(+), 5 deletions(-)
> 3d1275421b1b8d4952815151158c7af954d38ca6  timestamp_2.patch
> From 709cb4662132deff2d17a8700f58fa91b118c56d Mon Sep 17 00:00:00 2001
> From: Ulf Zibis <Ulf.Zibis@CoSoCo.de>
> Date: 29.06.2019, 17:52:06
> 
> avutil/timestamp: added 2 new print formats
> 
> diff --git a/libavutil/timestamp.h b/libavutil/timestamp.h
> index e082f01..b94e5ce 100644
> --- a/libavutil/timestamp.h
> +++ b/libavutil/timestamp.h
> @@ -48,14 +48,14 @@
>  }
>  
>  /**
> - * Convenience macro, the return value should be used only directly in
> + * Convenience macro. The return value should be used only directly in
>   * function arguments but never stand-alone.
>   */
> -#define av_ts2str(ts) av_ts_make_string((char[AV_TS_MAX_STRING_SIZE]){0}, ts)
> +#define av_ts2str(ts) av_ts_make_string((char[AV_TS_MAX_STRING_SIZE]){'\0'}, ts)
>  
>  /**
>   * Fill the provided buffer with a string containing a timestamp time
> - * representation.
> + * representation in seconds.
>   *
>   * @param buf a buffer with size in bytes of at least AV_TS_MAX_STRING_SIZE
>   * @param ts the timestamp to represent

> @@ -70,9 +70,77 @@
>  }
>  
>  /**
> - * Convenience macro, the return value should be used only directly in
> + * Convenience macro. The return value should be used only directly in
>   * function arguments but never stand-alone.
>   */

Unrelated change, belongs in a seperate patch


> -#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((char[AV_TS_MAX_STRING_SIZE]){'\0'}, ts, tb)
> +
> +/**
> + * Fill the provided buffer with a string containing a timestamp time
> + * representation in minutes and seconds.
> + *
> + * @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
> + * @return the buffer in input
> + */
> +static inline char *av_ts_make_minute_string(char *buf, int64_t ts, AVRational *tb)
> +{
> +    if (ts == AV_NOPTS_VALUE) snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
> +    else {
> +        double time = av_q2d(*tb) * ts;

If this could be done without float/double that would be preferred as it
avoids inaccuracies / slight differences between platforms


> +        const char *format = (time >= 0 ? "%3d:%09.6f" : "-%3d:%09.6f");
> +        time = (fabs(time) > INT_MAX * 60.0 ? INT_MAX * 60.0 : fabs(time));
> +        int len = snprintf(buf, AV_TS_MAX_STRING_SIZE, format, (int)(time / 60), fmod(time, 60));

mixed declarations and code

[...]

thx
Ulf Zibis July 1, 2019, 10:28 p.m. UTC | #2 
Thanks Michael for your review, answers inline ...

Am 01.07.19 um 17:16 schrieb Michael Niedermayer:
>>  timestamp.h |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 73 insertions(+), 5 deletions(-)
>> 3d1275421b1b8d4952815151158c7af954d38ca6  timestamp_2.patch
>> From 709cb4662132deff2d17a8700f58fa91b118c56d Mon Sep 17 00:00:00 2001
>> From: Ulf Zibis <Ulf.Zibis@CoSoCo.de>
>> Date: 29.06.2019, 17:52:06
>>
>> avutil/timestamp: added 2 new print formats
>>
>> diff --git a/libavutil/timestamp.h b/libavutil/timestamp.h
>> index e082f01..b94e5ce 100644
>> --- a/libavutil/timestamp.h
>> +++ b/libavutil/timestamp.h
>> @@ -48,14 +48,14 @@
>>  }
>>  
>>  /**
>> - * Convenience macro, the return value should be used only directly in
>> + * Convenience macro. The return value should be used only directly in
>>   * function arguments but never stand-alone.
>>   */
>> -#define av_ts2str(ts) av_ts_make_string((char[AV_TS_MAX_STRING_SIZE]){0}, ts)
>> +#define av_ts2str(ts) av_ts_make_string((char[AV_TS_MAX_STRING_SIZE]){'\0'}, ts)
>>  
>>  /**
>>   * Fill the provided buffer with a string containing a timestamp time
>> - * representation.
>> + * representation in seconds.
>>   *
>>   * @param buf a buffer with size in bytes of at least AV_TS_MAX_STRING_SIZE
>>   * @param ts the timestamp to represent
>> @@ -70,9 +70,77 @@
>>  }
>>  
>>  /**
>> - * Convenience macro, the return value should be used only directly in
>> + * Convenience macro. The return value should be used only directly in
>>   * function arguments but never stand-alone.
>>   */
> Unrelated change, belongs in a seperate patch
The following change I think should be part of the patch, as it helps to
distinguish between the existing timestamp functions and my new ones:
- * representation.
+ * representation in seconds.
The other above changes are cosmetic and can go in a separate patch. But
would you endorse them?

>> -#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((char[AV_TS_MAX_STRING_SIZE]){'\0'}, ts, tb)
>> +
>> +/**
>> + * Fill the provided buffer with a string containing a timestamp time
>> + * representation in minutes and seconds.
>> + *
>> + * @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
>> + * @return the buffer in input
>> + */
>> +static inline char *av_ts_make_minute_string(char *buf, int64_t ts, AVRational *tb)
>> +{
>> +    if (ts == AV_NOPTS_VALUE) snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
>> +    else {
>> +        double time = av_q2d(*tb) * ts;
> If this could be done without float/double that would be preferred as it
> avoids inaccuracies / slight differences between platforms

I too thought on that, but the existing functions too rely on
float/double. Should I anyway provide a solution with integer arithmetic?

>> +        const char *format = (time >= 0 ? "%3d:%09.6f" : "-%3d:%09.6f");
>> +        time = (fabs(time) > INT_MAX * 60.0 ? INT_MAX * 60.0 : fabs(time));
>> +        int len = snprintf(buf, AV_TS_MAX_STRING_SIZE, format, (int)(time / 60), fmod(time, 60));

Correct. I didn't bother on that for sake of readability and saving code
lines. This can be changed, if you want.

What's about "inline"? I think it's not helpful here as I argued in my
last post.

-Ulf
Ulf Zibis July 2, 2019, 12:09 a.m. UTC | #3 
Am 02.07.19 um 00:28 schrieb Ulf Zibis:
>> If this could be done without float/double that would be preferred as it
>> avoids inaccuracies / slight differences between platforms
> I too thought on that, but the existing functions too rely on
> float/double. Should I anyway provide a solution with integer arithmetic?
Then there could be inconsistencies with the existing
av_ts_make_time_string function.

My current approach is result of this discussion:
https://stackoverflow.com/questions/56797715/which-format-string-to-use-with-printf-for-a-time-as-78901-234/56814382#

-Ulf
Michael Niedermayer July 3, 2019, 8:52 a.m. UTC | #4 
On Tue, Jul 02, 2019 at 12:28:53AM +0200, Ulf Zibis wrote:
> Thanks Michael for your review, answers inline ...
> 
> Am 01.07.19 um 17:16 schrieb Michael Niedermayer:
> >>  timestamp.h |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> >>  1 file changed, 73 insertions(+), 5 deletions(-)
> >> 3d1275421b1b8d4952815151158c7af954d38ca6  timestamp_2.patch
> >> From 709cb4662132deff2d17a8700f58fa91b118c56d Mon Sep 17 00:00:00 2001
> >> From: Ulf Zibis <Ulf.Zibis@CoSoCo.de>
> >> Date: 29.06.2019, 17:52:06
> >>
> >> avutil/timestamp: added 2 new print formats
> >>
> >> diff --git a/libavutil/timestamp.h b/libavutil/timestamp.h
> >> index e082f01..b94e5ce 100644
> >> --- a/libavutil/timestamp.h
> >> +++ b/libavutil/timestamp.h
> >> @@ -48,14 +48,14 @@
> >>  }
> >>  
> >>  /**
> >> - * Convenience macro, the return value should be used only directly in
> >> + * Convenience macro. The return value should be used only directly in
> >>   * function arguments but never stand-alone.
> >>   */
> >> -#define av_ts2str(ts) av_ts_make_string((char[AV_TS_MAX_STRING_SIZE]){0}, ts)
> >> +#define av_ts2str(ts) av_ts_make_string((char[AV_TS_MAX_STRING_SIZE]){'\0'}, ts)
> >>  
> >>  /**
> >>   * Fill the provided buffer with a string containing a timestamp time
> >> - * representation.
> >> + * representation in seconds.
> >>   *
> >>   * @param buf a buffer with size in bytes of at least AV_TS_MAX_STRING_SIZE
> >>   * @param ts the timestamp to represent
> >> @@ -70,9 +70,77 @@
> >>  }
> >>  
> >>  /**
> >> - * Convenience macro, the return value should be used only directly in
> >> + * Convenience macro. The return value should be used only directly in
> >>   * function arguments but never stand-alone.
> >>   */
> > Unrelated change, belongs in a seperate patch
> The following change I think should be part of the patch, as it helps to
> distinguish between the existing timestamp functions and my new ones:
> - * representation.
> + * representation in seconds.
> The other above changes are cosmetic and can go in a separate patch. But
> would you endorse them?

Iam not a english composer ...


> 
> >> -#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((char[AV_TS_MAX_STRING_SIZE]){'\0'}, ts, tb)
> >> +
> >> +/**
> >> + * Fill the provided buffer with a string containing a timestamp time
> >> + * representation in minutes and seconds.
> >> + *
> >> + * @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
> >> + * @return the buffer in input
> >> + */
> >> +static inline char *av_ts_make_minute_string(char *buf, int64_t ts, AVRational *tb)
> >> +{
> >> +    if (ts == AV_NOPTS_VALUE) snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
> >> +    else {
> >> +        double time = av_q2d(*tb) * ts;
> > If this could be done without float/double that would be preferred as it
> > avoids inaccuracies / slight differences between platforms
> 
> I too thought on that, but the existing functions too rely on
> float/double. Should I anyway provide a solution with integer arithmetic?

its indepedant of your patch but i think all these should use integers
unless its too messy

thx 
[...]
Ulf Zibis July 19, 2019, 1:37 p.m. UTC | #5 
Am 03.07.19 um 10:52 schrieb Michael Niedermayer:
>
>> I too thought on that, but the existing functions too rely on
>> float/double. Should I anyway provide a solution with integer arithmetic?
> its indepedant of your patch but i think all these should use integers
> unless its too messy

Now seeing the code of the select filter I'm wondering that there too
generally float values are used even when dealing with typically integer
values e.g. frame no.

-Ulf
diff mbox

Patch

From 709cb4662132deff2d17a8700f58fa91b118c56d Mon Sep 17 00:00:00 2001
From: Ulf Zibis <Ulf.Zibis@CoSoCo.de>
Date: 29.06.2019, 17:52:06

avutil/timestamp: added 2 new print formats

diff --git a/libavutil/timestamp.h b/libavutil/timestamp.h
index e082f01..b94e5ce 100644
--- a/libavutil/timestamp.h
+++ b/libavutil/timestamp.h
@@ -48,14 +48,14 @@ 
 }
 
 /**
- * Convenience macro, the return value should be used only directly in
+ * Convenience macro. The return value should be used only directly in
  * function arguments but never stand-alone.
  */
-#define av_ts2str(ts) av_ts_make_string((char[AV_TS_MAX_STRING_SIZE]){0}, ts)
+#define av_ts2str(ts) av_ts_make_string((char[AV_TS_MAX_STRING_SIZE]){'\0'}, ts)
 
 /**
  * Fill the provided buffer with a string containing a timestamp time
- * representation.
+ * representation in seconds.
  *
  * @param buf a buffer with size in bytes of at least AV_TS_MAX_STRING_SIZE
  * @param ts the timestamp to represent
@@ -70,9 +70,77 @@ 
 }
 
 /**
- * Convenience macro, the return value should be used only directly in
+ * Convenience macro. The return value 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((char[AV_TS_MAX_STRING_SIZE]){'\0'}, ts, tb)
+
+/**
+ * Fill the provided buffer with a string containing a timestamp time
+ * representation in minutes and seconds.
+ *
+ * @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
+ * @return the buffer in input
+ */
+static inline char *av_ts_make_minute_string(char *buf, int64_t ts, AVRational *tb)
+{
+    if (ts == AV_NOPTS_VALUE) snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
+    else {
+        double time = av_q2d(*tb) * ts;
+        const char *format = (time >= 0 ? "%3d:%09.6f" : "-%3d:%09.6f");
+        time = (fabs(time) > INT_MAX * 60.0 ? INT_MAX * 60.0 : fabs(time));
+        int len = snprintf(buf, AV_TS_MAX_STRING_SIZE, format, (int)(time / 60), fmod(time, 60));
+        if (len - 9 >= 0 && buf[len - 9] > '5') // correct rare rounding issue
+            len = snprintf(buf, AV_TS_MAX_STRING_SIZE, format, (int)(time / 60) + 1, .0);
+        while (buf[--len] == '0'); // search trailing zeros or ...
+        buf[len + (buf[len] != '.')] = '\0'; // dot and strip them
+    }
+    return buf;
+}
+
+/**
+ * Convenience macro. The return value should be used only directly in
+ * function arguments but never stand-alone.
+ */
+#define av_ts2minutestr(ts, tb) av_ts_make_minute_string((char[AV_TS_MAX_STRING_SIZE]){'\0'}, ts, tb)
+
+/**
+ * Fill the provided buffer with a string containing a timestamp time
+ * representation in hours, minutes and seconds.
+ *
+ * @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
+ * @return the buffer in input
+ */
+static inline char *av_ts_make_hour_string(char *buf, int64_t ts, AVRational *tb)
+{
+    if (ts == AV_NOPTS_VALUE) snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
+    else {
+        double time = av_q2d(*tb) * ts;
+        const char *format = (time >= 0 ? "%d:%02d:%09.6f" : "-%d:%02d:%09.6f");
+        time = (fabs(time) > INT_MAX * 60.0 * 60.0 ? INT_MAX * 60.0 * 60.0 : fabs(time));
+        int hours = time / 60 / 60, minutes = fmod(time / 60, 60);
+        int len = snprintf(buf, AV_TS_MAX_STRING_SIZE, format, hours, minutes, fmod(time, 60));
+        if (len - 9 >= 0 && buf[len - 9] > '5') { // correct rare rounding issue
+            if (++minutes > 59) {
+                minutes = 0;
+                hours++;
+            }
+            len = snprintf(buf, AV_TS_MAX_STRING_SIZE, format, hours, minutes, .0);
+        }
+        while (buf[--len] == '0'); // search trailing zeros or ...
+        buf[len + (buf[len] != '.')] = '\0'; // dot and strip them
+    }
+    return buf;
+}
+
+/**
+ * Convenience macro. The return value should be used only directly in
+ * function arguments but never stand-alone.
+ */
+#define av_ts2hourstr(ts, tb) av_ts_make_hour_string((char[AV_TS_MAX_STRING_SIZE]){'\0'}, ts, tb)
 
 #endif /* AVUTIL_TIMESTAMP_H */