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_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 <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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
