[FFmpeg-devel] [mov] Fix trampling of ctts during seeks when sidx support is enabled.

Michael Niedermayer michael at niedermayer.cc
Thu Jul 20 01:28:37 EEST 2017


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 at 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 at 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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170720/7d905101/attachment.sig>


More information about the ffmpeg-devel mailing list