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

[Michael Niedermayer]

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

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190703/7e3ce85a/attachment.sig>