[FFmpeg-devel] [RFC] lavu PTS printing utils

Clément Bœsch ubitux at gmail.com
Fri Jan 27 19:09:38 CET 2012


On Fri, Jan 27, 2012 at 01:35:06PM +0100, Stefano Sabatini wrote:
> On date Friday 2012-01-20 20:21:08 +0100, Reimar Döffinger encoded:
> > On Fri, Jan 20, 2012 at 04:13:52PM +0100, Stefano Sabatini wrote:
> > > On date Tuesday 2012-01-17 22:56:35 +0100, Reimar Döffinger encoded:
> > > > On Tue, Jan 17, 2012 at 10:44:21PM +0100, Alexander Strasser wrote:
> > > > > [...]
> > > > > > +
> > > > > > +static inline char *av_get_ts_string(char *buf, size_t buf_size, int64_t ts)
> > > > > > +{
> > > > > > +    if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS");
> > > > > > +    else                      snprintf(buf, buf_size, "%"PRId64"", ts);
> > > > > > +    return buf;
> > > > > > +}
> > > > > > +
> > > > > > +#define av_ts2str(ts) av_get_ts_string((char[42]){ 0 }, 42, ts)
> > > > > > +
> > > > > > +static inline char* av_get_time_string(char *buf, size_t buf_size, int64_t ts, AVRational *tb)
> > > > > > +{
> > > > > > +    if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS");
> > > > > > +    else                      snprintf(buf, buf_size, "%f", av_q2d(*tb) * ts);
> > > > > > +    return buf;
> > > > > > +}
> > > > > > +
> > > > > > +#define av_time2str(ts, tb) av_get_time_string((char[42]){ 0 }, 42, ts, tb)
> > > > > 
> > > > >   The naming of the macros is a bit risky. Especially considering how often we
> > > > > append numbers to names to introduce a successor. av_time2str2 would be rather
> > > > > confusing.
> > > > 
> > > 
> > > > Also while I don't care too much about C++, the (char[42]){ 0 } syntax
> > > > has not made it into even the latest C++ version, so I'd strongly
> > > > suggest to think at least thrice before adding such a thing to the
> > > > public API (even though a macro means it won't cause compile errors
> > > > unless used - but if we want to take advantage of that it should
> > > > probably be documented).
> > > > Actually, I think it might be a right-out bad idea to use this
> > > > at all in public API, because I don't think anyone would accept
> > > > that
> > > 
> > > Agreed, also I don't think that:
> > > printf("pts1_time:%s pts2_time:%s\n", av_time2str(pts1, tb1), av_time2str(pts2, tb2))
> > > 
> > > has defined behavior in this case (evaluation order of arguments is
> > > undefined in C, and the construct may return an address in the stack
> > > which to a buffer which may be overwritten by the other function
> > > call).
> > 
> > No, I don't think that is a problem. They are different variables
> > which both have the same scope (the current block) so the compiler
> > can't place them any more at the same place that if you used
> > char a[42],b[42];
> > 
> > > char buf1[AV_TS_MAX_SIZE], buf2[AV_TS_MAX_SIZE];
> > 
> > Not much of an opinion otherwise, but _TS_ is not that great a name
> > due to the confusion potential with (MPEG-)TS.
> 
> Updated patch with sample ffmpeg patch, comments:
> 
> * av_ts2str(), av_ts2time2str() are retained for their usefulness,
>   although they should never be used in C++, we assume that C++
>   programmers using FFmpeg code have a clue regarding C/C++
>   incompatibilites, and they can still use the long av_ts_make*
>   functions
> 

C++ developers are used to work with a limited subset of the C
functionalities, let's not be concerned to much about this :)

> * double precision is reduced to 6 digits, which looks enough for A/V
>   applications, could not be enough for scientific computing but again
>   given the application domain I don't think this is a much serious
>   issue
> 

Isn't the way of printing a floating value up to a certain precision
depending on the printf implementation anyway?

> * "ts" is favored over "timestamp" for brevity/convenience reasons
> 
> * "ts2str" naming is kept in the macro for brevity/convenience reasons
> 

I'm OK with this.

[...]
> From f7ba7daa66ed3cb79ab425d0494889be8ad685f7 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefasab at gmail.com>
> Date: Fri, 20 Jan 2012 17:06:26 +0100
> Subject: [PATCH] ffmpeg.c: use timestamp.h utilities
> 
> ---
>  ffmpeg.c |   33 ++++++++++++++++++++++++++-------
>  1 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 88a4761..1243df6 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -23,6 +23,8 @@
>   * multimedia converter based on the FFmpeg libraries
>   */
>  
> +#define DEBUG_TS
> +

I'm not really fond of this; why not use the -debug pts (see FF_DEBUG_PTS)
or a new -debug ts instead?

[...]

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120127/f97de60c/attachment.asc>


More information about the ffmpeg-devel mailing list