[FFmpeg-devel] [PATCH] libavformat/mxfdec.c: initial support for EssenceGroups

[Mark Reid]

On Fri, Nov 28, 2014 at 9:26 AM, Tomas Härdin <tomas.hardin at codemill.se>
wrote:

> On Tue, 2014-11-25 at 15:14 -0800, Mark Reid wrote:
> > ---
> >  libavformat/mxf.c      |   1 +
> >  libavformat/mxf.h      |   1 +
> >  libavformat/mxfdec.c   | 148
> ++++++++++++++++++++++++++++++++++++++++---------
> >  tests/ref/lavf/mxf     |   6 +-
> >  tests/ref/lavf/mxf_d10 |   2 +-
> >  5 files changed, 127 insertions(+), 31 deletions(-)
> >
> > diff --git a/libavformat/mxf.c b/libavformat/mxf.c
> > index 4dc54d7..14d143e 100644
> > --- a/libavformat/mxf.c
> > +++ b/libavformat/mxf.c
> > @@ -94,6 +94,7 @@ static const struct {
> >      {AV_PIX_FMT_RGB565BE,{'R', 5,  'G', 6,  'B', 5
>    }},
> >      {AV_PIX_FMT_RGBA,    {'R', 8,  'G', 8,  'B', 8, 'A', 8
>    }},
> >      {AV_PIX_FMT_PAL8,    {'P', 8
>    }},
> > +    {AV_PIX_FMT_GRAY8,   {'A', 8
>    }},
>
> There's no pixel format for pure alpha?
>

I was looking for a  AV_PIX_FMT_A8 or something like that but could find
one.

http://ffmpeg.org/doxygen/trunk/pixfmt_8h.html#a9a8e335cf3be472042bc9f0cf80cd4c5

AV_PIX_FMT_GRAY8  has the right buffer size, 1 channel 8bpp.
AV_PIX_FMT_YA8 or AV_PIX_FMT_GRAY8A are 4 channels.


> >  };
> >
> >  static const int num_pixel_layouts =
> FF_ARRAY_ELEMS(ff_mxf_pixel_layouts);
> > diff --git a/libavformat/mxf.h b/libavformat/mxf.h
> > index 5b95efa..63b497a 100644
> > --- a/libavformat/mxf.h
> > +++ b/libavformat/mxf.h
> > @@ -35,6 +35,7 @@ enum MXFMetadataSetType {
> >      TimecodeComponent,
> >      PulldownComponent,
> >      Sequence,
> > +    EssenceGroup,
>
> Add this to the end of the enum? That way mxfenc output doesn't change.
>

Okay I'll do that, I just didn't because of the scary comment at the bottom
of the enum

           TypeBottom,// add metadata type before this


> >      MultipleDescriptor,
> >      Descriptor,
> >      Track,
> >  [...]
> >
> > +static int mxf_read_essence_group(void *arg, AVIOContext *pb, int tag,
> int size, UID uid, int64_t klv_offset)
> > +{
> > +    MXFEssenceGroup *essence_group = arg;
> > +    switch (tag) {
> > +    case 0x0202:
> > +        essence_group->duration = avio_rb64(pb);
> > +        break;
> > +    case 0x0501:
> > +        essence_group->structural_components_count = avio_rb32(pb);
> > +        essence_group->structural_components_refs =
> av_calloc(essence_group->structural_components_count, sizeof(UID));
> > +        if (!essence_group->structural_components_refs)
> > +            return AVERROR(ENOMEM);
> > +        avio_skip(pb, 4); /* useless size of objects, always 16
> according to specs */
> > +        avio_read(pb, (uint8_t
> *)essence_group->structural_components_refs,
> essence_group->structural_components_count * sizeof(UID));
>
> Is there any risk of this multiplication overflowing? I suspect
> av_calloc() will fail if it does, just making sure. Also not critical if
> it actually does since avio_read() just ends up reading less.
>

I copied the same code from the mxf_read_sequence func, so I assumed it was
okay.
http://ffmpeg.org/doxygen/trunk/mem_8c_source.html#l00251
av_calloc does check for a overflow and will return NULL if nmemb >=
INT_MAX / size. So it should fail before even getting to the avio_read.


> > +static MXFStructuralComponent*
> mxf_resolve_essence_group_choice(MXFContext *mxf, MXFEssenceGroup
> *essence_group)
> > +{
> > +    MXFStructuralComponent *component = NULL;
> > +    MXFPackage *package = NULL;
> > +    MXFDescriptor *descriptor = NULL;
> > +    int i;
> > +
> > +    if (!essence_group || !essence_group->structural_components_count)
> > +        return NULL;
> > +
> > +    /* essence groups contains multiple representations of the same
> media,
> > +       this return the first components with a valid Descriptor
> typically index 0 */
> > +    for (i =0; i < essence_group->structural_components_count; i++){
> > +        component = mxf_resolve_strong_ref(mxf,
> &essence_group->structural_components_refs[i], SourceClip);
> > +        if (!component)
> > +            continue;
> > +
> > +        if (!(package = mxf_resolve_source_package(mxf,
> component->source_package_uid)))
> > +            continue;
> > +
> > +        descriptor = mxf_resolve_strong_ref(mxf,
> &package->descriptor_ref, Descriptor);
> > +        if (descriptor){
> > +            /* HACK: force the duration of the component to match the
> duration of the descriptor */
> > +            if (descriptor->duration != AV_NOPTS_VALUE)
> > +                component->duration = descriptor->duration;
>
> Not a huge fan of this, but probably doesn't hurt since it's checking
> for AV_NOPTS_VALUE.
>
>
I was planing of removing this in a future patch. I was going to move the
logic into mxf_parse_structural_metadata, but need to refactor and shuffle
somethings around in there to get it to work.


> > +static int mxf_metadataset_init(MXFMetadataSet *ctx, enum
> MXFMetadataSetType type)
> > +{
> > +    switch (type){
> > +    case MultipleDescriptor:
> > +    case Descriptor:
> > +        ((MXFDescriptor*)ctx)->pix_fmt = AV_PIX_FMT_NONE;
> > +        ((MXFDescriptor*)ctx)->duration = AV_NOPTS_VALUE;
> > +        break;
> > +    default:
> > +        break;
> > +    }
> > +    return 0;
> > +}
> > +
>
> Good idea :)
>
> > diff --git a/tests/ref/lavf/mxf b/tests/ref/lavf/mxf
> > index 236661c..d237560 100644
> > --- a/tests/ref/lavf/mxf
> > +++ b/tests/ref/lavf/mxf
>
> Again, probably not needed if you stick EssenceGroup at the end of the
> enum.
>
> I like using mxf_resolve_source_package() to reduce the size of
> mxf_parse_physical_source_package() and mxf_parse_structural_metadata().
>
> Memory handling looks correct too. Did you double-check with valgrind?


> Looks pretty good overall.
>

Great, I'll double check it with valgrind, remove the changes to the tests,
put EssenceGroup at the end of the enum and send a new patch.
thanks for taking the time to review.


> /Tomas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>