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

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
>

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

Commit Message

Comments

Patch