[FFmpeg-devel] [PATCH 02/12] lavu: add public timecode API.
Clément Bœsch
ubitux at gmail.com
Wed Jan 25 17:13:47 CET 2012
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>
More information about the ffmpeg-devel
mailing list