[PATCH v3] libavformat/mxfdec.c: support demuxing opatom audio without index
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; + } + if (!p) + return AVERROR_INVALIDDATA; + + segment = av_mallocz(sizeof(*segment)); + if (!segment) + return AVERROR(ENOMEM); + + if (ret = mxf_add_metadata_set(mxf, segment)) { + 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; + segment->index_start_position = 0; + segment->index_duration = s->streams[0]->duration; + segment->index_sid = p->index_sid; + segment->body_sid = p->body_sid; + return 0; +} + static void mxf_read_random_index_pack(AVFormatContext *s) { MXFContext *mxf = s->priv_data; @@ -2584,6 +2638,7 @@ static int mxf_read_header(AVFormatContext *s) if ((ret = mxf_parse_structural_metadata(mxf)) < 0) goto fail; + mxf_handle_missing_index_segment(mxf); if ((ret = mxf_compute_index_tables(mxf)) < 0) goto fail; -- 2.2.1
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
On Wed, Jan 14, 2015 at 1:37 AM, <tomas.hardin@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.
participants (2)
-
Mark Reid -
tomas.hardin＠codemill.se