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

[Clément Bœsch]

On Tue, Jan 24, 2012 at 10:21:02PM +0100, Alexander Strasser wrote:
> Hi Clément,
> 

Hi Alexander,

> 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 :(
> 

Proper naming for public is important, better overthink it first.

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

Consider these changes done locally:
    av_timecode_get_string        → av_timecode_make_string
    av_timecode_get_mpegtc_string → av_timecode_make_mpegtc_string

BTW, for my git-sed question, I used the following command to do the
rename:

  git filter-branch -f --tree-filter 'for f in $(git ls-files "*.[ch]"); do
  sed -i s#old_name#new_name#g $f; done' HEAD...HEAD~12

It's very slow (I wasn't able to make git-ls-files list only the files
modified in the last commit), and I couldn't simplify HEAD...HEAD~12: I'm
in a branch with the 12 commits, and while HEAD...master works, it raises
a warning about master not being modified. Using only "master" doesn't
work either. Any hint welcome for the next rename request :)


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

OK, I keep the "init" then.

[...]

-- 
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/20120125/7d98770f/attachment.asc>