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

[Marton Balint <cus@passwd.hu>]

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;


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