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

[Matthieu Bouron]

Patch updated,
Does it look good to you ?

2011/10/29 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-gxf-add-timecode-information-to-metadata.patch
Type: application/octet-stream
Size: 4342 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111031/25e6f022/attachment.obj>