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 <[email protected]> > >> 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 [email protected] https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email [email protected] with subject "unsubscribe".
