[FFmpeg-devel] [PATCH v3] libavformat/mxfdec.c: support demuxing opatom audio without index

[tomas.hardin at codemill.se]

On 2015-01-12 02:18, Mark Reid wrote:
> changes since v2:
> *removed log line and changed av_mallocz sizeof
> 
> ---
>  libavformat/mxfdec.c | 55 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 4715169..743a6a0 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -2422,6 +2422,60 @@ static void 
> mxf_handle_small_eubc(AVFormatContext *s)
>      mxf->edit_units_per_packet = 1920;
>  }
> 
> +/**
> + * Deal with the case where OPAtom files does not have any 
> IndexTableSegments.
> + */
> +static int mxf_handle_missing_index_segment(MXFContext *mxf)
> +{
> +    AVFormatContext *s = mxf->fc;
> +    AVStream *st = NULL;
> +    MXFIndexTableSegment *segment = NULL;
> +    MXFPartition *p = NULL;
> +
> +    int i, ret;
> +
> +    if (mxf->op != OPAtom)
> +        return 0;
> +
> +    /* TODO: support raw video without a index if they exist */
> +    if (s->nb_streams != 1 || s->streams[0]->codec->codec_type !=
> AVMEDIA_TYPE_AUDIO || !is_pcm(s->streams[0]->codec->codec_id))
> +        return 0;
> +
> +    /* check if file already has a IndexTableSegment */
> +    for (i = 0; i < mxf->metadata_sets_count; i++) {
> +        if (mxf->metadata_sets[i]->type == IndexTableSegment)
> +            return 0;
> +    }
> +
> +    /* find the essence partition */
> +    for (i = 0; i < mxf->partitions_count; i++) {
> +        if(mxf->partitions[i].essence_offset <= 0)
> +            continue;
> +        p = &mxf->partitions[i];
> +        break;
> +    }

This should be done for all essence partitions, or it should be limited 
to files with only one essence partition.
Since I prefer to be conservative with MXF hacks I'd prefer the latter 
(return if EP count != 1), unless of course there's a file with multiple 
essence partitions where this code is required :)
(I forget whether OPAtom can ever have more than one essence partition, 
it wouldn't surprise me if the spec is silent on the matter)

> +    if (!p)
> +        return AVERROR_INVALIDDATA;
> +
> +    segment = av_mallocz(sizeof(*segment));
> +    if (!segment)

You could contract this to

     if (!(segment = av_mallocz(sizeof(*segment))))

to stay consistent with the next if statement

> +        return AVERROR(ENOMEM);
> +
> +    if (ret = mxf_add_metadata_set(mxf, segment)) {

Add another pair of parens around this, like so:

     if ((ret = mxf_add_metadata_set(mxf, segment))) {

Makes gcc happier.

> +        mxf_free_metadataset((MXFMetadataSet**)&segment);
> +        return ret;
> +    }
> +
> +    st = s->streams[0];
> +    segment->type = IndexTableSegment;
> +    segment->edit_unit_byte_count = 
> (av_get_bits_per_sample(st->codec->codec_id) * st->codec->channels) >> 
> 3;

Uhm, I think this should be larger? Or does it get adjusted up in the 
edit_unit_byte_count handling stuff elsewhere in the code?
We don't want to demuxer to return 4-byte PCM packets..

/Tomas