Message ID | CAPUDrwd3Ju+3y0mQUzPBu9G+QrDP-=mwJ2kDR+oV9B=Qy-G1Wg@mail.gmail.com |
---|---|
State | Superseded |
Headers | show |
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 <dalecurtis@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 <dalecurtis@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_YES)); > > 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 <dalecurtis@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 <dalecurtis@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=__ ... [...]
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 <michael@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 <dalecurtis@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 <dalecurtis@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_YES)); > > > 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 <dalecurtis@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 <dalecurtis@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 > >
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 <dalecurtis@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 < > michael@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 <dalecurtis@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 <dalecurtis@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 <dalecurtis@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 <dalecurtis@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 >> >> >
Hi On Wed, Jul 19, 2017 at 07:30:01PM -0700, Dale Curtis wrote: > Thanks will take a look. Is this test not part of fate? make fate passed no, we should have tests for all (fixed) tickets in fate ideally but in reality most tickets lack a corresponding test I tried both in outreachy and as well in GSoC to improve this situation with student projects but both only moved this forward by a small step. Its a large amount of work to create robust, portable and practical tests for "all" tickets and everything else. The way out to get this actually done would be to pay a developer to create tests for "all" tickets in fate. I belive carl would be the ideal one to do this work as he has since a very long time always tested and kept track of all our tickets. I did suggest a while ago to someone at google that funding such project would make sense but IIRC i never heared back. if some company would fund something like this, i belive this would be very usefull in the long run for code quality > 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 Thu, Jul 20, 2017 at 5:00 AM, Michael Niedermayer <michael@niedermayer.cc > wrote: > Hi > > On Wed, Jul 19, 2017 at 07:30:01PM -0700, Dale Curtis wrote: > > Thanks will take a look. Is this test not part of fate? make fate passed > > no, we should have tests for all (fixed) tickets in fate ideally > but in reality most tickets lack a corresponding test > I tried both in outreachy and as well in GSoC to improve this situation > with student projects but both only moved this forward by a small > step. Its a large amount of work to create robust, portable and > practical tests for "all" tickets and everything else. > The way out to get this actually done would be to pay a developer to > create tests for "all" tickets in fate. I belive carl would be the > ideal one to do this work as he has since a very long time always tested > and kept track of all our tickets. > I did suggest a while ago to someone at google that funding such > project would make sense but IIRC i never heared back. > if some company would fund something like this, i belive this would be > very usefull in the long run for code quality > I think it'd be pretty hard to get someone to go through and create tests for every issue ever seen. Even Chromium has trouble with this, but making it part of the culture helps. I.e. reviewers should ask for every change to include a test. I'm happy to add one to fate for this change. Or can do so in a followup patch if you prefer. Does anyone have comments on this change specifically? We've already rolled this into Chrome and it's working fine and passing all regression tests.
On Mon, Jul 24, 2017 at 02:32:41PM -0700, Dale Curtis wrote: > On Thu, Jul 20, 2017 at 5:00 AM, Michael Niedermayer <michael@niedermayer.cc > > wrote: > > > Hi > > > > On Wed, Jul 19, 2017 at 07:30:01PM -0700, Dale Curtis wrote: > > > Thanks will take a look. Is this test not part of fate? make fate passed > > > > no, we should have tests for all (fixed) tickets in fate ideally > > but in reality most tickets lack a corresponding test > > I tried both in outreachy and as well in GSoC to improve this situation > > with student projects but both only moved this forward by a small > > step. Its a large amount of work to create robust, portable and > > practical tests for "all" tickets and everything else. > > The way out to get this actually done would be to pay a developer to > > create tests for "all" tickets in fate. I belive carl would be the > > ideal one to do this work as he has since a very long time always tested > > and kept track of all our tickets. > > I did suggest a while ago to someone at google that funding such > > project would make sense but IIRC i never heared back. > > if some company would fund something like this, i belive this would be > > very usefull in the long run for code quality > > > > I think it'd be pretty hard to get someone to go through and create tests > for every issue ever seen. Even Chromium has trouble with this, but making yes, unless theres someone who enjoys doing this kind of work. > it part of the culture helps. I.e. reviewers should ask for every change to > include a test. yes though theres already 1.8 patch submission checklist ... 26. Consider adding a regression test for your code. on http://ffmpeg.org/developer.html > I'm happy to add one to fate for this change. Or can do so > in a followup patch if you prefer. please do (any variant is fine) thx > > Does anyone have comments on this change specifically? We've already rolled > this into Chrome and it's working fine and passing all regression tests. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
From 0f94ed2f3cca57bd6dc164e750e92efbbf5b612f Mon Sep 17 00:00:00 2001 From: Dale Curtis <dalecurtis@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 <dalecurtis@chromium.org> --- libavformat/isom.h | 1 + libavformat/mov.c | 58 ++++++++++++++++++++++++++++++++++-------------------- 2 files changed, 38 insertions(+), 21 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..ab8e914581 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -2726,7 +2726,7 @@ static int mov_read_ctts(MOVContext *c, AVIOContext *pb, MOVAtom atom) if (entries >= UINT_MAX / sizeof(*sc->ctts_data)) return AVERROR_INVALIDDATA; av_freep(&sc->ctts_data); - sc->ctts_data = av_realloc(NULL, entries * sizeof(*sc->ctts_data)); + sc->ctts_data = av_fast_realloc(NULL, &sc->ctts_allocated_size, entries * sizeof(*sc->ctts_data)); if (!sc->ctts_data) return AVERROR(ENOMEM); @@ -3046,7 +3046,6 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) int64_t index; int64_t index_ctts_count; int flags; - unsigned int ctts_allocated_size = 0; int64_t start_dts = 0; int64_t edit_list_media_time_dts = 0; int64_t edit_list_start_encountered = 0; @@ -3081,6 +3080,7 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) msc->ctts_count = 0; msc->ctts_index = 0; msc->ctts_sample = 0; + msc->ctts_allocated_size = 0; // If the dts_shift is positive (in case of negative ctts values in mov), // then negate the DTS by dts_shift @@ -3190,7 +3190,7 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) ctts_sample_old++; if (ctts_sample_old == ctts_data_old[ctts_index_old].count) { if (add_ctts_entry(&msc->ctts_data, &msc->ctts_count, - &ctts_allocated_size, + &msc->ctts_allocated_size, ctts_data_old[ctts_index_old].count - edit_list_start_ctts_sample, ctts_data_old[ctts_index_old].duration) == -1) { av_log(mov->fc, AV_LOG_ERROR, "Cannot add CTTS entry %"PRId64" - {%"PRId64", %d}\n", @@ -3289,7 +3289,7 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) if (ctts_data_old && ctts_sample_old != 0) { if (add_ctts_entry(&msc->ctts_data, &msc->ctts_count, - &ctts_allocated_size, + &msc->ctts_allocated_size, ctts_sample_old - edit_list_start_ctts_sample, ctts_data_old[ctts_index_old].duration) == -1) { av_log(mov->fc, AV_LOG_ERROR, "Cannot add CTTS entry %"PRId64" - {%"PRId64", %d}\n", @@ -4259,7 +4259,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) int64_t dts; int data_offset = 0; unsigned entries, first_sample_flags = frag->flags; - int flags, distance, i, err; + int flags, distance, i; for (i = 0; i < c->fc->nb_streams; i++) { if (c->fc->streams[i]->id == frag->track_id) { @@ -4287,7 +4287,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) if (!sc->ctts_count && sc->sample_count) { /* Complement ctts table if moov atom doesn't have ctts atom. */ - ctts_data = av_realloc(NULL, sizeof(*sc->ctts_data)); + ctts_data = av_fast_realloc(NULL, &sc->ctts_allocated_size, sizeof(*sc->ctts_data)); if (!ctts_data) return AVERROR(ENOMEM); sc->ctts_data = ctts_data; @@ -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_YES)); 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