[FFmpeg-devel] [RFC] Internal error messages

[Diederick C. Niehorster]

On Thu, Dec 31, 2020 at 9:52 PM <Derek Buitenhuis> wrote:
>
> On 31/12/2020 13:36, Nicolas George wrote:
> > This mail is about a project I have to make FFmpeg's API and
> > infrastructure more convenient. For a common introduction, see this
> thread:
> > https://ffmpeg.org/pipermail/ffmpeg-devel/2020-December/274167.html
> >
> > The way we currently report errors is fine for command-line tools
> running in
> > a terminal, but it does not suit GUI applications at all: they will get
> a
> > generic error, translated into a vague string like "invalid argument",
> while
> > a more precise error message that tells which argument is invalid and
> how
> > will go to the log, and disappear in a black-hole like
> ~/.xsession-errors or
> > even /dev/null; at best, the application will have registered a log
> callback
> > and display the whole log to the user.
> >
> > I want to add a new API to return an error all at once, something like
> this:
> >
> >     if (... failure condition...)
> >         return av_error_set(ctx, AVERROR_INVALIDDATA,
> >                             "Syntax error in file ${file} line ${line}",
> >                             "file", finename,
> >                             "line", av_d2str(line),
> >                             NULL);
> >
> > The complete error message will be stored into a pre-allocated
> AVErrorStatus
> > structure in the context, and can be then retrieved by the application
> > using:
> >
> >     av_error_get(ctx, buf);
> >
> > and displayed to the user in any convenient way (dialog box, HTTP
> response,
> > etc.).
>
> Finally having proper error support would be very nice; I like this idea a
> lot.

+1 from me too. Code using the libraries through their API has a hard
(lets call it impossible) job showing detailed and informative error
messages now. Indeed "invalid argument" or "I/O error" or so is little
helpful often. So i really like this idea, it would make the library a
lot more useful (also from tools user btw).

One thing to add is this: Errors are often propagated back through
various layers of function calls. Some next level may:
1. decide something is not an error/should be ignored. It should be
able to unset the current error (if it was set, so something like
av_error_clear(int error_code) which only unsets if error code
matches)
2. wish to add information about the error. Often useful info is not
available at the site where the error is generated, but only (several)
levels up. Errors should be set where they are generated, and info can
optionally be added as the stack is traversed back to the user's call.
Kinda like catch and rethrow with added info/std::nested_exception in
C++. something like
av_error_set(ctx, optional_additional_error_code,
    "additional formatted error string, like in the usage example");

> >
> > For compatibility, av_log(ctx, ...) will not only print to stderr, but
> also
> > keep the last line(s) of log in the AVErrorStatus buffer in ctx, so that
> > code that still does the good old:
> >
> >         av_log(ctx, AV_LOG_ERROR, "Syntax error in file %s line %d\n",
> >                filename, line);
> >         return AVERROR_INVALIDDATA;
> >
> > will now work ok with av_error_get().
>
> What I'm a little iffy on is this last part, mostly because a lot of code
> in
> FFmpeg just returns an error without any av_log call, so the current error
> buffer in the context will at best be unrelated, or at worst, really
> misleading to the user.

Agreed, this can't work consistently, I'd skip it. And it highlights
also that there may be stale errors returned through av_error_get().
So:
1. av_error_get() should clear error state (by default?)
2. av_error_get should perhaps have an error_code input argument, so
that an error is only returned if the error code (uppermost if nested
according to my above suggestion) matches--else it is assumed we're
dealing with a stale error. if error_code==0, such filtering is
bypassed.

By the way, av_error_get should perhaps have last in the name, like lasterr.

Cheers,
Dee