On Wed, Jan 14, 2015 at 1:37 AM, <tomas.har...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel