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

[Clément Bœsch]

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

Sorry to Alexander, I think I missed that…

> > +
> > +/**
> > + * 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?
> 

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?
> 
> > +
> > +/**
> > + * 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.

(Q: no git-sed or similar to do rename on successive commits without git
rebase, edit, git ammend, git rebase continue... etc?)

BTW, I'd like to apply the first part at least (patches 1 to 5) in a week
(or two if you want some more time to do some review).

[...]

-- 
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/20120124/d61c3981/attachment.asc>