[FFmpeg-devel] [PATCH] Add av_file_get_size() and av_file_read(), replace cmdutils.h:read_file().

[Michael Niedermayer]

On Tue, Dec 21, 2010 at 03:49:52PM +0100, Stefano Sabatini wrote:
> On date Tuesday 2010-12-21 13:02:52 +0100, Nicolas George encoded:
> > Le decadi 30 frimaire, an CCXIX, Stefano Sabatini a ?crit?:
> > > The problem is that the external function doesn't know the context of
> > > where the error occurred, and an error code is not enough, for example
> > > to distinguish if the error occurred in open(), lstat(), mmap() or if
> > > the file was too big (in this case an AVERROR(EINVAL) error code
> > > doesn't give a clue about the exact failure reason).
> > > 
> > > I spent unnamerable hours figuring out the meaning of lame error
> > > messages which provided no hint about where the failure occurred, so
> > > as a software user I tend to give *much* importance to these usability
> > > issues.
> > 
> > I have already met problem you are discussing here, as well as probably most
> > programmers.
> > 
> > The core of the problem resides in the fact that "error reporting" covers
> > two similar but fundamentally distinct tasks:
> > 
> > 1. reporting to the calling context a simple cause so it can hopefully be
> >    recovered (EAGAIN/EINTR, EEXIST vs. EACCESS, etc.);
> > 
> > 2. reporting to the used a detailed cause for the error so he can understand
> >    the problem and fix it.
> > 
> > The errno system works because libc functions are low-level and perform only
> > a simple task each: for example if opening a file fails, the calling context
> > knows which file was being opened.
> > 
> > Anything higher-level that wants to use integer error codes imitating the
> > errno system will stumble on the problem sooner or later.
> > 
> > Gtk+ has a nice way of solving the problem. Error causes are reported
> > through an additional "GError **" argument to each function that can
> > possibly fail. The GError structure has both an integer error code
> > (actually, a pair of integer error codes, allowing different parts of the
> > library to define new error codes independently) for automatic handling and
> > a human-readable string.
> > 
> > Libav* could possibly imitate the solution. Of course, adding an extra
> > argument to all existing functions is not an option, but new functions could
> > benefit from it. Something like that maybe:
> > 
> > - Add an AVError structure in avutil.
> > 
> > - New functions that can have something subtle to report take an additional
> >   pointer to an AVError. If it is null, the human-readable messages
> >   automatically goes to av_log.
> > 
> > - AVFoobarContext and similar structures gain an AVError field, so that
> >   whenever there is a context available, there is a place to store the
> >   error.
> > 
> > What do you think about it?
> 
> Currently we do:
> ret = av_foo(..., log_offset, log_ctx);
> if (ret < 0)
>    return ret;
> 
> with this approch is difficult to understand when the called will
> actually log something in case of error, which basically depends on
> the called function implementation (for example no error is logged in
> most cases when there is a memory allocation problem), also in some
> cases makes sense to log another more high-level message.

nothing stops you from logging a message before return <0


>
> Also this system is quite inflexible, for example what if you want to
> show the error message in a GUI? (Well I suppose you may implement a
> per-thread stack of error messages, and fill it with an ad-hoc
> implementation of the av_log callback, but this is not very
> convenient.)

I dont see what is inconventient to write a 10 line av_log_callcack() that
passes its strings to thread local storage (if the GUI has a log window
per thread) or global storeage through a mutex if theres one window
if what you have in mind is a kind of
show_lasterror_in_window_to_user(string)
getting this string can be implemented in log.c/h

it might look like this then:
open_log_buffer(log_ctx);
ret = av_foo(..., log_offset, log_ctx);
if (ret < 0){
    show_lasterror_in_window_to_user( get_log_buffer(log_ctx) );
    return ret;
}

this also can easily show all prior warnings or debug or just errors depending
on log level


[...]
> when using the errbuf in the log_ctx:
> ret = av_foo(..., log_ctx->errbuf, log_ctx->errbuf_size);

and if errbuf_size is too small ...


> ...
> 
> then we may find some way to automatically fill errbuf with the
> default error string for ret if no specific message is given
> (BTW extending av_log this way:
> av_log("Error occurred with code %e: %E"\n, errcode, errcode)
> may be useful).
> 
> Another problem with this approach is when you have a deep stack of
> function calls, and each one needs to report the error message to the
> callee.

and each level may want to add to it
like:
1. mmap failed
2. opening direct rendering window failed
3. playing foobar.avi failed
4. playing playlist entry line 125 of bar.list failed


> Also how to manage when we need to deal with messages with
> different log levels (not all messages pertain to error conditions)?
> 
> In general to design a sane error reporting system is difficult (that
> explains why most software sucks with it).

Its not hard to do with av_log() but it involves some complexity that iam not
completely happy about. Also as said elsewhere there are some low level
functions like mmaping a file or a generic AVL tree where a av_log dependancy
is simply annoying.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are best at talking, realize last or never when they are wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101227/a970384b/attachment.pgp>