Re: [FFmpeg-devel] [RFC] Advanced Error Codes

[Nicolas George <george@nsup.org>]

Michael Niedermayer (HE12025-07-06):
> Hi Nicolas

Hi. Sorry I procrastinated to reply to that too.

> theres a fixed length array of AdvancedError structs.

More global state is not a good idea.

> AdvancedError has a fixed length char field.
> Thats where a single custom text string can be stored.
> Which would be maybe "Whaetver you like pizza" if favorite_food was "pizza"
> its very very simple
> 
> typdef struct AdvancedError {
>     const char *error_name;          ///< like "Timeout"       or  "Invalid Bitstream"
>     const char *operation_name;      ///< like "Network Read"  or  "H.264 block parsing"
>     const char *source_filename;     ///< like ffmpeg.c
>     int source_line_number;          ///< like 123 for line 123 in ffmpeg.c
>     char free_form_description[FREE_FORM_LEN];  ///< anything extra that doesnt fit in pointers to static const strings
>     int64_t this_error;
>     int64_t previous_error;          ///< A previous related error, if any
>     int64_t timestamp;               ///< Timestamp at which the error occured
>     int thread_id;
> } AdvancedError;

On principle, I think this kind of structure is a good idea, and my plan
would use a similar structure.

My point is that it should reside in contexts, not as global state.

(As a more pedestrian comment, I would suggest to have the free-form
description a “const char *” format string with placeholders and use the
buffer for key-value pairs to be used in the format string.)

> > We already pass contexts around everywhere.
> 
> * Many places but not everywhere,

Granted, it was hyperbole. But…

I think we can sort the functions in our code into two groups:

The first group:

- do not get a context;

- perform a rather simple task;

- could be called in tight loops, even when they fail repeatedly, and
  therefore should not waste time fiddling with strings;

- do not have enough information to design a useful error message
  anyway.

The second group, is all the opposite. I do not think there is much in
between.

My point is: when a small function in the first group fails, it is the
responsibility of the large function that calls it to build the error
message, using the error code the simple function returned. And then we
have a context.

> * The contexts do not match the lifetime of the errors

How so? The typical code looks like:

42	ret = av_do_something(ctx, params);
43	if (ret < 0) {
44	    report_error(ctx, ret);
45	    return some_code;
46	}

ctx does not disappear between line 42 and line 44.

> * The contexts can be accessed from multiple threads

Only with mutexes. For our multithreaded code, we can synchronize errors
too if necessary. And for multithreaded applications we can document
that accessing the error must be done before releasing the mutex, in
case the application is not very smart and did not guess it.

> * There can be multiple relevant errors for a single context

Again: how so? If multiple errors appear in succession, the proper way
to deal with it is to report the first error as it has happened. Or
possibly copy it. Then processing continues and the next error happens,
it can reuse the storage.

Or do you mean cascading errors: error A caused error B caused error C?
Then sure, we must handle that, that was also my plan. We can include
room for a few errors in the context, not just one. Or better, design
the structure to be able to store a few cascading errors all using the
same buffer: that way, if the first error is terse, the second error has
more room.

> "square root of negative argument" in mydecoder.c at line 123 is more
> informative than EINVAL, and having no clue where or why that happened
> 
> for example decode() returning
> AVERROR(EINVAL)
> with 5000 lines of cryptic parser errors in messages
> 
> vs.
> 
> "square root of negative argument" in mydecoder.c at line 123, called by
> vector_length_compute() line 511 called by
> read_nnet() line 732 free_form_description="training_set_maybe_corrupted.dat"

I will stop you right here: the first one is terrible, but yours is
terrible too.

Errors messages are for users. What you suggest is not an error message,
it is a debug message meant for developers.

In particular, the error message presented to users should not by
default include source file names and lines, because it confuses users.
(We might want to enable them in the fftools, because the fftools are
meant for power users. But remember that this is also for GUI
applications.)

The issue is not just the line numbers. “Square root of negative
argument” is absolutely useless too: user never asked to compute a
square root, they asked to play a media file. What the user needs to
know is how to fix the error: change a command-line option, change the
permission of a file, report a bug to the developers, or just too bad
the file is damaged beyond repair.

> Note by the time this is returned decode failed and many contexts have
> been destroyed.

You mean:

42	ret = av_do_something(ctx, params);
43	if (ret < 0) {
44	    av_free_this_context(&ctx);
45	    return some_code;
46	}

? Well, it should be:

42	ret = av_do_something(ctx, params);
43	if (ret < 0) {
44	    av_forward_error(caller_ctx, ctx);
45	    av_free_this_context(&ctx);
46	    return some_code;
47	}

I.e.: when freeing a context on error, the error messages must be
forwarded to the caller.

> > - We allocate the memory for error messages contexts. That way, we avoid
> >   the issue of using locks or thread-local storage but do not have a
> >   hard limit on simultaneous errors.
> malloc() can fail and it needs to be freed exactly once. And not freed
> before the caller is finished doing anything it likes to do with the error
> information. And it very possibly wants to pass it on to its caller

Sorry, there was two words missing in my sentence, and that made it
nonsensical. It should have been:

- We allocate the memory for error messages IN THE contexts.

i.e. we add the room for the error structure(s) to the memory we reserve
for the contexts when creating them, and we free all at the same time.
Possibly, we even allocate it in same malloc() as the context.

> > - We do not change the return type of all our API, we still return “a
> >   negative AVERROR code” to keep source compatibility, and use the best
> >   code we can find.
> Then you no longer know which error relates to which message

When “ret = av_do_something(ctx, …)” is negative, the error is in ctx
and stays there until the application does something else with ctx. The
application is supposed to do something with the error before that. If
it is a multithreaded application, it must not release the mutex
protecting ctx before using the error.

> So the problem that I see, isnt solved.

I think it is.

Regards,

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".