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

[Stefano Sabatini]

On date Tuesday 2012-01-24 22:21:02 +0100, Alexander Strasser encoded:
> 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
> > > 

Sorry that was a note and I somehow misread it as a @param, it's
perfectly fine then.

> > 
> > 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'd like to stick with the convenction: a function name contains a
verb, even if it is not always respected.

> > > 
> > 
> > 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.

I'll stop bikeshedding now, I generally agree with the opinion of
Alexander, but I'm not strong about them.
-- 
FFmpeg = Foolish and Free MultiPurpose Epic Genius