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

[Stefano Sabatini]

On date Friday 2012-01-27 22:14:35 +0100, Alexander Strasser encoded:
> 
> Hi Stefano,
> 
> Stefano Sabatini wrote:
> [...]
> > 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
> 
>   As they are macros and are for convenience only (functionality is still
> available for C++ programmers), I am OK with that.
> 
> > * 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
> 
>   Fine if you prefer it this way.
> 

> [...] 
> > +static inline char *av_ts_make_string(char *buf, int64_t ts)
> [...] 
> > +static inline char *av_ts_make_time_string(char *buf, int64_t ts, AVRational *tb)
> 
> NIT:
>   Here and before I would prefer ts as the first argument, it is also consistent
> with the naming which is av_ts_*. Like it is saying these functions are acting on
> a timestamp (I know they can't and don't need to modify it as they only get a copy).
>
>   IMHO following that convention makes the order more easy to remember. I would be
> glad to hear your opinion.

Procedural C style convenctions usually demand this order of arguments:

make_thing(dst, src);

so in this case that would be:
av_ts_make_string(char *buf, int64_t ts)

which is interpreted as:
buf <= ts

i.e. like an assignment.

In case of:
static inline char *av_ts_make_time_string(char *buf, int64_t ts, AVRational *tb);

here the timestamp can't be represented by a single entity (indeed it
needs a tb in order to be interpreted), so I think that preferring:

static inline char *av_ts_make_time_string(int64_t ts, AVRational *tb, char *buf)

would be mostly confusing.

Note that here "_ts_" is used like a namespace rather than for
indicating the "object" of the "operator" denoted by the function, in
case you're adopting an object-oriented programming style, for which:

int av_bike_set_shed_color(AVBike *bike, char *color_str);

would have made much sense.
-- 
FFmpeg = Frenzy and Forgiving Multimedia Powered Erroneous Gospel