> On Jul 18, 2024, at 17:26, Marton Balint <c...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
