[FFmpeg-devel] [PATCH 3/3] mov: support timecode extraction.

[Clément Bœsch]

On Wed, Jan 04, 2012 at 11:07:27PM -0800, Baptiste Coudurier wrote:
[...]
> > +static int parse_timecode_in_framenum_format(AVFormatContext *s, AVStream *st,
> > +                                             uint32_t value)
> > +{
> > +    char buf[16];
> > +    struct ff_timecode tc = {
> > +        .drop  = st->codec->flags2 & CODEC_FLAG2_DROP_FRAME_TIMECODE,
> > +        .rate  = (AVRational){st->codec->time_base.den,
> > +                              st->codec->time_base.num},
> > +    };
> > +
> > +    if (avpriv_check_timecode_rate(s, tc.rate, tc.drop) < 0)
> > +        return AVERROR(EINVAL);
> > +    av_dict_set(&st->metadata, "timecode",
> > +                avpriv_timecode_to_string(buf, &tc, value), 0);
> 
> Overall the API is pretty ugly.

Yes, that's far from perfect because I couldn't think of all the usages at
first.

> Why 2 params for check_timecode_rate when passing a struct tcblah would
> be sufficient.

This was an internal function at first and thus wasn't opaque for clarity.
Yes I could export it using the struct for usage between libs; I'll update
the patchset.

> Why timecode_to_string when it's obviously a frame number instead of a timecode ?

Agreed, framenum_to_timecode_str() could be better. Still, the function
mainly use the timecode struct, so the current name makes some sense, a
bit. I'll certainly think of a better naming when the API will be moved
(and publicly exported?) to lavu.

> Why a struct tcblah + value instead of one struct tc ?
> 

The tcblah struct may be seen like an opaque context struct, and the value
as the user input. Ideally we would have a init_timecode(AVTimecode *tc,
AVRationnal rate, int drop, unsigned frame_start); AVTimecode being
opaque, the user will have some need to change the frame number without
updating the struct.

I'll certainly consider all these API changes when the timecode support
will be in a better shape. Thanks for pointing out these issues.

[...]
> >  
> > -    if (pb->seekable && mov->chapter_track > 0)
> > -        mov_read_chapters(s);
> > +    if (pb->seekable) {
> > +        int i;
> > +        if (mov->chapter_track > 0)
> > +            mov_read_chapters(s);
> > +        for (i = 0; i < s->nb_streams; i++)
> > +            if (s->streams[i]->codec->codec_tag == MKTAG('t','m','c','d'))
> 
> AV_RL32("tmcd")
> 

OK, changed locally. I don't see much usage of AV_RL32() for this, any
reason to prefer it over MKTAG?

-- 
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/20120105/b8210ede/attachment.asc>