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

[Ulf Zibis]

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 at 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