[FFmpeg-devel] [PATCH][RFC - DO NOT MERGE] Revert "mov: Discard invalid CTTS."

[Michael Niedermayer]

On Mon, Mar 08, 2021 at 03:55:19PM +0000, Derek Buitenhuis wrote:
> This reverts commit 4093220029a4d77f272c491e9299680480a08c00.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis at gmail.com>
> ---
> I have CC'd Michael here, as he is the original author here and the "soltuion" is nor clear.
> 
> To explain this RFC:
> 
> We (Vimeo) have started seeing an uptick in broken MP4 files (what creates them is under
> investigation; we have reached out to some users in hopes that we can identify the broken
> muxer and contact its authors). They are broken in a very specific way: The CTTS box starts
> off normal, but then at some point, the duration field of the CTTS entries starts to increment
> on each entry, and this continues until the end of the file, resulting in incorrect and insane
> PTS/DTS differences (like PTS/DTS differing by 15 minutes / 10,000 frames). I have linked both
> a text dump of the boxes and a trimmed moov box as an example in [1] and [2]. I can only provide
> a full file (~12 GiB) privately.
> 
> Thes files *happen* to decode linearly just fine, of course, because FFmpeg doesn't care about the
> CTTS info in that case, but if you seek, everything will explode, as you would expect.
> 
> So, anyway, in the meanwhile, I was writing a simple detection filter for these sorts of files,
> and I noticed it only detected some of them, and this lead me to 1fb9efbda0149c3491529ea5fa92cfdcb8cebfaa.
> What happens is that if the file is long enough, the incrementing CTTS entry durations eventually
> trigger this, as far as I can tell, totally arbitrary check for 1<<28, and that results in the
> entire CTTS being thrown away, and libavformat setting PTS==DTS for all packets, which both
> makes detection impossible, and breaks seeking in a completely different way.
> 
> So the options are:
>     * Revert 1fb9efbda0149c3491529ea5fa92cfdcb8cebfaa and detect them as I was already.
>     * Change the check to something else or some other value, which would still be totally
>       arbitrary - but maybe something like >16 frames. This would still leave seeking totally
>       broken, of course, since it will still set PTS==DTS for all packets. This is still maybe
>       detectable for API users by checking b_frames and has_b_frames of video_delay, but
>       PTS==DTS? Seems kind gross, but... eh.
>     * (Possibly in addition to other changes) Make the check an error, or make seeking return an
>       error when this sort of file is detected.
>     * Something I haven't listed here.
> 
> So, basically, all options suck, and I thought some people may have opinions on the least bad
> option, here - specifically Michael, as the author of the original patch.

I would try to detect the specific abberant muxer based on the input. 
Then have custom code in the demuxer which is based on reverse engnenering the 
issue to do a best effort to recover as much as is possible. And also print a big 
warning about the broken input / muxer so the user doesnt stay unaware.

You can see this also in 2 ways
1. It sucks its not our job to workaround someone elses bugs
2. It cool, it gives FFmpegs demuxer a unique advantage over competing demuxers

Implementing this will be more enjoyable to whoever does, if (s)he sees it as in
"2."

Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210308/ee34b439/attachment.sig>