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

[Mark Reid]

On Wed, Jan 14, 2015 at 1:37 AM, <tomas.hardin at codemill.se> wrote:

> 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)
>
>
yeah that sounds much better, I'll return if there isn't exactly one
essence partition.  I'm pretty sure OPAtom are only suppose to only have
one,  all the files I have with this issue do.


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

Its taken care of in mxf_handle_small_eubc
if edit_unit_byte_count < 32 it sets mxf->edit_units_per_packet = 1920

Would you rather I set mxf->edit_units_per_packet this new function too?

I'll send a new patch incorporating these changes, thanks for taking the
time to review.