[FFmpeg-devel] [HACK] mov edl desync fix

[Reimar Döffinger]

On Sun, Jan 11, 2009 at 04:11:17AM -0800, Baptiste Coudurier wrote:
> I always prefer isolating hacks the more it is possible and where they
> potentially do no harm. Still having only one chunk correct does not fix
> the problem while it clutters more the code.

Well, it fixes the problem for the first chunk, which may be enough for
some cases, my idea was to make it work for as many cases (including
at least partially correctly playing files) as possible without actually
implementing edl.
Most importantly, I don't understand the point "while it clutters more
the code", how does "if (i == 0)" clutter the code more that "if
(edit_count == 1)", going by character count it actually clutter the
code less.
Anyway, I don't mind that one really, it just would be easier to hack it
the right way if I understood your reasoning.

> Adding this hack is also one step away from getting full edit list
> support, so I won't accept an ugly mess, and there is no point wasting
> time on hacks that should go asap.

Well, that may be true, but I have some doubts that this hack will
_ever_ go (how long has this been unresolved?)...
I also think that full edit list support might not be possible from
within libavformat framework, since it requires seeking to
non-keyframes, thus it could only work if either the application codes
special support for it or uses libavcodec (and I doubt Michael would
accept a solution that is based on the later).

> > Do you have a specific case in mind where it would be less safe? 
> 
> Yes, if time is 1, first sample dts is 0 and pts is 1, you must not
> offset pts since timestamps are good.

First I thought you expect this to be fixed anyway, second that issue
exists whether you have one or more edit list chunks, so I don't
understand why that is an argument to treat the case where we have one
chunk and the case where we have more chunks differently...

> > Do you have any direct link to a PDF? I always end up with either HTML
> > or a PDF that one way or another is wrong.
> 
> Humm not really, I found it on the apple site. IIRC 15444-12 (iso media)
> is freely available on jpeg site.

Better than nothing. Low typesetting quality seems to be a must for
standards though, no index, no clickable page references, no version
with Corrigendums integrated.

> > Nevertheless, given that there are these two ways pts is set in mov.c:
> > pkt->pts = pkt->dts + sc->ctts_data[sc->sample_to_ctime_index].duration / sc->time_rate;
> > and
> > pkt->pts = pkt->dts;
> > Attached patch should fix that pts/dts discrepancy assuming the ctts
> > atom comes before the elst atom (probably a bad idea to assume that?).
> 
> I've never seen 'ctts' coming before 'elst' so this won't work

Not a surprise, but I don't want to fix it if it won't get accepted
anyway. With attached patch it seems to work for the sample, but it
still assumes that ctts will not be set after mov_build_index.
I am mostly confident that is the case for valid files, but I am
confused enough by the parsing code to not be sure what happens in the
actual code if someone puts a ctts atom outside a trak atom.

> and both
> pts and dts should be adjusted so b frames still have pts == dts.

Huh? I think my patch adjusts dts, and as above quoted lines show, pts
is calculated from dts, so both pts and dts get adjusted and the
relative difference between the certainly stay the same.

Greetings,
Reimar D?ffinger
-------------- next part --------------
Index: libavformat/mov.c
===================================================================
--- libavformat/mov.c	(revision 16530)
+++ libavformat/mov.c	(working copy)
@@ -124,6 +124,7 @@
     int *keyframes;
     int time_scale;
     int time_rate;
+    int time_offset;
     int current_sample;
     unsigned int bytes_per_frame;
     unsigned int samples_per_frame;
@@ -1225,6 +1226,11 @@
     unsigned int stss_index = 0;
     unsigned int i, j;
 
+    if (sc->time_offset != -1) {
+        int64_t pts_delay = sc->ctts_data ? sc->ctts_data[sc->sample_to_ctime_index].duration : 0;
+        current_dts = (pts_delay - sc->time_offset) / sc->time_rate;
+    }
+            
     /* only use old uncompressed audio chunk demuxing when stts specifies it */
     if (!(st->codec->codec_type == CODEC_TYPE_AUDIO &&
           sc->stts_count == 1 && sc->stts_data[0].duration == 1)) {
@@ -1735,21 +1743,25 @@
 /* edit list atom */
 static int mov_read_elst(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
 {
-    MOVStreamContext *sc = c->fc->streams[c->fc->nb_streams-1]->priv_data;
+    int streamnr = c->fc->nb_streams-1;
+    MOVStreamContext *sc = c->fc->streams[streamnr]->priv_data;
     int i, edit_count;
 
     get_byte(pb); /* version */
     get_be24(pb); /* flags */
     edit_count= sc->edit_count = get_be32(pb);     /* entries */
 
+    sc->time_offset = -1;
     for(i=0; i<edit_count; i++){
         int time;
         get_be32(pb); /* Track duration */
         time = get_be32(pb); /* Media time */
         get_be32(pb); /* Media rate */
+        if (time != -1 && edit_count == 1)
+            sc->time_offset = time;
         if (time != 0)
-            av_log(c->fc, AV_LOG_WARNING, "edit list not starting at 0, "
-                   "a/v desync might occur, patch welcome\n");
+            av_log(c->fc, AV_LOG_WARNING, "edit list %i for track %i not starting at 0 (but %i), "
+                   "a/v desync might occur, patch welcome\n", i, streamnr, time);
     }
     dprintf(c->fc, "track[%i].edit_count = %i\n", c->fc->nb_streams-1, sc->edit_count);
     return 0;