[FFmpeg-devel] [PATCH 02/12] lavu: add public timecode API.

[Alexander Strasser]

Hi Clément,

Clément Bœsch wrote:
> On Tue, Jan 24, 2012 at 01:47:30PM +0100, Stefano Sabatini wrote:
> [...]
> > > + * @note Frame number adjustment is automatically done in case of drop timecode,
> > > + *       you do NOT have to call av_adjust_ntsc_framenum().
> > 
> > nit++: no complete sentence, lowcased and no dot at the end
> > 
> 
> Most of the the @note I see are of two forms:
> 
>   @note frame number adjust is [...] ...framenum().
>     ==> "note that xxxx."
>   @note Frame number [...] ...framenum().
>     ==> "NOTE: xxxx."
> 
> I used the second form, but it seems you do not want the first form
> either. Should I just drop the period? The sentence still has verbs… Could
> you be a bit more explicit about what you are expecting with an example?
> 
> [...]
> > > + */
> > > +uint32_t av_timecode_get_smpte_from_framenum(const AVTimecode *tc, int framenum);
> > 
> > Nit: get->make is maybe more approriate (as noted by Alexander
> > Strasser in a recent mail)

  First of all thanks to Stefano for caring about naming. Second I appologize to
you, Clément, as proper naming needs some discussion :(

> > 
> 
> Sorry to Alexander, I think I missed that…

  Please pardon me if I caused any confusion. I did not mean to say that every get
should be replaced with make. I only meant to say that "make" is propably more
precise if you pass a string buffer into a function which gets filled by that
function.

  If I am not mistaken this returns the result as an uint32_t "value" type, so get
is fine IMHO. In other words you can attribute creation/allocation to the function
itself.
 
> > > +
> > > +/**
> > > + * Load timecode string in buf.
> > > + *
> > > + * @param buf      destination buffer, must be at least AV_TIMECODE_STR_SIZE long
> > > + * @param tc       timecode data correctly initialized
> > > + * @param framenum frame number
> > > + * @return         the buf parameter
> > > + *
> > 
> > > + * @note Timecode representation can be a negative timecode and have more than
> > > + *       24 hours, but will only be honored if the flags are correctly set.
> > > + * @note The frame number is relative to tc->start.
> > 
> > ditto
> > 
> > > + */
> > > +char *av_timecode_get_string(char *buf, const AVTimecode *tc, int framenum);
> > 
> > ditto: get -> make?

  Here probably "make" is better. Or maybe av_timecode_to_string would be prettier?

> > 
> 
> I see the make form is used a little in avfilter, but I'm not sure. "make"
> makes me think of a function returning a sucess or failure after a
> processing, while get will return what is announced in the function name.
> 
> "get_" looks fine to me here... Of course, I get the point of the buffer
> being the destination but well…
> 
> > 
> > > +/**
> > > + * Get the timecode string from the 25-bit timecode format (MPEG GOP format).
> > > + *
> > > + * @param buf     destination buffer, must be at least AV_TIMECODE_STR_SIZE long
> > > + * @param tc25bit the 25-bits timecode
> > > + * @return        the buf parameter
> > > + */
> > > +char *av_timecode_get_mpegtc_string(char *buf, uint32_t tc25bit);
> > 
> > make?

  same as above

> > > +
> > > +/**
> > > + * Init a timecode struct with the passed parameters.
> > > + *
> > > + * @param log_ctx     a pointer to an arbitrary struct of which the first field
> > > + *                    is a pointer to an AVClass struct (used for av_log)
> > > + * @param tc          pointer to an allocated AVTimecode
> > > + * @param rate        frame rate in rational form
> > > + * @param flags       miscellaneous flags such as drop frame, +24 hours, ...
> > > + *                    (see AVTimecodeFlag)
> > > + * @param frame_start the first frame number
> > > + * @return            0 on success, AVERROR otherwise
> > > + */
> > 
> > > +int av_timecode_init(void *log_ctx, AVTimecode *tc, AVRational rate, int flags, int frame_start);
> > > +
> > > +/**
> > > + * Parse timecode representation (hh:mm:ss[:;.]ff).
> > > + *
> > > + * @param log_ctx a pointer to an arbitrary struct of which the first field is a
> > > + *                pointer to an AVClass struct (used for av_log).
> > > + * @param tc      pointer to an allocated AVTimecode
> > > + * @param rate    frame rate in rational form
> > > + * @param str     timecode string which will determine the frame start
> > > + * @return        0 on success, AVERROR otherwise
> > > + */
> > > +int av_timecode_init_from_string(void *log_ctx, AVTimecode *tc, AVRational rate, const char *str);
> > 
> > For these two, either make or init should be fine, I have no strong
> > opinion though but a slight bias towards "make" for internal
> > consistency reasons.
> 
> Why? It just initialized the passed struct given a few parameters. It
> doesn't really build anything.
> 
> If someone strongly believes in one form over the other, I'll do the
> necessary; otherwise I'd avoid reworking the whole patchset for this if
> you don't mind.

  I believe init is appropriate here.

[...]

  Alexander