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

[Baptiste Coudurier]

Hi Reimar,

Reimar D?ffinger wrote:
> 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.

Yes that's true.

> [...]
>
>> 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?)...

Quite some time now, that's true, but I have something pending for some
time now, didn't catch up yet. The warning is not there since a long
time though.

> 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).

I think Edit lists have to be handled by the playing/transcoding
application, the demuxer should demux all samples with their correct
timestamps.
Also you could stream copy and keep the editlist, if later you want to
re-edit with all the originally available samples.

>>> 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...

Humm that was not an argument to treat cases differently, this was
refering to the fact that edit lists refers to pts.

> [...]
>
>>> 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.

This can be assumed, 'ctts' atom is supposed to be contained in 'trak' atom.

> 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.

Who knows what crappy muxers can do...

>> 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.

Right.

> 
> 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;

A comment explaining what it is used for would be nice.

>      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;
> +    }
> +    

I think sc->ctts_data[0] can be used here.

>      /* 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;

I just commited once change so sc->ffindex can be used to avoid this.

Except these, Im ok with the patch.

[...]

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no