I found some samples with ctts and trun boxes, so I've updated the patch to handle these cases since I don't know how common they are in the wild and it's an easy fix. I'll send a followup patch if this one is accepted to remove support for > 1 count ctts entries.
- dale On Wed, Jul 19, 2017 at 7:30 PM, Dale Curtis <dalecur...@chromium.org> wrote: > Thanks will take a look. Is this test not part of fate? make fate passed > for me. The attached patch fixes this; the issue was that the index entries > are 1 to 1 with ctts values. When samples are added without ctts entries > we'd just initialize a single ctts entry with a count of 5. This left a gap > in the ctts table; the fix is to use only 1-count entries when this case is > hit. > > Note: This made me realize the presence of a ctts box and a trun box with > ctts samples has always been broken. If the ctts box comes second it'll > wipe the trun's generated table, but if the trun box comes after the ctts > box it will try to insert samples at incorrect positions. Prior to my patch > they would be looked up at incorrect positions, so there shouldn't be any > new bad behavior here. > > - dale > > On Wed, Jul 19, 2017 at 3:28 PM, Michael Niedermayer < > mich...@niedermayer.cc> wrote: > >> On Tue, Jul 18, 2017 at 11:49:26AM -0700, Dale Curtis wrote: >> > Updated patch that fixes other ctts modification code to use the new >> > ctts_allocated_size value; I've verified this passes fate. >> > >> > - dale >> > >> > On Tue, Jul 18, 2017 at 9:53 AM, Dale Curtis <dalecur...@chromium.org> >> > wrote: >> > >> > > Resending as it's own message per contribution rules. I've also >> attached >> > > the patch in case the formatting gets trashed. >> > > >> > > When sidx box support is enabled, the code will skip reading all >> > > trun boxes (each containing ctts entries for samples inthat box). >> > > >> > > If seeks are attempted before all ctts values are known, the old >> > > code would dump ctts entries into the wrong location. These are >> > > then used to compute pts values which leads to out of order and >> > > incorrectly timestamped packets. >> > > >> > > This patch fixes ctts processing by always using the index returned >> > > by av_add_index_entry() as the ctts_data index. When the index gains >> > > new entries old values are reshuffled as appropriate. >> > > >> > > This approach makes sense since the mov demuxer is already relying >> > > on the mapping of AVIndex entries to samples for correct demuxing. >> > > >> > > Notes for future improvement: >> > > Probably there are other boxes (stts, stsc, etc) that are impacted >> > > by this issue... this patch only attempts to fix ctts since it >> > > completely breaks packet timestamping. >> > > >> > > This patch continues using an array for the ctts data, which is not >> > > the most ideal given the rearrangement that needs to happen (via >> > > memmove as new entries are read in). Ideally AVIndex and the ctts >> > > data would be set-type structures so addition is always worst case >> > > O(lg(n)) instead of the O(n^2) that exists now; this slowdown is >> > > noticeable during seeks. >> > > >> > > Additionally since ctts samples from trun boxes always have a count >> > > of 1, there's probably an opportunity to avoid the post-seek fixup >> > > that iterates O(n) over all samples with an O(1) when no non-1 count >> > > samples are present. >> > > >> > > Signed-off-by: Dale Curtis <dalecur...@chromium.org> >> > > --- >> > > libavformat/isom.h | 1 + >> > > libavformat/mov.c | 46 ++++++++++++++++++++++++++++++ >> +--------------- >> > > 2 files changed, 32 insertions(+), 15 deletions(-) >> > > >> > > diff --git a/libavformat/isom.h b/libavformat/isom.h >> > > index ff009b0896..fdd98c28f5 100644 >> > > --- a/libavformat/isom.h >> > > +++ b/libavformat/isom.h >> > > @@ -137,6 +137,7 @@ typedef struct MOVStreamContext { >> > > unsigned int stts_count; >> > > MOVStts *stts_data; >> > > unsigned int ctts_count; >> > > + unsigned int ctts_allocated_size; >> > > MOVStts *ctts_data; >> > > unsigned int stsc_count; >> > > MOVStsc *stsc_data; >> > > diff --git a/libavformat/mov.c b/libavformat/mov.c >> > > index 63f84be782..412290b435 100644 >> > > --- a/libavformat/mov.c >> > > +++ b/libavformat/mov.c >> > > @@ -4297,11 +4297,6 @@ static int mov_read_trun(MOVContext *c, >> AVIOContext >> > > *pb, MOVAtom atom) >> > > } >> > > if ((uint64_t)entries+sc->ctts_count >= >> > > UINT_MAX/sizeof(*sc->ctts_data)) >> > > return AVERROR_INVALIDDATA; >> > > - if ((err = av_reallocp_array(&sc->ctts_data, entries + >> > > sc->ctts_count, >> > > - sizeof(*sc->ctts_data))) < 0) { >> > > - sc->ctts_count = 0; >> > > - return err; >> > > - } >> > > if (flags & MOV_TRUN_DATA_OFFSET) data_offset = >> > > avio_rb32(pb); >> > > if (flags & MOV_TRUN_FIRST_SAMPLE_FLAGS) first_sample_flags = >> > > avio_rb32(pb); >> > > dts = sc->track_end - sc->time_offset; >> > > @@ -4312,26 +4307,28 @@ static int mov_read_trun(MOVContext *c, >> > > AVIOContext *pb, MOVAtom atom) >> > > unsigned sample_size = frag->size; >> > > int sample_flags = i ? frag->flags : first_sample_flags; >> > > unsigned sample_duration = frag->duration; >> > > + unsigned ctts_duration = 0; >> > > int keyframe = 0; >> > > + int ctts_index = 0; >> > > + int old_nb_index_entries = st->nb_index_entries; >> > > >> > > if (flags & MOV_TRUN_SAMPLE_DURATION) sample_duration = >> > > avio_rb32(pb); >> > > if (flags & MOV_TRUN_SAMPLE_SIZE) sample_size = >> > > avio_rb32(pb); >> > > if (flags & MOV_TRUN_SAMPLE_FLAGS) sample_flags = >> > > avio_rb32(pb); >> > > - sc->ctts_data[sc->ctts_count].count = 1; >> > > - sc->ctts_data[sc->ctts_count].duration = (flags & >> > > MOV_TRUN_SAMPLE_CTS) ? >> > > - avio_rb32(pb) : 0; >> > > - mov_update_dts_shift(sc, sc->ctts_data[sc->ctts_count]. >> duration); >> > > + if (flags & MOV_TRUN_SAMPLE_CTS) ctts_duration = >> > > avio_rb32(pb); >> > > + >> > > + mov_update_dts_shift(sc, ctts_duration); >> > > if (frag->time != AV_NOPTS_VALUE) { >> > > if (c->use_mfra_for == FF_MOV_FLAG_MFRA_PTS) { >> > > int64_t pts = frag->time; >> > > av_log(c->fc, AV_LOG_DEBUG, "found frag time %"PRId64 >> > > " sc->dts_shift %d ctts.duration %d" >> > > " sc->time_offset %"PRId64" flags & >> > > MOV_TRUN_SAMPLE_CTS %d\n", pts, >> > > - sc->dts_shift, sc->ctts_data[sc->ctts_count]. >> > > duration, >> > > + sc->dts_shift, ctts_duration, >> > > sc->time_offset, flags & >> MOV_TRUN_SAMPLE_CTS); >> > > dts = pts - sc->dts_shift; >> > > if (flags & MOV_TRUN_SAMPLE_CTS) { >> > > - dts -= sc->ctts_data[sc->ctts_count].duration; >> > > + dts -= ctts_duration; >> > > } else { >> > > dts -= sc->time_offset; >> > > } >> > > @@ -4343,7 +4340,7 @@ static int mov_read_trun(MOVContext *c, >> AVIOContext >> > > *pb, MOVAtom atom) >> > > } >> > > frag->time = AV_NOPTS_VALUE; >> > > } >> > > - sc->ctts_count++; >> > > + >> > > if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) >> > > keyframe = 1; >> > > else >> > > @@ -4352,11 +4349,30 @@ static int mov_read_trun(MOVContext *c, >> > > AVIOContext *pb, MOVAtom atom) >> > > MOV_FRAG_SAMPLE_FLAG_DEPENDS_Y >> ES)); >> > > if (keyframe) >> > > distance = 0; >> > > - err = av_add_index_entry(st, offset, dts, sample_size, >> distance, >> > > - keyframe ? AVINDEX_KEYFRAME : 0); >> > > - if (err < 0) { >> > > + ctts_index = av_add_index_entry(st, offset, dts, sample_size, >> > > distance, >> > > + keyframe ? AVINDEX_KEYFRAME >> : 0); >> > > + if (ctts_index >= 0 && old_nb_index_entries < >> > > st->nb_index_entries) { >> > > + unsigned int size_needed = st->nb_index_entries * >> > > sizeof(*sc->ctts_data); >> > > + unsigned int request_size = size_needed > >> > > sc->ctts_allocated_size ? >> > > + FFMAX(size_needed, 2 * sc->ctts_allocated_size) : >> > > size_needed; >> > > + sc->ctts_data = av_fast_realloc(sc->ctts_data, >> > > &sc->ctts_allocated_size, request_size); >> > > + if (!sc->ctts_data) { >> > > + sc->ctts_count = 0; >> > > + return AVERROR(ENOMEM); >> > > + } >> > > + >> > > + if (ctts_index != old_nb_index_entries) { >> > > + memmove(sc->ctts_data + ctts_index + 1, >> sc->ctts_data + >> > > ctts_index, >> > > + sizeof(*sc->ctts_data) * (sc->ctts_count - >> > > ctts_index)); >> > > + } >> > > + >> > > + sc->ctts_data[ctts_index].count = 1; >> > > + sc->ctts_data[ctts_index].duration = ctts_duration; >> > > + sc->ctts_count++; >> > > + } else { >> > > av_log(c->fc, AV_LOG_ERROR, "Failed to add index >> entry\n"); >> > > } >> > > + >> > > av_log(c->fc, AV_LOG_TRACE, "AVIndex stream %d, sample %u, >> offset >> > > %"PRIx64", dts %"PRId64", " >> > > "size %u, distance %d, keyframe %d\n", st->index, >> > > sc->sample_count+i, >> > > offset, dts, sample_size, distance, keyframe); >> > > -- >> > > 2.13.2.932.g7449e964c-goog >> > > >> >> > isom.h | 1 + >> > mov.c | 58 ++++++++++++++++++++++++++++++ >> +++++++--------------------- >> > 2 files changed, 38 insertions(+), 21 deletions(-) >> > f1e0078808ae40c006b68acfd075fb1cfe62acf2 >> 0001-Fix-trampling-of-ctts-during-seeks-when-sidx-support.patch >> > From 0f94ed2f3cca57bd6dc164e750e92efbbf5b612f Mon Sep 17 00:00:00 2001 >> > From: Dale Curtis <dalecur...@chromium.org> >> > Date: Mon, 17 Jul 2017 17:38:09 -0700 >> > Subject: [PATCH] Fix trampling of ctts during seeks when sidx support is >> > enabled. >> > >> > When sidx box support is enabled, the code will skip reading all >> > trun boxes (each containing ctts entries for samples inthat box). >> > >> > If seeks are attempted before all ctts values are known, the old >> > code would dump ctts entries into the wrong location. These are >> > then used to compute pts values which leads to out of order and >> > incorrectly timestamped packets. >> > >> > This patch fixes ctts processing by always using the index returned >> > by av_add_index_entry() as the ctts_data index. When the index gains >> > new entries old values are reshuffled as appropriate. >> > >> > This approach makes sense since the mov demuxer is already relying >> > on the mapping of AVIndex entries to samples for correct demuxing. >> > >> > Notes for future improvement: >> > Probably there are other boxes (stts, stsc, etc) that are impacted >> > by this issue... this patch only attempts to fix ctts since it >> > completely breaks packet timestamping. >> > >> > This patch continues using an array for the ctts data, which is not >> > the most ideal given the rearrangement that needs to happen (via >> > memmove as new entries are read in). Ideally AVIndex and the ctts >> > data would be set-type structures so addition is always worst case >> > O(lg(n)) instead of the O(n^2) that exists now; this slowdown is >> > noticeable during seeks. >> > >> > Additionally since ctts samples from trun boxes always have a count >> > of 1, there's probably an opportunity to avoid the post-seek fixup >> > that iterates O(n) over all samples with an O(1) when no non-1 count >> > samples are present. >> > >> > Signed-off-by: Dale Curtis <dalecur...@chromium.org> >> > --- >> > libavformat/isom.h | 1 + >> > libavformat/mov.c | 58 ++++++++++++++++++++++++++++++ >> ++++-------------------- >> > 2 files changed, 38 insertions(+), 21 deletions(-) >> >> this seems to change timestamps for: >> ./ffmpeg -i matrixbench_mpeg2.mpg -t 1 -acodec aac -frag_duration 200k >> test.mov >> ./ffprobe -v 0 test.mov -show_packets -print_format compact >> >> http://samples.ffmpeg.org/benchmark/testsuite1/matrixbench_mpeg2.mpg >> >> The dts/pts pairs after the patch do not look good >> >> @@ -15,59 +15,59 @@ >> packet|codec_type=video|stream_index=0|pts=5632|pts_time=0. >> 440000|dts=3584|dts_time=0.280000|duration=512|duration_ >> time=0.040000|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=3995|pos=29570|flags=__ >> packet|codec_type=video|stream_index=0|pts=5120|pts_time=0. >> 400000|dts=4096|dts_time=0.320000|duration=512|duration_ >> time=0.040000|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=1526|pos=33565|flags=__ >> packet|codec_type=video|stream_index=0|pts=4608|pts_time=0. >> 360000|dts=4608|dts_time=0.360000|duration=512|duration_ >> time=0.040000|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=1309|pos=35091|flags=__ >> -packet|codec_type=audio|stream_index=1|pts=9984|pts_time=0. >> 208000|dts=9984|dts_time=0.208000|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=358|pos=36400|flags=K_ >> -packet|codec_type=audio|stream_index=1|pts=11008|pts_time= >> 0.229333|dts=11008|dts_time=0.229333|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=351|pos=36758|flags=K_ >> -packet|codec_type=audio|stream_index=1|pts=12032|pts_time= >> 0.250667|dts=12032|dts_time=0.250667|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=353|pos=37109|flags=K_ >> -packet|codec_type=audio|stream_index=1|pts=13056|pts_time= >> 0.272000|dts=13056|dts_time=0.272000|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=337|pos=37462|flags=K_ >> -packet|codec_type=audio|stream_index=1|pts=14080|pts_time= >> 0.293333|dts=14080|dts_time=0.293333|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=351|pos=37799|flags=K_ >> -packet|codec_type=audio|stream_index=1|pts=15104|pts_time= >> 0.314667|dts=15104|dts_time=0.314667|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=337|pos=38150|flags=K_ >> -packet|codec_type=audio|stream_index=1|pts=16128|pts_time= >> 0.336000|dts=16128|dts_time=0.336000|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=337|pos=38487|flags=K_ >> -packet|codec_type=audio|stream_index=1|pts=17152|pts_time= >> 0.357333|dts=17152|dts_time=0.357333|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=353|pos=38824|flags=K_ >> -packet|codec_type=audio|stream_index=1|pts=18176|pts_time= >> 0.378667|dts=18176|dts_time=0.378667|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=340|pos=39177|flags=K_ >> +packet|codec_type=audio|stream_index=1|pts=42509|pts_time= >> 0.885604|dts=9984|dts_time=0.208000|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=358|pos=36400|flags=K_ >> +packet|codec_type=audio|stream_index=1|pts=43533|pts_time= >> 0.906937|dts=11008|dts_time=0.229333|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=351|pos=36758|flags=K_ >> +packet|codec_type=audio|stream_index=1|pts=44557|pts_time= >> 0.928271|dts=12032|dts_time=0.250667|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=353|pos=37109|flags=K_ >> +packet|codec_type=audio|stream_index=1|pts=45581|pts_time= >> 0.949604|dts=13056|dts_time=0.272000|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=337|pos=37462|flags=K_ >> +packet|codec_type=audio|stream_index=1|pts=46605|pts_time= >> 0.970938|dts=14080|dts_time=0.293333|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=351|pos=37799|flags=K_ >> +packet|codec_type=audio|stream_index=1|pts=47629|pts_time= >> 0.992271|dts=15104|dts_time=0.314667|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=337|pos=38150|flags=K_ >> +packet|codec_type=audio|stream_index=1|pts=48653|pts_time= >> 1.013604|dts=16128|dts_time=0.336000|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=337|pos=38487|flags=K_ >> +packet|codec_type=audio|stream_index=1|pts=49677|pts_time= >> 1.034938|dts=17152|dts_time=0.357333|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=353|pos=38824|flags=K_ >> +packet|codec_type=audio|stream_index=1|pts=50701|pts_time= >> 1.056271|dts=18176|dts_time=0.378667|duration=1024|duration_ >> time=0.021333|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=340|pos=39177|flags=K_ >> packet|codec_type=video|stream_index=0|pts=7168|pts_time=0. >> 560000|dts=5120|dts_time=0.400000|duration=512|duration_ >> time=0.040000|convergence_duration=N/A|convergence_ >> duration_time=N/A|size=4204|pos=39797|flags=__ >> ... >> >> [...] >> >> -- >> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB >> >> I do not agree with what you have to say, but I'll defend to the death >> your >> right to say it. -- Voltaire >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> >
0001-Fix-trampling-of-ctts-during-seeks-when-sidx-support.patch
Description: Binary data
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel