[FFmpeg-devel] [PATCH 00/13] check all fclose usage

[Ronald S. Bultje]

Hi,

On Tue, Jan 12, 2016 at 10:07 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
wrote:

> On Tue, Jan 12, 2016 at 9:43 AM, Ronald S. Bultje <rsbultje at gmail.com>
> wrote:
> > Hi,
> >
> > On Tue, Jan 12, 2016 at 7:52 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
> > wrote:
> >
> >> On Tue, Jan 12, 2016 at 4:38 AM, wm4 <nfxjfg at googlemail.com> wrote:
> >> > This makes no sense. Even if fclose() should fail for
> >> > whatever obscure reasons there might be, reading already worked
> >> > without errors, and thus closing failure has no meaning to use.
> >>
> >> Well, reading may not have worked, and the fclose may have been called
> >> in a failure path.
> >
> >
> > Then the error should be in the code path of fread(), not fclose().
> > Displaying an error (in whatever way) related to close while the actual
> > problem was reading data is going to be confusing to the user.
>
> Read the rest of it; checking for every fread/fseek is not productive;
> there are at least 3 of fread/fseek in the example I gave. Printing at
> the time of closure is a natural means of doing things, again see:
> https://www.gnu.org/ghm/2011/paris/slides/jim-meyering-goodbye-world.pdf
> (particularly slide 7).


He's very smart, but you still have to see his advice in context, also see
[1].

Most production uses of ffmpeg involve long-running processes in a
multi-component pipeline, where fail-to-read will cause errors downstream.
Reading the file from disk is only one small component. Let's say that I
read a buffer of 10 bytes but I only got 5 because whatever went wrong. I
parse the input buffer of 5 bytes, it's obviously corrupt, decoding stream
goes wrong, and the decoder displays error ("ffvp9: corrupt input stream,
failed to decode"). Note how this is the topmost error in stderr, therefore
the user (logically) thinks: "FFmpeg's decoder sucks. FFmpeg sucks." That's
bad. [2]

The first error should (ideally) be indicative of the actual problem. So,
if the read is the error, the error should be associated with that read
early on to help the user diagnose the actual source of the problem.

Ronald

[1] https://en.wikipedia.org/wiki/Argument_from_authority
[2] 10s or 1000s of bytes or frames failing to decode later, there's some
little error saying the file descriptor failed to close. Did you ever look
at line 1000 in your error log?