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

[Alexander Strasser]

Hi,

Clément Bœsch wrote:
> On Tue, Jan 24, 2012 at 10:21:02PM +0100, Alexander Strasser wrote:
> > 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.

  I fear I have another one. Seems I am only causing you work :(

  But please don't do anything if you think I am not making sense here.
 
  I just saw it would be good to have the tc parameter as the first one,
for all av_timecode_* functions that use (read/write) its state (I think
all others don't have that param at all).

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

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

  Fine.

  Alexander