[FFmpeg-devel] [PATCH] mxfdec: add timecode to metadata

[Michael Niedermayer]

On Fri, Mar 09, 2012 at 06:33:30PM +0100, Matthieu Bouron wrote:
> 2012/2/23 Matthieu Bouron <matthieu.bouron at gmail.com>:
> > 2012/2/23 Clément Bœsch <ubitux at gmail.com>:
> >> On Thu, Feb 23, 2012 at 06:00:11PM +0100, Matthieu Bouron wrote:
> >> [...]
> >>> +static int mxf_read_timecode_component(void *arg, AVIOContext *pb, int tag, int size, UID uid, int64_t klv_offset)
> >>> +{
> >>> +    MXFTimecodeComponent *mxf_timecode = arg;
> >>> +    switch(tag) {
> >>> +    case 0x1501:
> >>> +        mxf_timecode->start_frame = avio_rb64(pb);
> >>> +        break;
> >>> +    case 0x1502:
> >>> +        mxf_timecode->rate = (AVRational){avio_rb16(pb), 1};
> >>> +        break;
> >>> +    case 0x1503:
> >>> +        mxf_timecode->drop_frame = avio_r8(pb);
> >>> +        break;
> >>> +    }
> >>> +    return 0;
> >>> +}
> >>> +
> >>>  static int mxf_read_track(void *arg, AVIOContext *pb, int tag, int size, UID uid, int64_t klv_offset)
> >>>  {
> >>>      MXFTrack *track = arg;
> >>> @@ -1233,6 +1260,14 @@ finish_decoding_index:
> >>>      return ret;
> >>>  }
> >>>
> >>> +static int mxf_add_timecode_metadata(AVDictionary **pm, const char *key, AVTimecode *tc)
> >>> +{
> >>> +    char buf[16];
> >>
> >> AV_TIMECODE_STR_SIZE instead of 16
> >>
> >>> +    av_dict_set(pm, key, av_timecode_make_string(tc, buf, 0), 0);
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>>  static int mxf_parse_structural_metadata(MXFContext *mxf)
> >>>  {
> >>>      MXFPackage *material_package = NULL;
> >>> @@ -1257,24 +1292,44 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
> >>>          MXFTrack *temp_track = NULL;
> >>>          MXFDescriptor *descriptor = NULL;
> >>>          MXFStructuralComponent *component = NULL;
> >>> +        MXFTimecodeComponent *mxf_tc = NULL;
> >>>          UID *essence_container_ul = NULL;
> >>>          const MXFCodecUL *codec_ul = NULL;
> >>>          const MXFCodecUL *container_ul = NULL;
> >>>          AVStream *st;
> >>> +        AVTimecode tc;
> >>>
> >>>          if (!(material_track = mxf_resolve_strong_ref(mxf, &material_package->tracks_refs[i], Track))) {
> >>>              av_log(mxf->fc, AV_LOG_ERROR, "could not resolve material track strong ref\n");
> >>>              continue;
> >>>          }
> >>>
> >>> +        if ((component = mxf_resolve_strong_ref(mxf, &material_track->sequence_ref, TimecodeComponent))) {
> >>> +            mxf_tc = (MXFTimecodeComponent*)component;
> >>> +            if (av_timecode_init(&tc, mxf_tc->rate, mxf_tc->drop_frame == 1, mxf_tc->start_frame, mxf) == 0) {
> >>
> >> Better use a variable like flags = mxf_tc->drop_frame == 1 ?
> >> AV_TIMECODE_FLAG_DROPFRAME : 0, instead of assuming the value of the flag.
> >>
> >>> +                mxf_add_timecode_metadata(&mxf->fc->metadata, "timecode", &tc);
> >>> +            }
> >>> +        }
> >>> +
> >>>          if (!(material_track->sequence = mxf_resolve_strong_ref(mxf, &material_track->sequence_ref, Sequence))) {
> >>>              av_log(mxf->fc, AV_LOG_ERROR, "could not resolve material track sequence strong ref\n");
> >>>              continue;
> >>>          }
> >>>
> >>> +        for (j = 0; j < material_track->sequence->structural_components_count; j++) {
> >>> +            component = mxf_resolve_strong_ref(mxf, &material_track->sequence->structural_components_refs[j], TimecodeComponent);
> >>> +            if (!component)
> >>> +                continue;
> >>> +
> >>> +            mxf_tc = (MXFTimecodeComponent*)component;
> >>> +            if (av_timecode_init(&tc, mxf_tc->rate, mxf_tc->drop_frame == 1, mxf_tc->start_frame, mxf) == 0) {
> >>
> >> Same as above
> >>
> >>> +                mxf_add_timecode_metadata(&mxf->fc->metadata, "timecode", &tc);
> >>
> >> It looks like you are replacing the timecode metadata, I guess it's
> >> desired?
> >
> > Structural information should only contain zero or one timecode track,
> > so i guess it's not a problem.
> >
> >>
> >> BTW, what about the SMPTE timecode in the "system item"?
> >
> > I think It could be part of an another patch.
> >
> >>
> >> Also, you might want to update doc/general.texi if the feature is
> >> complete.
> >
> > New patch attached.
> > Thanks
> 
> Ping.
> Rebased patch attached.

I interpret Tomas "Looks OK overall." from 2 month ago as that he
is ok with this and it looks ok to me too thus
applied

thanks

sorry for the long delay


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120311/f37d73bd/attachment.asc>