[PATCH] lavf/mov: add support for sidx fragment indexes
Fixes trac #3842 --- libavformat/isom.h | 2 + libavformat/mov.c | 245 ++++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 208 insertions(+), 39 deletions(-) diff --git a/libavformat/isom.h b/libavformat/isom.h index aee9d6e..6e921c0 100644 --- a/libavformat/isom.h +++ b/libavformat/isom.h @@ -103,6 +103,7 @@ typedef struct MOVSbgp { typedef struct MOVFragmentIndexItem { int64_t moof_offset; int64_t time; + int headers_read; } MOVFragmentIndexItem; typedef struct MOVFragmentIndex { @@ -197,6 +198,7 @@ typedef struct MOVContext { int has_looked_for_mfra; MOVFragmentIndex** fragment_index_data; unsigned fragment_index_count; + int fragment_index_complete; int atom_depth; unsigned int aax_mode; ///< 'aax' file has been detected uint8_t file_key[20]; diff --git a/libavformat/mov.c b/libavformat/mov.c index da170a6..8463d57 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -3349,7 +3349,7 @@ static int mov_read_tfhd(MOVContext *c, AVIOContext *pb, MOVAtom atom) MOVFragment *frag = &c->fragment; MOVTrackExt *trex = NULL; MOVFragmentIndex* index = NULL; - int flags, track_id, i; + int flags, track_id, i, found = 0; avio_r8(pb); /* version */ flags = avio_rb24(pb); @@ -3367,15 +3367,6 @@ static int mov_read_tfhd(MOVContext *c, AVIOContext *pb, MOVAtom atom) av_log(c->fc, AV_LOG_ERROR, "could not find corresponding trex\n"); return AVERROR_INVALIDDATA; } - for (i = 0; i < c->fragment_index_count; i++) { - MOVFragmentIndex* candidate = c->fragment_index_data[i]; - if (candidate->track_id == frag->track_id) { - av_log(c->fc, AV_LOG_DEBUG, - "found fragment index for track %u\n", frag->track_id); - index = candidate; - break; - } - } frag->base_data_offset = flags & MOV_TFHD_BASE_DATA_OFFSET ? avio_rb64(pb) : flags & MOV_TFHD_DEFAULT_BASE_IS_MOOF ? @@ -3389,24 +3380,33 @@ static int mov_read_tfhd(MOVContext *c, AVIOContext *pb, MOVAtom atom) frag->flags = flags & MOV_TFHD_DEFAULT_FLAGS ? avio_rb32(pb) : trex->flags; frag->time = AV_NOPTS_VALUE; - if (index) { - int i, found = 0; - for (i = index->current_item; i < index->item_count; i++) { - if (frag->implicit_offset == index->items[i].moof_offset) { - av_log(c->fc, AV_LOG_DEBUG, "found fragment index entry " - "for track %u and moof_offset %"PRId64"\n", - frag->track_id, index->items[i].moof_offset); - frag->time = index->items[i].time; - index->current_item = i + 1; - found = 1; + for (i = 0; i < c->fragment_index_count; i++) { + int j; + MOVFragmentIndex* candidate = c->fragment_index_data[i]; + if (candidate->track_id == frag->track_id) { + av_log(c->fc, AV_LOG_DEBUG, + "found fragment index for track %u\n", frag->track_id); + index = candidate; + for (j = index->current_item; j < index->item_count; j++) { + if (frag->implicit_offset == index->items[j].moof_offset) { + av_log(c->fc, AV_LOG_DEBUG, "found fragment index entry " + "for track %u and moof_offset %"PRId64"\n", + frag->track_id, index->items[j].moof_offset); + frag->time = index->items[j].time; + index->current_item = j + 1; + found = 1; + break; + } } - } - if (!found) { - av_log(c->fc, AV_LOG_WARNING, "track %u has a fragment index " - "but it doesn't have an (in-order) entry for moof_offset " - "%"PRId64"\n", frag->track_id, frag->implicit_offset); + if (found) + break; } } + if (index && !found) { + av_log(c->fc, AV_LOG_DEBUG, "track %u has a fragment index but " + "it doesn't have an (in-order) entry for moof_offset " + "%"PRId64"\n", frag->track_id, frag->implicit_offset); + } av_log(c->fc, AV_LOG_TRACE, "frag flags 0x%x\n", frag->flags); return 0; } @@ -3596,7 +3596,106 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) return AVERROR_EOF; frag->implicit_offset = offset; - st->duration = sc->track_end = dts + sc->time_offset; + + sc->track_end = dts + sc->time_offset; + if (st->duration < sc->track_end) + st->duration = sc->track_end; + + return 0; +} + +static int mov_read_sidx(MOVContext *c, AVIOContext *pb, MOVAtom atom) +{ + int64_t offset = avio_tell(pb) + atom.size, pts; + uint8_t version; + unsigned i, track_id; + AVStream *st = NULL; + MOVStreamContext *sc; + MOVFragmentIndex *index = NULL; + MOVFragmentIndex **tmp; + AVRational timescale; + + version = avio_r8(pb); + if (version > 1) { + avpriv_request_sample(c->fc, "sidx version %u", version); + return AVERROR_PATCHWELCOME; + } + + avio_rb24(pb); // flags + + track_id = avio_rb32(pb); // Reference ID + for (i = 0; i < c->fc->nb_streams; i++) { + if (c->fc->streams[i]->id == track_id) { + st = c->fc->streams[i]; + break; + } + } + if (!st) { + av_log(c->fc, AV_LOG_ERROR, "could not find corresponding track id %d\n", track_id); + return AVERROR_INVALIDDATA; + } + + sc = st->priv_data; + + timescale = av_make_q(1, avio_rb32(pb)); + + if (version == 0) { + pts = avio_rb32(pb); + offset += avio_rb32(pb); + } else { + pts = avio_rb64(pb); + offset += avio_rb64(pb); + } + + avio_rb16(pb); // reserved + + index = av_mallocz(sizeof(MOVFragmentIndex)); + if (!index) + return AVERROR(ENOMEM); + + index->track_id = track_id; + + index->item_count = avio_rb16(pb); + index->items = av_mallocz_array(index->item_count, sizeof(MOVFragmentIndexItem)); + + if (!index->items) { + av_freep(&index); + return AVERROR(ENOMEM); + } + + for (i = 0; i < index->item_count; i++) { + uint32_t size = avio_rb32(pb); + uint32_t duration = avio_rb32(pb); + if (size & 0x80000000) { + avpriv_request_sample(c->fc, "sidx reference_type 1"); + av_freep(&index->items); + av_freep(&index); + return AVERROR_PATCHWELCOME; + } + avio_rb32(pb); // sap_flags + index->items[i].moof_offset = offset; + index->items[i].time = av_rescale_q(pts, st->time_base, timescale); + offset += size; + pts += duration; + } + + st->duration = sc->track_end = pts; + + tmp = av_realloc_array(c->fragment_index_data, + c->fragment_index_count + 1, + sizeof(MOVFragmentIndex*)); + if (!tmp) { + av_freep(&index->items); + av_freep(&index); + return AVERROR(ENOMEM); + } + + c->fragment_index_data = tmp; + c->fragment_index_data[c->fragment_index_count++] = index; + + if (offset == avio_size(pb)) + c->fragment_index_complete = 1; + return 0; } @@ -3854,6 +3953,7 @@ static const MOVParseTableEntry mov_default_parse_table[] = { { MKTAG('a','l','a','c'), mov_read_alac }, /* alac specific atom */ { MKTAG('a','v','c','C'), mov_read_glbl }, { MKTAG('p','a','s','p'), mov_read_pasp }, +{ MKTAG('s','i','d','x'), mov_read_sidx }, { MKTAG('s','t','b','l'), mov_read_default }, { MKTAG('s','t','c','o'), mov_read_stco }, { MKTAG('s','t','p','s'), mov_read_stps }, @@ -3978,9 +4078,9 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom) return err; } if (c->found_moov && c->found_mdat && - ((!pb->seekable || c->fc->flags & AVFMT_FLAG_IGNIDX) || + ((!pb->seekable || c->fc->flags & AVFMT_FLAG_IGNIDX || c->fragment_index_complete) || start_pos + a.size == avio_size(pb))) { - if (!pb->seekable || c->fc->flags & AVFMT_FLAG_IGNIDX) + if (!pb->seekable || c->fc->flags & AVFMT_FLAG_IGNIDX || c->fragment_index_complete) c->next_root_atom = start_pos + a.size; c->atom_depth --; return 0; @@ -4585,6 +4685,52 @@ static int should_retry(AVIOContext *pb, int error_code) { return 1; } +static int mov_switch_root(AVFormatContext *s, int64_t target) +{ + MOVContext *mov = s->priv_data; + int i, j; + int already_read = 0; + + if (avio_seek(s->pb, target, SEEK_SET) != target) { + av_log(mov->fc, AV_LOG_ERROR, "root atom offset 0x%"PRIx64": partial file\n", target); + return AVERROR_INVALIDDATA; + } + + mov->next_root_atom = 0; + + for (i = 0; i < mov->fragment_index_count; i++) { + MOVFragmentIndex *index = mov->fragment_index_data[i]; + int found = 0; + for (j = 0; j < index->item_count; j++) { + MOVFragmentIndexItem *item = &index->items[j]; + if (found) { + mov->next_root_atom = item->moof_offset; + break; // Advance to next index in outer loop + } else if (item->moof_offset == target) { + index->current_item = FFMIN(j, index->current_item); + if (item->headers_read) + already_read = 1; + item->headers_read = 1; + found = 1; + } + } + if (!found) + index->current_item = 0; + } + + if (already_read) + return 0; + + mov->found_mdat = 0; + + if (mov_read_default(mov, s->pb, (MOVAtom){ AV_RL32("root"), INT64_MAX }) < 0 || + avio_feof(s->pb)) + return AVERROR_EOF; + av_log(s, AV_LOG_TRACE, "read fragments, offset 0x%"PRIx64"\n", avio_tell(s->pb)); + + return 1; +} + static int mov_read_packet(AVFormatContext *s, AVPacket *pkt) { MOVContext *mov = s->priv_data; @@ -4595,19 +4741,11 @@ static int mov_read_packet(AVFormatContext *s, AVPacket *pkt) mov->fc = s; retry: sample = mov_find_next_sample(s, &st); - if (!sample) { - mov->found_mdat = 0; + if (!sample || (mov->next_root_atom && sample->pos > mov->next_root_atom)) { if (!mov->next_root_atom) return AVERROR_EOF; - if (avio_seek(s->pb, mov->next_root_atom, SEEK_SET) != mov->next_root_atom) { - av_log(mov->fc, AV_LOG_ERROR, "next root atom offset 0x%"PRIx64": partial file\n", mov->next_root_atom); - return AVERROR_INVALIDDATA; - } - mov->next_root_atom = 0; - if (mov_read_default(mov, s->pb, (MOVAtom){ AV_RL32("root"), INT64_MAX }) < 0 || - avio_feof(s->pb)) - return AVERROR_EOF; - av_log(s, AV_LOG_TRACE, "read fragments, offset 0x%"PRIx64"\n", avio_tell(s->pb)); + if ((ret = mov_switch_root(s, mov->next_root_atom)) < 0) + return ret; goto retry; } sc = st->priv_data; @@ -4685,12 +4823,41 @@ static int mov_read_packet(AVFormatContext *s, AVPacket *pkt) return 0; } +static int mov_seek_fragment(AVFormatContext *s, AVStream *st, int64_t timestamp) +{ + MOVContext *mov = s->priv_data; + int i, j; + + if (!mov->fragment_index_complete) + return 0; + + for (i = 0; i < mov->fragment_index_count; i++) { + if (mov->fragment_index_data[i]->track_id == st->id) { + MOVFragmentIndex *index = index = mov->fragment_index_data[i]; + for (j = index->item_count - 1; j >= 0; j--) { + if (index->items[j].time <= timestamp) { + if (index->items[j].headers_read) + return 0; + + return mov_switch_root(s, index->items[j].moof_offset); + } + } + } + } + + return 0; +} + static int mov_seek_stream(AVFormatContext *s, AVStream *st, int64_t timestamp, int flags) { MOVStreamContext *sc = st->priv_data; int sample, time_sample; int i; + int ret = mov_seek_fragment(s, st, timestamp); + if (ret < 0) + return ret; + sample = av_index_search_timestamp(st, timestamp, flags); av_log(s, AV_LOG_TRACE, "stream %d, timestamp %"PRId64", sample %d\n", st->index, timestamp, sample); if (sample < 0 && st->nb_index_entries && timestamp < st->index_entries[0].timestamp) -- 2.6.0
On Tue, Oct 06, 2015 at 03:50:23AM -0500, Rodger Combs wrote:
Fixes trac #3842 --- libavformat/isom.h | 2 + libavformat/mov.c | 245 ++++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 208 insertions(+), 39 deletions(-)
i think this should be applied unless someone has more comments [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you think the mosad wants you dead since a long time then you are either wrong or dead since a long time.
This patch seems to be causing some issues with some h264 video-only content. Attempting to play the following in ffplay with "-v debug -drp 1" (disables pts/dts fixups and purely uses pts) will cause jerky playback after seeking around a few times due to dropped out of order frames. These are shown in the log as "invalid dts/pts combination..." Skipping sidx parsing for this file yields smooth playback. I haven't figured out yet if this is bad encoding or something is wrong with the patch, but FYI. http://storage.googleapis.com/dalecurtis-shared/buck480p30_na.mp4 - dale On Tue, Oct 6, 2015 at 9:18 AM, Michael Niedermayer <michael@niedermayer.cc> wrote:
On Tue, Oct 06, 2015 at 03:50:23AM -0500, Rodger Combs wrote:
Fixes trac #3842 --- libavformat/isom.h | 2 + libavformat/mov.c | 245 ++++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 208 insertions(+), 39 deletions(-)
i think this should be applied unless someone has more comments
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If you think the mosad wants you dead since a long time then you are either wrong or dead since a long time.
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
I took a look at the structure of this file, and the encoding appears to be correct. The frame order in the moof boxes matches the H.264 stream, and the durations of the moof boxes match with the sidx. Some notes to help with analysis: There are no per-frame durations specified, every frame is exactly 3000 time units long (=1s/30). The first frame PTS is 3000, but there is an edit list to shift that to movie time 0. The fragments contain 152 frames each, and the base times of the fragments reflect that correctly. On Fri, Dec 11, 2015 at 2:39 PM Dale Curtis <dalecurtis@chromium.org> wrote:
This patch seems to be causing some issues with some h264 video-only content. Attempting to play the following in ffplay with "-v debug -drp 1" (disables pts/dts fixups and purely uses pts) will cause jerky playback after seeking around a few times due to dropped out of order frames. These are shown in the log as "invalid dts/pts combination..." Skipping sidx parsing for this file yields smooth playback. I haven't figured out yet if this is bad encoding or something is wrong with the patch, but FYI.
http://storage.googleapis.com/dalecurtis-shared/buck480p30_na.mp4
- dale
On Tue, Oct 6, 2015 at 9:18 AM, Michael Niedermayer < michael@niedermayer.cc> wrote:
On Tue, Oct 06, 2015 at 03:50:23AM -0500, Rodger Combs wrote:
Fixes trac #3842 --- libavformat/isom.h | 2 + libavformat/mov.c | 245 ++++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 208 insertions(+), 39 deletions(-)
i think this should be applied unless someone has more comments
[...]
-- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If you think the mosad wants you dead since a long time then you are either wrong or dead since a long time.
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Michael, I wanted to check if you have you looked into this playback issue, or were planning to? - Dan
On Fri, Jan 15, 2016 at 10:24:43PM +0000, Dan Sanders wrote:
Michael, I wanted to check if you have you looked into this playback issue, or were planning to?
i didnt look into it, i had thought rodger would look into it as it was his patch ... rodger, did you look into this ? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better.
This issue is fixed by this patch, but I'm unsure of possible implications on other files. It passes FATE, at least. diff --git a/libavformat/mov.c b/libavformat/mov.c index 149e3b4..c5e0a1e 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -3609,7 +3609,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) } av_log(c->fc, AV_LOG_DEBUG, "calculated into dts %"PRId64"\n", dts); } else { - dts = frag->time; + dts = frag->time - sc->time_offset; av_log(c->fc, AV_LOG_DEBUG, "found frag time %"PRId64 ", using it for dts\n", dts); }
On Jan 15, 2016, at 16:57, Michael Niedermayer <michael@niedermayer.cc> wrote:
On Fri, Jan 15, 2016 at 10:24:43PM +0000, Dan Sanders wrote:
Michael, I wanted to check if you have you looked into this playback issue, or were planning to?
i didnt look into it, i had thought rodger would look into it as it was his patch ...
rodger, did you look into this ?
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better.
Hmm, finally got around to looking into this again and this still isn't fixed. Just seeking a few times in ffplay can trigger this issue with the clip linked in my original message: http://storage.googleapis.com/dalecurtis-shared/buck480p30_na.mp4 ./ffplay -v debug -drp 1 ~/Downloads/buck480p30_na.mp4 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14583000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14586000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found fragment index for track 1 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found fragment index entry for track 1 and moof_offset 16686198 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found frag time 14589000, using it for dts [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14607000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14610000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14622000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14631000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14634000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14643000 Disabled sidx processing resolves this issue: diff --git a/libavformat/mov.c b/libavformat/mov.c index 63f84be782..919475f12f 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -5497,7 +5497,7 @@ static const MOVParseTableEntry mov_default_parse_table[] = { { MKTAG('a','l','a','c'), mov_read_alac }, /* alac specific atom */ { MKTAG('a','v','c','C'), mov_read_glbl }, { MKTAG('p','a','s','p'), mov_read_pasp }, -{ MKTAG('s','i','d','x'), mov_read_sidx }, +// { MKTAG('s','i','d','x'), mov_read_sidx }, Rodger, are you able to still look into this? - dale On Fri, Feb 12, 2016 at 2:21 AM, Rodger Combs <rodger.combs@gmail.com> wrote:
This issue is fixed by this patch, but I'm unsure of possible implications on other files. It passes FATE, at least.
diff --git a/libavformat/mov.c b/libavformat/mov.c index 149e3b4..c5e0a1e 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -3609,7 +3609,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) } av_log(c->fc, AV_LOG_DEBUG, "calculated into dts %"PRId64"\n", dts); } else { - dts = frag->time; + dts = frag->time - sc->time_offset; av_log(c->fc, AV_LOG_DEBUG, "found frag time %"PRId64 ", using it for dts\n", dts); }
On Jan 15, 2016, at 16:57, Michael Niedermayer <michael@niedermayer.cc> wrote:
On Fri, Jan 15, 2016 at 10:24:43PM +0000, Dan Sanders wrote:
Michael, I wanted to check if you have you looked into this playback issue, or were planning to?
i didnt look into it, i had thought rodger would look into it as it was his patch ...
rodger, did you look into this ?
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better.
After looking at this some more. I don't think the way ctts entries are stored is correct unless you assume all ctts entries are known prior to any seeks taking place. Either through reading all trun boxes ahead of time or content having a ctts atom. The current code is broken in the presence of seeks since seeks can not know the proper ctts index unless all trun boxes have been read. Instead it performs the seek+root switch and then ends up writing ctts entries into the wrong slots of the global ctts index as trun boxes are encountered after the seek. To fix this I think the code needs to only use the global ctts index if the ctts atom is present. If the ctts data is just being pulled from the trun boxes, then it should probably be stored in along with each MOVFragmentIndex. Is anyone else familiar enough with this code to have opinions on direction here? Rodger seems MIA. - dale On Fri, Jun 30, 2017 at 2:56 PM, Dale Curtis <dalecurtis@chromium.org> wrote:
Hmm, finally got around to looking into this again and this still isn't fixed. Just seeking a few times in ffplay can trigger this issue with the clip linked in my original message:
http://storage.googleapis.com/dalecurtis-shared/buck480p30_na.mp4
./ffplay -v debug -drp 1 ~/Downloads/buck480p30_na.mp4 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14583000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14586000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found fragment index for track 1 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found fragment index entry for track 1 and moof_offset 16686198 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found frag time 14589000, using it for dts [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14607000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14610000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14622000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14631000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14634000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14643000
Disabled sidx processing resolves this issue:
diff --git a/libavformat/mov.c b/libavformat/mov.c index 63f84be782..919475f12f 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -5497,7 +5497,7 @@ static const MOVParseTableEntry mov_default_parse_table[] = { { MKTAG('a','l','a','c'), mov_read_alac }, /* alac specific atom */ { MKTAG('a','v','c','C'), mov_read_glbl }, { MKTAG('p','a','s','p'), mov_read_pasp }, -{ MKTAG('s','i','d','x'), mov_read_sidx }, +// { MKTAG('s','i','d','x'), mov_read_sidx },
Rodger, are you able to still look into this?
- dale
On Fri, Feb 12, 2016 at 2:21 AM, Rodger Combs <rodger.combs@gmail.com> wrote:
This issue is fixed by this patch, but I'm unsure of possible implications on other files. It passes FATE, at least.
diff --git a/libavformat/mov.c b/libavformat/mov.c index 149e3b4..c5e0a1e 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -3609,7 +3609,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) } av_log(c->fc, AV_LOG_DEBUG, "calculated into dts %"PRId64"\n", dts); } else { - dts = frag->time; + dts = frag->time - sc->time_offset; av_log(c->fc, AV_LOG_DEBUG, "found frag time %"PRId64 ", using it for dts\n", dts); }
On Jan 15, 2016, at 16:57, Michael Niedermayer <michael@niedermayer.cc> wrote:
On Fri, Jan 15, 2016 at 10:24:43PM +0000, Dan Sanders wrote:
Michael, I wanted to check if you have you looked into this playback issue, or were planning to?
i didnt look into it, i had thought rodger would look into it as it was his patch ...
rodger, did you look into this ?
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better.
Hi On Fri, Jul 14, 2017 at 02:52:23PM -0700, Dale Curtis wrote:
After looking at this some more. I don't think the way ctts entries are stored is correct unless you assume all ctts entries are known prior to any seeks taking place. Either through reading all trun boxes ahead of time or content having a ctts atom.
The current code is broken in the presence of seeks since seeks can not know the proper ctts index unless all trun boxes have been read. Instead it performs the seek+root switch and then ends up writing ctts entries into the wrong slots of the global ctts index as trun boxes are encountered after the seek.
To fix this I think the code needs to only use the global ctts index if the ctts atom is present. If the ctts data is just being pulled from the trun boxes, then it should probably be stored in along with each MOVFragmentIndex.
Is anyone else familiar enough with this code to have opinions on direction here? Rodger seems MIA.
I dont think CTTS is the only affected atom. what you describe sounds like an option. but i might be missing something, possibly even something significant. I dont feel that confident with this code after quickly looking at it.
- dale
On Fri, Jun 30, 2017 at 2:56 PM, Dale Curtis <dalecurtis@chromium.org> wrote:
Hmm, finally got around to looking into this again and this still isn't fixed. Just seeking a few times in ffplay can trigger this issue with the clip linked in my original message:
http://storage.googleapis.com/dalecurtis-shared/buck480p30_na.mp4
./ffplay -v debug -drp 1 ~/Downloads/buck480p30_na.mp4 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14583000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14586000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found fragment index for track 1 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found fragment index entry for track 1 and moof_offset 16686198 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found frag time 14589000, using it for dts [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14607000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14610000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14622000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14631000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14634000 [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination 14643000
Disabled sidx processing resolves this issue:
diff --git a/libavformat/mov.c b/libavformat/mov.c index 63f84be782..919475f12f 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -5497,7 +5497,7 @@ static const MOVParseTableEntry mov_default_parse_table[] = { { MKTAG('a','l','a','c'), mov_read_alac }, /* alac specific atom */ { MKTAG('a','v','c','C'), mov_read_glbl }, { MKTAG('p','a','s','p'), mov_read_pasp }, -{ MKTAG('s','i','d','x'), mov_read_sidx }, +// { MKTAG('s','i','d','x'), mov_read_sidx },
Rodger, are you able to still look into this?
- dale
On Fri, Feb 12, 2016 at 2:21 AM, Rodger Combs <rodger.combs@gmail.com> wrote:
This issue is fixed by this patch, but I'm unsure of possible implications on other files. It passes FATE, at least.
diff --git a/libavformat/mov.c b/libavformat/mov.c index 149e3b4..c5e0a1e 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -3609,7 +3609,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) } av_log(c->fc, AV_LOG_DEBUG, "calculated into dts %"PRId64"\n", dts); } else { - dts = frag->time; + dts = frag->time - sc->time_offset; av_log(c->fc, AV_LOG_DEBUG, "found frag time %"PRId64 ", using it for dts\n", dts); }
On Jan 15, 2016, at 16:57, Michael Niedermayer <michael@niedermayer.cc> wrote:
On Fri, Jan 15, 2016 at 10:24:43PM +0000, Dan Sanders wrote:
Michael, I wanted to check if you have you looked into this playback issue, or were planning to?
i didnt look into it, i had thought rodger would look into it as it was his patch ...
rodger, did you look into this ?
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better.
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
-- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
On Fri, Jul 14, 2017 at 5:38 PM, Michael Niedermayer <michael@niedermayer.cc
wrote:
I dont think CTTS is the only affected atom.
Thanks for the response. Yes, I think that's likely true. The ctts one is just likely the most important since it controls timestamp ordering.
what you describe sounds like an option. but i might be missing something, possibly even something significant. I dont feel that confident with this code after quickly looking at it.
Here are my experiments thus far if you have further thoughts: https://chromium-review.googlesource.com/c/572730/ https://chromium-review.googlesource.com/c/572585/ Both fixes and the existing code seem to suffer from assuming that the sample number in the AVIndex and that of the ctts_data are the same; which seems questionable. The second experiment should be viable with the addition of memmove() when inserting in the middle of the array. Will look at it more next week. - dale
Okay the following patch should fix this issue. Chromium/reviewable version here https://chromium-review.googlesource.com/c/572585/ - dale On Fri, Jul 14, 2017 at 6:31 PM, Dale Curtis <dalecurtis@chromium.org> wrote:
On Fri, Jul 14, 2017 at 5:38 PM, Michael Niedermayer < michael@niedermayer.cc> wrote:
I dont think CTTS is the only affected atom.
Thanks for the response. Yes, I think that's likely true. The ctts one is just likely the most important since it controls timestamp ordering.
what you describe sounds like an option. but i might be missing something, possibly even something significant. I dont feel that confident with this code after quickly looking at it.
Here are my experiments thus far if you have further thoughts: https://chromium-review.googlesource.com/c/572730/ https://chromium-review.googlesource.com/c/572585/
Both fixes and the existing code seem to suffer from assuming that the sample number in the AVIndex and that of the ctts_data are the same; which seems questionable. The second experiment should be viable with the addition of memmove() when inserting in the middle of the array. Will look at it more next week.
- dale
participants (4)
-
Dale Curtis -
Dan Sanders -
Michael Niedermayer -
Rodger Combs