[FFmpeg-devel] [PATCH] gxf: add timecode information to metadata

[Reimar Döffinger]

On Fri, Oct 28, 2011 at 01:57:04AM +0200, Matthieu Bouron wrote:
> +struct gxf_timecode {
> +    int hh;
> +    int mm;
> +    int ss;
> +    int frame;
> +    int drop_frame;
> +    int color_frame;
> +    int invalid;
> +};
> +
>  struct gxf_stream_info {
>      int64_t first_field;
>      int64_t last_field;
> @@ -31,6 +41,23 @@ struct gxf_stream_info {
>      int32_t fields_per_frame;
>  };
>  
> +static void parse_gxf_timecode(struct gxf_timecode *timecode, uint32_t t) {
> +    timecode->hh = t >> 24 & 0x1f;
> +    timecode->mm = t >> 16 & 0xff;
> +    timecode->ss = t >> 8 & 0xff;
> +    timecode->frame = t & 0xff;
> +    timecode->drop_frame = t >> 29 & 0x01;
> +    timecode->color_frame = t >> 30 & 0x01;
> +    timecode->invalid = t >> 31;
> +}
> +
> +static int gxf_timecode2str(struct gxf_timecode *t, char *buf, size_t len) {
> +    return snprintf(buf, len, "%02d:%02d:%02d%c%02d",
> +                    t->hh, t->mm, t->ss,
> +                    t->drop_frame ? ';' : ':',
> +                    t->frame);
> +}
> +
>  /**
>   * @brief parses a packet header, extracting type and length
>   * @param pb AVIOContext to read header from
[...]
> +    if (!timecode.invalid) {
> +        char buf[128];
> +        if (si->fields_per_frame == 2)
> +            timecode.frame = timecode.frame / 2;
> +        gxf_timecode2str(&timecode, buf, sizeof(buf));
> +        av_dict_set(&s->metadata, "gxf_timecode", buf, 0);

I don't think the way this code is split makes any sense,
particularly since it leaves that last part duplicated over
and over for no good reason.
There should be function like
int add_timecode_metadata(AVDictionary **pm, const char *key, uint32_t timecode, int fields_per_frame)
{
    char tmp[128];
    int field  =  timecode & 0xff;
    int frame  = fields_per_frame ? field / fields_per_frame : field;
    int second = (timecode >>  8) & 0xff;
    int minute = (timecode >> 16) & 0xff;
    int hour   = (timecode >> 24) & 0xff;
    int drop   =(timecode >> 29) & 1;
    // bit 30: color_frame, unused
    // ignore invalid time code
    if (timecode >> 31)
        return 0;
    snprintf(tmp, sizeof(tmp), "%02d:%02d:%02d%c%02d",
        hour, minute, second, drop ? ';' : ':', frame);
    return av_dict_set(pm, key, tmp, 0);
}


> @@ -313,6 +357,9 @@ static int gxf_header(AVFormatContext *s, AVFormatParameters *ap) {
>             continue;
>          }
>          track_id &= 0x3f;
> +        gxf_track_tags(s, pb, &track_len, si, track_type);

In addition passing the aux info out through si seems a lot more
reasonable than passing both s and track type in into the
gxf_track_tags function.

> @@ -340,9 +387,11 @@ static int gxf_header(AVFormatContext *s, AVFormatParameters *ap) {
>          }
>      }
>      if (pkt_type == PKT_UMF) {
> -        if (len >= 0x39) {
> +        if (len >= 0x51) {
> +            char buf[128];
> +            struct gxf_timecode timecode_in = {0};
> +            struct gxf_timecode timecode_out = {0};
>              AVRational fps;
> -            len -= 0x39;
>              avio_skip(pb, 5); // preamble
>              avio_skip(pb, 0x30); // payload description
>              fps = fps_umf2avr(avio_rl32(pb));
> @@ -353,6 +402,22 @@ static int gxf_header(AVFormatContext *s, AVFormatParameters *ap) {
>                  main_timebase.num = fps.den;
>                  main_timebase.den = fps.num * 2;
>              }
> +            avio_skip(pb, 0x10);
> +
> +            parse_gxf_timecode(&timecode_in, avio_rl32(pb));
> +            if (si->fields_per_frame == 2)
> +                timecode_in.frame = timecode_in.frame / 2;
> +            gxf_timecode2str(&timecode_in, buf, sizeof(buf));
> +            if (!timecode_in.invalid)
> +                av_dict_set(&s->metadata, "gxf_timecode_at_mark_in", buf, 0);
> +
> +            parse_gxf_timecode(&timecode_out, avio_rl32(pb));
> +            if (si->fields_per_frame == 2)
> +                timecode_out.frame = timecode_out.frame / 2;
> +            gxf_timecode2str(&timecode_out, buf, sizeof(buf));
> +            if (!timecode_out.invalid)
> +                av_dict_set(&s->metadata, "gxf_timecode_at_mark_out", buf, 0);
> +            len -= 0x51;

And since this is purely optional information while the fps info
is essential for correct playback separate len checks for each part
seems more reasonable to me.