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

[Tomas Härdin]

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?

>  };
>  
>  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.

>      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.

> +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.

> +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.

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141128/b69adfad/attachment.asc>