Re: [FFmpeg-devel] [RFC PATCH v2 0/1] avutil/error: Provide better feedback about unknown error codes

[Zhao Zhili <quinkblack@foxmail.com>]

> On Jul 18, 2024, at 17:26, Marton Balint <cus@passwd.hu> wrote:
> 
> 
> On Tue, 16 Jul 2024, Andrew Sayers wrote:
> 
>> I'm having trouble managing this conversation.  On one hand, you've brought up
>> several important details that would need to be included in a new patch.
>> On the other hand, I'm pretty sure we're talking past each other on the big
>> problems, and need to start over.  So let's fork the discussion.
>> 
>> # First, let's haggle over some details
>> 
>> The patch below fixes a number of small issues brought up by your comments...
>> 
>> Error numbers are always expressed in the code as either uppercase hex numbers
>> or FourCCs (or ThreeCCs, but you get the point).  This patch prints error codes
>> as hex, which is no less unintelligible for ordinary users, might make problems
>> easier to find on Google, and will sometimes make them easier to grep for.
>> 
>> Having said that, this patch prints non-negative numbers in decimal,
>> because all bets are off if that manages to happen.
>> 
>> A developer could create an error code that just happens to be valid ASCII.
>> In that situation, the previous patch would have printed something like
>> "Unrecognised error code \"~!X\"" occurred", which is worse than the current
>> behaviour.  This patch includes both (hex) number and name in those messages.
>> 
>> This patch adds "please report this bug" for all unknown error messages.
>> I'll cover the reasoning below, but the relevant detail is that the previous
>> patch just gave users a different heiroglyphic before abandoning them.
>> 
>> # Second, let's talk about the big picture
>> 
>> Consider the following scenario:
>> 
>> 1. a new developer adds some code to FFmpeg that calls an existing function
>> 2. it turns out that function sometimes calls another function that
>>  returns a variety of internal error codes (FFERROR_REDO among others)
>> 3. their testing uncovers a situation that intermittently returns an unknown
>>  error number, but they don't notice there are two different numbers
>> 4. they spend a lot of time tracking down an error message based on a random
>>  number, and eventually fix "the" bug (actually one of two intermittent bugs)
>> 5. the review doesn't catch the other bug, and the new code goes live
>> 6. a user trips over the other bug and sees "Error number <number> occurred"
>> 7. the user wastes a lot of time trying to work out what they did wrong,
>>  badmouthing FFmpeg to anyone who will listen as they do
>> 8. they eventually catch the attention of a developer
>> 9. that developer spends a lot of time bisecting the bug
>> 10. the new developer is expected to fix this patch, and feels like they're
>>   to blame for the whole situation
>> 
>> An error message like "Unrecognised error code \"REDO\" occurred, please report
>> this bug" would give the newbie a fighting chance to catch both bugs at step 3,
>> would make step 4 much shorter, and would optimise steps 7-10 to almost nothing.
>> 
>> Catching this in a fate test would involve checking for an unknown function
>> returning an unknown number that gets reused in a context it's subtly
>> inappropriate for.  I have no idea where to begin with that, and anyway it
>> wouldn't help a developer in the process of tracking down an intermittent bug.
> 
> The fate test should be added for checking that all ffmpeg-specific errors (defined with AVERROR_ prefix in error.h) has a textual representation. That does not help the FFERROR_REDO case, but it does help if somebody adds a new AVERROR_xxx constant but forget to add the text counterpart for it.
> 
>> 
>> As mentioned above, the v2 patch adds "please report this bug" in a few places.
>> Any negative error code can be valid, but returning a raw error number is always
>> a bug, so it's all the same to users - if they see this message, they're not
>> expected to fix it themselves, they're expected to let us know.
> 
> It is not necessarily a bug though. AVERROR values can be based on any system errno, and not all errno-s used by system libraries necessarily are supported by the platform strerrro_r() or our drop-in replacement if
> that is not available.
> 
> I still feel like you are adding a lot of code for questionable benefit, so I suggest the following simple change:
> 
> diff --git a/libavutil/error.c b/libavutil/error.c
> index 90bab7b9d3..f78c4b35b4 100644
> --- a/libavutil/error.c
> +++ b/libavutil/error.c
> @@ -20,6 +20,7 @@
> #define _XOPEN_SOURCE 600 /* XSI-compliant version of strerror_r */
> #include <stdio.h>
> #include <string.h>
> +#include "avutil.h"
> #include "config.h"
> #include "avstring.h"
> #include "error.h"
> @@ -126,7 +127,7 @@ int av_strerror(int errnum, char *errbuf, size_t errbuf_size)
>         ret = -1;
> #endif
>         if (ret < 0)
> -            snprintf(errbuf, errbuf_size, "Error number %d occurred", errnum);
> +            snprintf(errbuf, errbuf_size, "Error number %d (%s) occurred", errnum, av_fourcc2str(-errnum));
>     }
> 
>     return ret;

I like this version.

> 
> 
> Regards,
> Marton
> _______________________________________________
> 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".

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