Re: [FFmpeg-devel] [PATCH v2] Add 2 timestamp print formats
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] Add 2 timestamp print formats
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
> >> 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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Observe your enemies, for they first find out your faults. -- Antisthenes
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] Add 2 timestamp print formats
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] Add 2 timestamp print formats
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
>> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] Add 2 timestamp print formats
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
> 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
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The educated differ from the uneducated as much as the living from the
dead. -- Aristotle
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] Add 2 timestamp print formats
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
>From 709cb4662132deff2d17a8700f58fa91b118c56d Mon Sep 17 00:00:00 2001
From: Ulf Zibis
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 */
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmp
