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

[Stefano Sabatini]

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

* 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

* "ts" is favored over "timestamp" for brevity/convenience reasons

* "ts2str" naming is kept in the macro for brevity/convenience reasons

Please comment.
-- 
FFmpeg = Fast and Funny Muttering Pitiless Embarassing Glue
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-lavu-add-timestamp.h-header-with-convenience-timesta.patch
Type: text/x-diff
Size: 3495 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120127/580bbc9b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-ffmpeg.c-use-timestamp.h-utilities.patch
Type: text/x-diff
Size: 3606 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120127/580bbc9b/attachment-0001.bin>