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-requ...@ffmpeg.org with subject "unsubscribe".
