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

Dale Curtis dalecurtis at chromium.org
Thu Jul 20 06:23:18 EEST 2017


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 at 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 at 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 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_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 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
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-trampling-of-ctts-during-seeks-when-sidx-support.patch
Type: application/octet-stream
Size: 12175 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170719/7590e778/attachment.obj>


More information about the ffmpeg-devel mailing list