[FFmpeg-devel] [PATCH] avformat/mxfdec: read reel_name and source timecode from physical source package

[Mark Reid]

On Sun, Sep 28, 2014 at 3:16 AM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> On Sun, Sep 28, 2014 at 11:04:34AM +0200, Tomas Härdin wrote:
> > On Thu, 2014-09-25 at 16:13 -0700, Mark Reid wrote:
> > > ---
> > >  libavformat/mxfdec.c | 118
> ++++++++++++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 97 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index 7a4633f..3a1889f 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -188,6 +188,7 @@ typedef struct {
> > >      int tracks_count;
> > >      MXFDescriptor *descriptor; /* only one */
> > >      UID descriptor_ref;
> > > +    char *name;
> > >  } MXFPackage;
> > >
> > >  typedef struct {
> > > @@ -731,6 +732,27 @@ static int mxf_read_sequence(void *arg,
> AVIOContext *pb, int tag, int size, UID
> > >      return 0;
> > >  }
> > >
> > > +static int mxf_read_utf16_string(AVIOContext *pb, int size, char**
> str)
> > > +{
> > > +    int ret;
> > > +    size_t buf_size;
> > > +
> > > +    if (size < 0)
> > > +        return AVERROR(EINVAL);
> > > +
> > > +    buf_size = size + size / 2 + 1;
> >
> > This should be a function, like ff_utf16_buflen() or something. I see
> > that this hunk is just moving the function though, so don't let that
> > hold this patch up.
> >
> > > +    *str = av_malloc(buf_size);
> > > +    if (!*str)
> > > +        return AVERROR(ENOMEM);
> > > +
> > > +    if ((ret = avio_get_str16be(pb, size, *str, buf_size)) < 0) {
> > > +        av_freep(str);
> > > +        return ret;
> > > +    }
> > > +
> > > +    return ret;
> > > +}
> >
> >
> > > +static int mxf_parse_physical_source_package(MXFContext *mxf,
> MXFTrack *source_track, AVStream *st)
> > > +{
> > >      [...]
>
> > > +
> > > +            for (k = 0; k <
> physical_track->sequence->structural_components_count; k++) {
> > > +                component = mxf_resolve_strong_ref(mxf,
> &physical_track->sequence->structural_components_refs[k],
> TimecodeComponent);
> > > +                if (!component)
> > > +                    continue;
> > > +
> > > +                mxf_tc = (MXFTimecodeComponent*)component;
> > > +                flags = mxf_tc->drop_frame == 1 ?
> AV_TIMECODE_FLAG_DROPFRAME : 0;
> > > +                /* scale sourceclip start_position to match physical
> track edit rate */
> > > +                start_position =
> av_rescale_q(sourceclip->start_position, av_inv_q(source_track->edit_rate),
> av_inv_q(physical_track->edit_rate));
> >
> > av_rescale()
>
> edit_rate is a AVRational, so av_rescale_q() seems fitting to me
> or do i misunderstand ?
> the av_inv_q() could be avoided though by exchanging the 2 arguments
>
> [...]
>

yeah, the arguments should simply be reversed, i was just thinking
backwards, I'll fix it and send a new patch. I did a bit of fuzz testing
with zzuf, but I'll do a bit more before submitting again. Thanks for
taking the time to review.