diff mbox

[FFmpeg-devel] lavf/mov: add support for sidx fragment indexes

Message ID CAPUDrwcZ2FBV3sC0nsqzL3X99f8oPEcfOYpH8tdHWACP--Herg@mail.gmail.com
State New
Headers show

Commit Message

Dale Curtis July 18, 2017, 12:54 a.m. UTC 
Okay the following patch should fix this issue. Chromium/reviewable version
here https://chromium-review.googlesource.com/c/572585/

- dale

On Fri, Jul 14, 2017 at 6:31 PM, Dale Curtis <dalecurtis@chromium.org>
wrote:

> On Fri, Jul 14, 2017 at 5:38 PM, Michael Niedermayer <
> michael@niedermayer.cc> wrote:
>
>>
>> I dont think CTTS is the only affected atom.
>>
>>
> Thanks for the response. Yes, I think that's likely true. The ctts one is
> just likely the most important since it controls timestamp ordering.
>
>
>> what you describe sounds like an option. but i might be missing something,
>> possibly even something significant. I dont feel that confident with
>> this code after quickly looking at it.
>>
>
> Here are my experiments thus far if you have further thoughts:
> https://chromium-review.googlesource.com/c/572730/
> https://chromium-review.googlesource.com/c/572585/
>
> Both fixes and the existing code seem to suffer from assuming that the
> sample number in the AVIndex and that of the ctts_data are the same; which
> seems questionable. The second experiment should be viable with the
> addition of memmove() when inserting in the middle of the array. Will look
> at it more next week.
>
> - dale
>
>
>
diff mbox

Patch

From 4460d4b16472d1fd201818df6c18ba3e515941c4 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  | 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