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

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 6235 bytes --]

Hi

On Tue, Jul 29, 2025 at 12:01:50AM +0200, Nicolas George wrote:
> 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.
> 

[...]

> > * 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.

well, you may have deep call stacks

user_app->libavfilter->libavformat->libavcodec->decoder->jpeg_decoder->...

and the user of the user_app needs to know what file had what failure
so the error details must pass through all this.

Some of the API calls failing here can be final calls that are
expected to leave nothing allocated on a failure return.

also other subsequent and prior errors may have occured in some contexts
I thiink we want to be able to distingish what caused teh current error from
the previous or next


[...]

> > * 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?

it can be both

lets say we have 4 slices that can be independantly decoded.
We try to decode the first and hit an error. Now we continue trying the
2nd and it too has an error, and so on. only once we tried all can we
detrmine that the whole frame is a loss but now we have 4 independant errors.
We need to report these 4 errors together as the cause of the frame
failing to decode. None of them has "higher" priority than the others.
We would have been able to decode the frame somewhat had one remained.

so its a failure to decode file because failure to decode the only keyframe
which failed because all 4  slices had errors (error1, error2, error3, error4)

further you could wrap this in a failure to stream this because the input
failed to decode and so on ...

of course there has to be some limit on how much we can represent


> 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.)

people copy and paste error messages into bugreports and such,
developer details can be helpfull in fixing bugs


[...]
> > 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.

I see what you suggest. The problem is simply that we likely dont have the
manpower to implement such explicit forwarding of errors throughout the
codebase

My suggestion doesnt need it, but yes globals are ugly, yes


> 
> > > - 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.

thats possible, but the same problem with manpower exists.
If every structure needs to be changed, I have to point out that what i
suggested needs no such change.

What i suggested can simply be done with very little change to existing code
That simplicity is directly tied to the use of globals.

If you want to implement a system with context based error details,
instead of th global system i suggested
iam perfectly fine with that, iam just saying it looks like quite a bit
more work and not something i can add to my TODO list

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".