[FFmpeg-devel] [PATCH] MXF index table based seeking
Tomas Härdin
tomas.hardin at codemill.se
Wed Jun 22 14:10:24 CEST 2011
I took some time today to give these patches a bit more of a review.
Partial review follows:
> From 912ad3f89b75a6b99a9ffd700757cbd57508ab9d Mon Sep 17 00:00:00 2001
> From: Georg Lippitsch <georg at linteum.at>
> Date: Sun, 15 May 2011 15:15:20 +0200
> Subject: [PATCH] MXF index table based seeking
>
> ---
> snip
> +static void mxf_read_delta_entry_array(AVIOContext *pb, MXFIndexTableSegment *segment)
> +{
> + int i;
> + int length;
> + segment->nb_delta_entries = avio_rb32(pb);
> + length = avio_rb32(pb);
> + segment->slice =
> + av_malloc(segment->nb_delta_entries * sizeof(*segment->slice));
> + segment->element_delta =
> + av_malloc(segment->nb_index_entries * sizeof(*segment->element_delta));
> + for (i = 0; i < segment->nb_delta_entries; i++) {
> + avio_r8(pb);
> + segment->slice[i] = avio_r8(pb);
> + segment->element_delta[i] = avio_rb32(pb);
> + }
> +}
> +
> +static void mxf_read_index_entry_array(AVIOContext *pb, MXFIndexTableSegment *segment)
> +{
> + int i, j;
> + int length;
> + segment->nb_index_entries = avio_rb32(pb);
> + length = avio_rb32(pb);
> + segment->flag_entries =
> + av_malloc(segment->nb_index_entries * sizeof(*segment->flag_entries));
> + segment->stream_offset_entries =
> + av_malloc(segment->nb_index_entries * sizeof(*segment->stream_offset_entries));
> + if (segment->slice_count)
> + segment->slice_offset_entries =
> + av_malloc(segment->nb_index_entries * sizeof(*segment->slice_offset_entries));
> + for (i = 0; i < segment->nb_index_entries; i++) {
> + avio_rb16(pb);
> + segment->flag_entries[i] = avio_r8(pb);
> + segment->stream_offset_entries[i] = avio_rb64(pb);
> + if (segment->slice_count) {
> + segment->slice_offset_entries[i] =
> + av_malloc(segment->slice_count * sizeof(**segment->slice_offset_entries));
> + for (j = 0; j < segment->slice_count; j++) {
> + segment->slice_offset_entries[i][j] = avio_rb32(pb);
> + }
> + avio_skip(pb, length - 11 - 4 * segment->slice_count);
> + } else
> + avio_skip(pb, length - 11);
> + }
> +}
You might want to make sure any of the allocations in here failing
doesn't cause a crash.
> @@ -837,6 +934,46 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
Will review this later.
> @@ -907,6 +1044,7 @@ static int mxf_read_header(AVFormatContext *s, AVFormatParameters *ap)
> {
> MXFContext *mxf = s->priv_data;
> KLVPacket klv;
> + int64_t file_size = avio_size(s->pb);
>
> if (!mxf_read_sync(s->pb, mxf_header_partition_pack_key, 14)) {
> av_log(s, AV_LOG_ERROR, "could not find header partition pack key\n");
> @@ -914,18 +1052,24 @@ static int mxf_read_header(AVFormatContext *s, AVFormatParameters *ap)
> }
> avio_seek(s->pb, -14, SEEK_CUR);
> mxf->fc = s;
> - while (!url_feof(s->pb)) {
> + while (avio_tell(s->pb) < file_size) {
> const MXFMetadataReadTableEntry *metadata;
>
> if (klv_read_packet(&klv, s->pb) < 0)
> return -1;
Use break instead of returning and drop file_size/avio_tell in favor of
keeping the url_feof().
> PRINT_KEY(s, "read header", klv.key);
> av_dlog(s, "size %lld offset %#llx\n", klv.length, klv.offset);
> +
Stray newline
> if (IS_KLV_KEY(klv.key, mxf_encrypted_triplet_key) ||
> - IS_KLV_KEY(klv.key, mxf_essence_element_key)) {
> - /* FIXME avoid seek */
> - avio_seek(s->pb, klv.offset, SEEK_SET);
> - break;
> + IS_KLV_KEY(klv.key, mxf_essence_element_key) ||
> + IS_KLV_KEY(klv.key, mxf_system_item_key)) {
> + if (!mxf->essence_offset) {
> + mxf->essence_offset = klv.offset;
> + if (IS_KLV_KEY(klv.key, mxf_system_item_key))
> + mxf->system_item = 1;
> + }
> + avio_skip(s->pb, klv.length);
> + continue;
> }
>
> for (metadata = mxf_metadata_read_table; metadata->read; metadata++) {
> @@ -945,13 +1089,15 @@ static int mxf_read_header(AVFormatContext *s, AVFormatParameters *ap)
> if (!metadata->read)
> avio_skip(s->pb, klv.length);
> }
> + /* FIXME avoid seek */
There need to be an if (!mxf->essence_offset) return here if you go with
my "keep using url_feof()" suggestion. I see there's one in the next
patch - move it here instead.
> + avio_seek(s->pb, mxf->essence_offset, SEEK_SET);
> return mxf_parse_structural_metadata(mxf);
> }
>
> static int mxf_read_close(AVFormatContext *s)
> {
> MXFContext *mxf = s->priv_data;
> - int i;
> + int i, j;
>
> av_freep(&mxf->packages_refs);
>
> @@ -970,6 +1116,15 @@ static int mxf_read_close(AVFormatContext *s)
> case MaterialPackage:
> av_freep(&((MXFPackage *)mxf->metadata_sets[i])->tracks_refs);
> break;
> + case IndexTableSegment:
> + for (j = 0; j < ((MXFIndexTableSegment *)mxf->metadata_sets[i])->nb_index_entries; j++)
> + av_freep(&((MXFIndexTableSegment *)mxf->metadata_sets[i])->slice_offset_entries[j]);
> + av_freep(&((MXFIndexTableSegment *)mxf->metadata_sets[i])->slice);
> + av_freep(&((MXFIndexTableSegment *)mxf->metadata_sets[i])->element_delta);
> + av_freep(&((MXFIndexTableSegment *)mxf->metadata_sets[i])->flag_entries);
> + av_freep(&((MXFIndexTableSegment *)mxf->metadata_sets[i])->stream_offset_entries);
> + av_freep(&((MXFIndexTableSegment *)mxf->metadata_sets[i])->slice_offset_entries);
Consider simplifying using a MXFIndexTableSegment pointer. That would
make this part more readable.
> + break;
> default:
> break;
> }
> @@ -997,20 +1152,30 @@ static int mxf_probe(AVProbeData *p) {
> return 0;
> }
>
> -/* rudimentary byte seek */
> -/* XXX: use MXF Index */
> static int mxf_read_seek(AVFormatContext *s, int stream_index, int64_t sample_time, int flags)
> {
> AVStream *st = s->streams[stream_index];
> - int64_t seconds;
>
> - if (!s->bit_rate)
> - return -1;
> - if (sample_time < 0)
> - sample_time = 0;
> - seconds = av_rescale(sample_time, st->time_base.num, st->time_base.den);
> - avio_seek(s->pb, (s->bit_rate * seconds) >> 3, SEEK_SET);
> - av_update_cur_dts(s, st, sample_time);
> + if (st->nb_index_entries) {
> + int index = av_index_search_timestamp(st, sample_time, flags);
> + av_dlog(s, "stream %d, timestamp %"PRId64", sample %d\n", st->index, sample_time, index);
> + if (index < 0 && sample_time < st->index_entries[0].timestamp)
> + index = 0;
> + if (index < 0)
> + return -1;
> + avio_seek(s->pb, st->index_entries[index].pos, SEEK_SET);
> + av_update_cur_dts(s, st, st->index_entries[index].timestamp);
> + } else {
> + int64_t seconds;
> + if (!s->bit_rate)
> + return -1;
> + if (sample_time < 0)
> + sample_time = 0;
> + seconds = av_rescale(sample_time, st->time_base.num, st->time_base.den);
> + avio_seek(s->pb, (s->bit_rate * seconds) >> 3, SEEK_SET);
> + av_update_cur_dts(s, st, sample_time);
Please avoid mixing functional and cosmetic changes. I can see that this
doesn't change the behavior of the else-case though.
> From 0661022b30eb8c447a5a3da0b38937253ebb2b45 Mon Sep 17 00:00:00 2001
> From: Georg Lippitsch <georg at linteum.at>
> Date: Mon, 16 May 2011 20:37:28 +0200
> Subject: [PATCH 2/2] Index table based handling of clip-wrapped mxf files
>
> ---
> enum MXFWrappingScheme {
> @@ -322,8 +327,24 @@ static int mxf_decrypt_triplet(AVFormatContext *s, AVPacket *pkt, KLVPacket *klv
> static int mxf_read_packet(AVFormatContext *s, AVPacket *pkt)
> @@ -761,6 +792,7 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
Will review later as well..
> @@ -1068,6 +1149,10 @@ static int mxf_read_header(AVFormatContext *s, AVFormatParameters *ap)
> if (IS_KLV_KEY(klv.key, mxf_system_item_key))
> mxf->system_item = 1;
> }
> + if (!mxf->first_essence_kl_length && IS_KLV_KEY(klv.key, mxf_essence_element_key)) {
else if maybe?
> + mxf->first_essence_kl_length = avio_tell(s->pb) - klv.offset;
> + mxf->first_essence_length = klv.length;
> + }
> avio_skip(s->pb, klv.length);
> continue;
> }
> @@ -1090,6 +1175,8 @@ static int mxf_read_header(AVFormatContext *s, AVFormatParameters *ap)
> avio_skip(s->pb, klv.length);
> }
> /* FIXME avoid seek */
> + if (!mxf->essence_offset)
> + return -1;
Move this to the first patch as I mentioned earlier. Also perhaps
consider a better return value like AVERROR_INVALIDDATA.
> avio_seek(s->pb, mxf->essence_offset, SEEK_SET);
> return mxf_parse_structural_metadata(mxf);
> }
> @@ -1155,6 +1242,9 @@ static int mxf_probe(AVProbeData *p) {
> static int mxf_read_seek(AVFormatContext *s, int stream_index, int64_t sample_time, int flags)
> {
> AVStream *st = s->streams[stream_index];
> + MXFContext* mxf = s->priv_data;
> + MXFTrack *tr = st->priv_data;
> + int64_t seekpos;
>
> if (st->nb_index_entries) {
> int index = av_index_search_timestamp(st, sample_time, flags);
> @@ -1163,8 +1253,9 @@ static int mxf_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
> index = 0;
> if (index < 0)
> return -1;
> - avio_seek(s->pb, st->index_entries[index].pos, SEEK_SET);
> + seekpos = st->index_entries[index].pos;
> av_update_cur_dts(s, st, st->index_entries[index].timestamp);
> + tr->current_sample_index = index;
> } else {
> int64_t seconds;
> if (!s->bit_rate)
> @@ -1172,10 +1263,23 @@ static int mxf_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
> if (sample_time < 0)
> sample_time = 0;
> seconds = av_rescale(sample_time, st->time_base.num, st->time_base.den);
> - avio_seek(s->pb, (s->bit_rate * seconds) >> 3, SEEK_SET);
> + seekpos = (s->bit_rate * seconds);
>> 3 lost.
> av_update_cur_dts(s, st, sample_time);
> }
>
> + if (mxf->current_klv_data.length) {
> + if (mxf->current_klv_index == stream_index) {
> + int64_t seekoffset = seekpos - avio_tell(s->pb);
> + mxf->current_klv_data.offset += seekoffset;
> + if (seekoffset < (int64_t)mxf->current_klv_data.length)
> + mxf->current_klv_data.length -= seekoffset;
> + else
> + mxf->current_klv_data.length = 0;
> + }
> + }
> +
> + avio_seek(s->pb, seekpos, SEEK_SET);
> +
> return 0;
> }
I need to find some P2 AVC-Intra 50 samples to test this stuff properly.
Hopefully the above should keep you a little busy.
If you like I can attach an updated set of patches that have been
rebased, have some of the things I mentioned above fixed and have your
recent fix patches squashed in.
/Tomas
More information about the ffmpeg-devel
mailing list