[FFmpeg-devel] [PATCH 01/11] lavu: add public timecode API.
Clément Bœsch
ubitux at gmail.com
Mon Jan 23 16:55:00 CET 2012
On Thu, Jan 19, 2012 at 01:15:26AM +0100, Stefano Sabatini wrote:
[...]
> > +/**
> > + * @file
> > + * Timecode helpers
> > + */
>
> Please add a link or a reference to the spec, it's always nice to find
> them when you're exploring for the first time a format implementation.
>
Added two references.
> > +
> > +#include <stdio.h>
>
> > +#include "timecode.h"
> > +#include "log.h"
> > +#include "error.h"
>
> nit+++: sort them
>
I can't, error.h seems to depend on log.h.
> > +
> > +char *av_mpegtc_to_timecode_string(char *buf, uint32_t tc25bit)
> > +{
> > + snprintf(buf, AVTIMECODE_STR_LEN, "%02d:%02d:%02d%c%02d",
> > + tc25bit>>19 & 0x1f, // hours
> > + tc25bit>>13 & 0x3f, // minutes
> > + tc25bit>>6 & 0x3f, // seconds
> > + tc25bit & 1<<24 ? ';' : ':', // drop
> > + tc25bit & 0x3f); // frames
> > + return buf;
> > +}
> > +
> > +int av_adjust_ntsc_framenum(int framenum)
> > +{
> > + /* only works for NTSC 29.97 */
> > + int d = framenum / 17982;
> > + int m = framenum % 17982;
> > + //if (m < 2) m += 2; /* not needed since -2,-1 / 1798 in C returns 0 */
> > + return framenum + 18 * d + 2 * ((m - 2) / 1798);
> > +}
> > +
> > +uint32_t av_framenum_to_smpte_timecode(const AVTimecode *tc, int framenum)
> > +{
> > + unsigned fps = tc->fps;
> > + int drop = !!(tc->flags & AVTIMECODE_FLAG_DROPFRAME);
> > + int hh, mm, ss, ff;
> > +
> > + framenum += tc->start;
> > + if (drop)
> > + framenum = av_adjust_ntsc_framenum(framenum);
> > + ff = framenum % fps;
> > + ss = framenum / fps % 60;
> > + mm = framenum / (fps*60) % 60;
> > + hh = framenum / (fps*3600) % 24;
> > + return 0 << 31 | // color frame flag
> > + drop << 30 | // drop frame flag
> > + (ff / 10) << 28 | // tens of frames
> > + (ff % 10) << 24 | // units of frames
> > + 0 << 23 | // field phase (NTSC), b0 (PAL)
> > + (ss / 10) << 20 | // tens of seconds
> > + (ss % 10) << 16 | // units of seconds
> > + 0 << 15 | // b0 (NTSC), b2 (PAL)
> > + (mm / 10) << 12 | // tens of minutes
> > + (mm % 10) << 8 | // units of minutes
> > + 0 << 7 | // b1
> > + 0 << 6 | // b2 (NTSC), field phase (PAL)
> > + (hh / 10) << 4 | // tens of hours
> > + (hh % 10); // units of hours
> > +}
>
> can't comment without the spec
>
It is similar to avpriv_framenum_to_smpte_timecode() in lavc/timecode.c,
but with the adjustment in it.
> > +char *av_framenum_to_timecode_string(char *buf, const AVTimecode *tc, int framenum)
> > +{
> > + int fps = tc->fps;
> > + int drop = tc->flags & AVTIMECODE_FLAG_DROPFRAME;
> > + int hh, mm, ss, ff, neg = 0;
> > +
> > + framenum += tc->start;
> > + if (drop)
> > + framenum = av_adjust_ntsc_framenum(framenum);
> > + if (framenum < 0) {
> > + framenum = -framenum;
> > + neg = tc->flags & AVTIMECODE_FLAG_NEGATIVEOK;
> > + }
>
> I assume it is acceptable to show the opposite of the number in case
> it is negative and the flag is not enabled.
>
Yes I guess, the specs is not really obvious about this.
> > + ff = framenum % fps;
> > + ss = framenum / fps % 60;
> > + mm = framenum / (fps*60) % 60;
> > + hh = framenum / (fps*3600);
> > + if (tc->flags & AVTIMECODE_FLAG_24HOURSMAX)
> > + hh = hh % 24;
> > + snprintf(buf, AVTIMECODE_STR_LEN, "%s%02d:%02d:%02d%c%02d",
> > + neg ? "-" : "",
> > + hh, mm, ss, drop ? ';' : ':', ff);
> > + return buf;
> > +}
> > +
>
> > +static int fps_from_frame_rate(AVRational rate)
> > +{
> > + if (!rate.den || !rate.num)
> > + return -1;
> > + return (rate.num + rate.den/2) / rate.den;
> > +}
> > +
>
> Move this down, just before the function where it is used.
>
Moved.
> > +static int check_timecode(void *avcl, AVTimecode *tc)
>
> avcl -> log_ctx?
>
Good idea, changed everywhere.
[...]
> > +/**
> > + * @file
> > + * Timecode helpers header
> > + */
> > +
> > +#ifndef AVUTIL_TIMECODE_H
> > +#define AVUTIL_TIMECODE_H
> > +
>
> > +#include <stdint.h>
>
> is this required?
>
I guess, there is a uint32_t in one prototype. Should we trust the user to
do that include?
> > +#include "rational.h"
>
> > +
> > +#define AVTIMECODE_STR_LEN 16
>
> General, AV_TIMECODE_*
>
Changed here and in every places I was using AVTIMECODE_*
> > +
> > +#define AVTIMECODE_OPTION(ctx, string_field, flags) \
> > + "timecode", "set timecode value following hh:mm:ss[:;.]ff format, " \
> > + "use ';' or '.' before frame number for drop frame", \
> > + offsetof(ctx, string_field), \
> > + AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX, flags
> > +
> > +enum AVTimecodeFlag {
> > + AVTIMECODE_FLAG_DROPFRAME = 1<<0,
> > + AVTIMECODE_FLAG_24HOURSMAX = 1<<1,
>
> > + AVTIMECODE_FLAG_NEGATIVEOK = 1<<2,
>
> NEGATIVEOK is *very* confusing, please use a less confusing name, in
> general "OK" should be banned from API.
>
This is taken from QuickTime specs; changed to ALLOWNEGATIVE
> > +};
>
> please shortly comment the meaning of each flag
>
Added.
> > +
> > +typedef struct {
> > + int start; ///< timecode frame start (first base frame number)
> > + uint32_t flags; ///< flags such as drop frame, +24 hours support, ...
> > + AVRational rate; ///< frame rate in rational form
> > + unsigned fps; ///< frame per second; must be consistent with the rate field
> > +} AVTimecode;
> > +
> > +/**
> > + * Get the timecode string from the 25-bit timecode format (MPEG GOP format).
> > + *
>
> > + * @param buf destination buffer, must be at least AVTIMECODE_STR_LEN long.
>
> nit, no need for final point
>
Fixed here and below.
> > + * @param tc25bit the 25-bits timecode
> > + * @return the buf parameter
> > + */
> > +char *av_mpegtc_to_timecode_string(char *buf, uint32_t tc25bit);
>
> For keeping consistent hierarchical naming scheme:
> av_timecode_mpegtc_to_string()
>
> alternatively:
> av_timecode_get_mpegtc_string()
>
> more consistent with the av_*_get_*_string()
> (e.g. av_get_pix_fmt_string(), get_sample_fmt_name(), also consistent
> with the convenction of keeping a verb in a function name.
>
I used the av_timecode_ and +get_ when necessary. Thank you for the
suggestion.
> > +
> > +/**
> > + * Adjust frame number for NTSC drop frame time code.
> > + *
> > + * @param framenum frame number to adjust
> > + * @return adjusted frame number
> > + * @warning adjustment is only valid in NTSC 29.97
> > + */
> > +int av_adjust_ntsc_framenum(int framenum);
>
> _timecode_ ? (ignore me if you think this function is not necessarily
> timecode-related).
>
It is actually timecode related, so _timecode_ added.
> > +
> > +/**
> > + * Convert frame number to SMPTE 12M binary representation.
> > + *
> > + * @param tc timecode data correctly initialized
> > + * @param framenum frame number
> > + * @return the SMPTE binary representation
> > + *
> > + * @note frame number adjustment is automatically done in case of drop timecode,
> > + * you do NOT have to call av_adjust_ntsc_framenum().
> > + * @note the frame number is relative to tc->start.
> > + */
> > +uint32_t av_framenum_to_smpte_timecode(const AVTimecode *tc, int framenum);
>
> I suggest:
> av_timecode_convert_framenum_to_smpte()
> av_timecode_get_smpte_from_framenum() => (shorter and more mnemonic)
>
Picked av_timecode_get_smpte_from_framenum()
> > +
> > +/**
> > + * Load timecode string in buf.
> > + *
>
> > + * @param buf destination buffer, must be at least AVTIMECODE_STR_LEN long.
>
> nit++, incomplete main sentence so no need of the final point
>
> > + * @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.
>
> nit++: complete sentences, Capitalize
>
nits fixed.
> > + */
> > +char *av_framenum_to_timecode_string(char *buf, const AVTimecode *tc, int framenum);
>
> uhm a bit confusing, as the timecode is obtained not only from the
> framenum.
> I'd say:
> av_timecode_get_string()
>
Yes, ok.
> > +/**
> > + * Init AVTimecode.
> > + *
>
> "Init AVTimecode" is a bit weird, it is like to say "Groom AVHorse",
> rather than "Groom an allocated multimedia horse".
>
Should be better now :)
> > + * @param avcl a pointer to an arbitrary struct of which the first field
> > + * is a pointer to an AVClass struct (used for av_log)
>
> I suggest log_ctx as it's used in many other places, also avcl is
> misleading as I'd tend to read it as "avclass"
>
> BTW usual dilemma for logging, we have the options:
> * use no log_ctx, messages are logged to the NULL context (BAD) or not
> at all (bad in case an error message may provide useful specific information)
> * use log_ctx for logging, but logging can't be inhibited
> * use log_ctx + log_level_offset (check av_expr_* API), you have
> control but the API gets more complex from the user POV
>
> Anyway I'm OK with log_ctx alone.
>
I'd like to keep the API simple, so I used log_ctx alone.
> > + * @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_init_timecode(void *avcl, AVTimecode *tc, AVRational rate, int flags, int frame_start);
>
> av_timecode_init()
>
OK
> > +
> > +/**
> > + * Parse timecode representation (hh:mm:ss[:;.]ff).
> > + *
>
> > + * @param avcl a pointer to an arbitrary struct of which the first field is a
> > + * pointer to an AVClass struct (used for av_log).
>
> ditto
>
OK
> > + * @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_init_timecode_from_string(void *avcl, AVTimecode *tc, AVRational rate, const char *str);
>
> av_timecode_init_from_string() should be fine()
>
Yes, changed.
[...]
--
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/20120123/800d4898/attachment.asc>
More information about the ffmpeg-devel
mailing list