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