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

[Baptiste Coudurier]

Reimar D?ffinger wrote:
> On Sun, Jan 11, 2009 at 01:49:12AM -0800, Baptiste Coudurier wrote:
>> Reimar D?ffinger wrote:
>> [...]
>>
>>> Though it seems more correct to me to also do this for edls with more
>>> than one part (of course only the time of the first chunk is relevant),
>>> attached patch would do that.
>> I prefer not to do so. Edit list is not about hacking dts, it is about
>> selectionning specific part of the coded track to take into account.
>> Better applying the hack when it is safe to do so.
> 
> What do you consider "safe to do so"? To me it sounds like what you are
> say is "better it is completely broken than possibly broken".
> IMO my suggestion would change things to "the first edl chunk is always
> played synced correctly" whereas what you want would be "only files with
> only one edl chunk play right" which does not really seem safer to me.

It is correct in one case, you can offset pts if there is only one elst
atom and time > first pts. Also the timescale of 'time' is different
than the timescale of pts/dts.
With the hack, it will use more data than it should but it is still
correct in term of sync.

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.

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.

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

> I do agree that the warning should not be disabled, but that is true anyway,
> since only the timestamps are adjusted, nothing is actually skipped.

Well that is the problem, edit list are used to _skip_ things not adjust
timestamps. Edit list even refers to decoded samples in case of audio.

>>>> - The time value refers to the pts not the dts according to specs,
>>>> typically this value is 1 for there is delay (first pts is 1 and first
>>>> dts is 0)
>>> The specification pdf from May 1996 that I have at hand says nothing
>>> about that. But assuming that were true, the current check for time != 0
>>> would be wrong already.
>> The check is correct, and the message too, that's why it says "might".
>> In some case it does no harm. Latest qt specs are from 2007, and you can
>> refer to iso media specs also.
> 
> 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.

> 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, and both
pts and dts should be adjusted so b frames still have pts == dts.

[...]

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