[FFmpeg-devel] [PATCH] Avoid suppressing parse errors in matroska

[Ami Fischman]

> FFmpeg error handling sucks, there's only the option of failing completely
> (which often enough is really not reasonable) or given no indication of
> error at all, at most a error message.
> The primary guiding rule in either way is though: get as much possibly valid data
> out of the file as possible.
> Fail if there is nothing useful you can do with the data you get out of the file.
> Print error/warning messages if something is wrong (and this really should be
> complemented with extra error flags - though even more for decoders so that applications
> can decide when and at what kind of curruption they do or do not want to display a frame.

Interesting.  The goal of extracting as much valid data as
possible from an input file (as opposed to failing fast at the
first sign of corruption) means that any data corruption that
*is* encountered but not treated as fatal must be corrected for
in the internal data structures.  The reason I started my
investigation of this issue is that this kind of corruption (at
least) is not covered up.  For example, in ffmpeg-mt, ffmpeg dies
on the file in question with an FPE due to a division by 0:

Program terminated with signal 8, Arithmetic exception.
#0  0x00007fcb8a60ff86 in dump_stream_format (ic=<value optimized
out>, i=0, index=<value optimized out>, is_output=0) at
libavformat/utils.c:3194
3194        av_log(NULL, AV_LOG_DEBUG, ", %d, %d/%d",
st->codec_info_nb_frames, st->time_base.num/g, st->time_base.den/g);
(gdb) where
#0  0x00007fcb8a60ff86 in dump_stream_format (ic=<value optimized
out>, i=0, index=<value optimized out>, is_output=0) at
libavformat/utils.c:3194
#1  0x00007fcb8a6107b9 in dump_format (ic=0x885300, index=0,
url=<value optimized out>, is_output=0) at libavformat/utils.c:3295
#2  0x00000000004097c4 in ?? ()
#3  0x00007fcb893e5695 in ?? () from /usr/lib/libSDL-1.2.so.0
#4  0x00007fcb8942ae09 in ?? () from /usr/lib/libSDL-1.2.so.0
#5  0x00007fcb891be9ca in start_thread (arg=<value optimized out>) at
pthread_create.c:300
#6  0x00007fcb88f1b70d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:112
#7  0x0000000000000000 in ?? ()

(note this is in a log message, but in a somewhat older version
of the code the same corrupt input makes it as far as
libavutil/mathematics.c:av_rescale_rnd with c==0 because a
stream's time_base.{den,num} are both zero when a parse error had
been encountered, triggering a SIGFPE for div-by-0).

Given my (very limited) understanding of the architecure of the
code it's unclear to me how the goal you stated can be achieved.
In particular when parsed structures are memset to 0 as
initialization, and parsing returns early (but silently) when
unexpected values are encountered, it seems like there must be
many ways for decoding paths to end up being fed (invalid) zeros
for fields that the parser never got far enough to parse.

> It might be that there's a bug in ffplay or just that a timestamp is ridiculously
> high or something else, but any of these are an application's job to decide
> what to do with, not for libavformat to discard the data just to protect applications
> "from themselves".

An alternative point of view (mine :)) is that the users should
be protected from bad data by the applications they use.  If
libavformat externalized the fact that errors were encountered
during parsing then applications using it could make a policy
decision about what to do about it.  Is that possible today?

> I am sure that libavformat can be more helpful to these applications,
> but just failing is not a soution to playback issues.

The opposite POV is that failing where an app can check for
it (i.e. via error return code) is preferable to failing
unexpectedly (i.e. via SIGFPE).  It'd be nice to avoid the
SIGFPE, whatever the mechanism :)

Cheers,
-a