[FFmpeg-devel] [PATCH] avutil/error: Provide better feedback about unknown error codes

[FFmpeg-devel] [PATCH] avutil/error: Provide better feedback about unknown error codes

[Andrew Sayers]

AVERROR messages should always be less than zero,
and are usually based on three or four ASCII characters.

For error codes that aren't explicitly handled by error.c (e.g. FFERROR_REDO),
print the ASCII code so the user has a little more information.

If a non-negative number somehow gets passed to this function,
print a message saying this shouldn't happen.
---
 libavutil/error.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/libavutil/error.c b/libavutil/error.c
index 90bab7b9d3..c40b54b1f9 100644
--- a/libavutil/error.c
+++ b/libavutil/error.c
@@ -119,6 +119,38 @@ int av_strerror(int errnum, char *errbuf, size_t errbuf_size)
     }
     if (entry) {
         av_strlcpy(errbuf, entry->str, errbuf_size);
+    } else if (
+        -errnum <= 0xFFFFFFFF
+        && ((-errnum >>  0) & 0xFF) >= 0x20 && ((-errnum >>  0) & 0xFF) <= 0x7F
+        && ((-errnum >>  8) & 0xFF) >= 0x20 && ((-errnum >>  8) & 0xFF) <= 0x7F
+        && ((-errnum >> 16) & 0xFF) >= 0x20 && ((-errnum >> 16) & 0xFF) <= 0x7F
+        && (
+           (((-errnum >> 24) & 0xFF) >= 0x20 && ((-errnum >> 24) & 0xFF) <= 0x7F)
+           || !((-errnum >> 24) & 0xFF)
+        )
+    ) {
+        if ((-errnum >> 24) & 0xFF) {
+            snprintf(
+                errbuf,
+                errbuf_size,
+                "Unrecognised error code \"%c%c%c%c\" occurred",
+                (-errnum >>  0) & 0xFF,
+                (-errnum >>  8) & 0xFF,
+                (-errnum >> 16) & 0xFF,
+                (-errnum >> 24) & 0xFF
+            );
+        } else {
+            snprintf(
+                errbuf,
+                errbuf_size,
+                "Unrecognised error code \"%c%c%c\" occurred",
+                (-errnum >>  0) & 0xFF,
+                (-errnum >>  8) & 0xFF,
+                (-errnum >> 16) & 0xFF
+            );
+        }
+    } else if (errnum >= 0) {
+        snprintf(errbuf, errbuf_size, "Impossible: non-negative error number %d occurred", errnum);
     } else {
 #if HAVE_STRERROR_R
         ret = AVERROR(strerror_r(AVUNERROR(errnum), errbuf, errbuf_size));
-- 
2.45.2

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

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

[Marton Balint]

On Mon, 15 Jul 2024, Andrew Sayers wrote:

> AVERROR messages should always be less than zero,
> and are usually based on three or four ASCII characters.
>
> For error codes that aren't explicitly handled by error.c (e.g. FFERROR_REDO),
> print the ASCII code so the user has a little more information.

All ffmpeg internal error codes (including the ones having some special 
tag representation) should be handled by error.c. The user should never 
receive FFERROR_REDO, that is an internal error code, it should never 
reach the user. Therefore I see no benefit in disclosing the error bytes, 
because that is not the proper fix.

Regards,
Marton


>
> If a non-negative number somehow gets passed to this function,
> print a message saying this shouldn't happen.
> ---
> libavutil/error.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/libavutil/error.c b/libavutil/error.c
> index 90bab7b9d3..c40b54b1f9 100644
> --- a/libavutil/error.c
> +++ b/libavutil/error.c
> @@ -119,6 +119,38 @@ int av_strerror(int errnum, char *errbuf, size_t errbuf_size)
>     }
>     if (entry) {
>         av_strlcpy(errbuf, entry->str, errbuf_size);
> +    } else if (
> +        -errnum <= 0xFFFFFFFF
> +        && ((-errnum >>  0) & 0xFF) >= 0x20 && ((-errnum >>  0) & 0xFF) <= 0x7F
> +        && ((-errnum >>  8) & 0xFF) >= 0x20 && ((-errnum >>  8) & 0xFF) <= 0x7F
> +        && ((-errnum >> 16) & 0xFF) >= 0x20 && ((-errnum >> 16) & 0xFF) <= 0x7F
> +        && (
> +           (((-errnum >> 24) & 0xFF) >= 0x20 && ((-errnum >> 24) & 0xFF) <= 0x7F)
> +           || !((-errnum >> 24) & 0xFF)
> +        )
> +    ) {
> +        if ((-errnum >> 24) & 0xFF) {
> +            snprintf(
> +                errbuf,
> +                errbuf_size,
> +                "Unrecognised error code \"%c%c%c%c\" occurred",
> +                (-errnum >>  0) & 0xFF,
> +                (-errnum >>  8) & 0xFF,
> +                (-errnum >> 16) & 0xFF,
> +                (-errnum >> 24) & 0xFF
> +            );
> +        } else {
> +            snprintf(
> +                errbuf,
> +                errbuf_size,
> +                "Unrecognised error code \"%c%c%c\" occurred",
> +                (-errnum >>  0) & 0xFF,
> +                (-errnum >>  8) & 0xFF,
> +                (-errnum >> 16) & 0xFF
> +            );
> +        }
> +    } else if (errnum >= 0) {
> +        snprintf(errbuf, errbuf_size, "Impossible: non-negative error number %d occurred", errnum);
>     } else {
> #if HAVE_STRERROR_R
>         ret = AVERROR(strerror_r(AVUNERROR(errnum), errbuf, errbuf_size));
> -- 
> 2.45.2
>
> _______________________________________________
> 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".

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

[Andrew Sayers]

On Mon, Jul 15, 2024 at 05:45:24PM +0200, Marton Balint wrote:
> 
> 
> On Mon, 15 Jul 2024, Andrew Sayers wrote:
> 
> > AVERROR messages should always be less than zero,
> > and are usually based on three or four ASCII characters.
> > 
> > For error codes that aren't explicitly handled by error.c (e.g. FFERROR_REDO),
> > print the ASCII code so the user has a little more information.
> 
> All ffmpeg internal error codes (including the ones having some special tag
> representation) should be handled by error.c. The user should never receive
> FFERROR_REDO, that is an internal error code, it should never reach the
> user. Therefore I see no benefit in disclosing the error bytes, because that
> is not the proper fix.
> 
> Regards,
> Marton

So it sounds like this patch is addressing two separate issues:

1. any messages caught by the test in the patch represent a bug in FFmpeg
   * how about I modify this patch to ask the user to report the bug?
   * would the ASCII error code help with triage?
2. FFERROR_REDO should be added to error.c
   * let me know if I should submit a separate patch for this
_______________________________________________
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".

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

[Marton Balint]

On Mon, 15 Jul 2024, Andrew Sayers wrote:

> On Mon, Jul 15, 2024 at 05:45:24PM +0200, Marton Balint wrote:
>>
>>
>> On Mon, 15 Jul 2024, Andrew Sayers wrote:
>>
>>> AVERROR messages should always be less than zero,
>>> and are usually based on three or four ASCII characters.
>>>
>>> For error codes that aren't explicitly handled by error.c (e.g. FFERROR_REDO),
>>> print the ASCII code so the user has a little more information.
>>
>> All ffmpeg internal error codes (including the ones having some special tag
>> representation) should be handled by error.c. The user should never receive
>> FFERROR_REDO, that is an internal error code, it should never reach the
>> user. Therefore I see no benefit in disclosing the error bytes, because that
>> is not the proper fix.
>>
>> Regards,
>> Marton
>
> So it sounds like this patch is addressing two separate issues:
>
> 1. any messages caught by the test in the patch represent a bug in FFmpeg
>   * how about I modify this patch to ask the user to report the bug?
>   * would the ASCII error code help with triage?

I don't really like adding extra code for this, and from an API point of 
view any negative error code can be valid, so you can't really warn about 
them.

If you want to make sure that every ffmpeg error code has a text, then add 
a fate test for checking it.

> 2. FFERROR_REDO should be added to error.c
>   * let me know if I should submit a separate patch for this

FFERROR_REDO is an avformat internal error code, av_strerror() being in 
avutil cannot properly support it.

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] [RFC PATCH v2 0/1] avutil/error: Provide better feedback about unknown error codes

[Andrew Sayers]

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.

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.


_______________________________________________
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] [RFC PATCH v2] avutil/error: Provide better feedback about unknown error codes

[Andrew Sayers]

AVERROR messages should always be less than zero,
and are usually based on three or four ASCII characters.

For error codes that aren't explicitly handled by error.c (e.g. FFERROR_REDO),
print the ASCII code so the user has a little more information.

If a non-negative number somehow gets passed to this function,
print a message saying this shouldn't happen.
---
 libavutil/error.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/libavutil/error.c b/libavutil/error.c
index 90bab7b9d3..9706578be6 100644
--- a/libavutil/error.c
+++ b/libavutil/error.c
@@ -119,6 +119,40 @@ int av_strerror(int errnum, char *errbuf, size_t errbuf_size)
     }
     if (entry) {
         av_strlcpy(errbuf, entry->str, errbuf_size);
+    } else if (
+        -errnum <= 0xFFFFFFFF
+        && ((-errnum >>  0) & 0xFF) >= 0x20 && ((-errnum >>  0) & 0xFF) <= 0x7F
+        && ((-errnum >>  8) & 0xFF) >= 0x20 && ((-errnum >>  8) & 0xFF) <= 0x7F
+        && ((-errnum >> 16) & 0xFF) >= 0x20 && ((-errnum >> 16) & 0xFF) <= 0x7F
+        && (
+           (((-errnum >> 24) & 0xFF) >= 0x20 && ((-errnum >> 24) & 0xFF) <= 0x7F)
+           || !((-errnum >> 24) & 0xFF)
+        )
+    ) {
+        if ((-errnum >> 24) & 0xFF) {
+            snprintf(
+                errbuf,
+                errbuf_size,
+                "Error number -0x%x (\"%c%c%c%c\") occurred, please report this bug",
+                -errnum,
+                (-errnum >>  0) & 0xFF,
+                (-errnum >>  8) & 0xFF,
+                (-errnum >> 16) & 0xFF,
+                (-errnum >> 24) & 0xFF
+            );
+        } else {
+            snprintf(
+                errbuf,
+                errbuf_size,
+                "Error number -0x%x (\"%c%c%c\") occurred, please report this bug",
+                -errnum,
+                (-errnum >>  0) & 0xFF,
+                (-errnum >>  8) & 0xFF,
+                (-errnum >> 16) & 0xFF
+            );
+        }
+    } else if (errnum >= 0) {
+        snprintf(errbuf, errbuf_size, "Impossible: non-negative error number %d occurred, please report this bug", errnum);
     } else {
 #if HAVE_STRERROR_R
         ret = AVERROR(strerror_r(AVUNERROR(errnum), errbuf, errbuf_size));
@@ -126,7 +160,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 -0x%X occurred, please report this bug", -errnum);
     }
 
     return ret;
-- 
2.45.2

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

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

[Anton Khirnov]

Quoting Marton Balint (2024-07-16 01:01:20)
> I don't really like adding extra code for this, and from an API point of 
> view any negative error code can be valid, so you can't really warn about 
> them.
> 
> If you want to make sure that every ffmpeg error code has a text, then add 
> a fate test for checking it.
> 
> [...]
> 
> FFERROR_REDO is an avformat internal error code, av_strerror() being in 
> avutil cannot properly support it.

+1

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

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

[Michael Niedermayer]

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

On Tue, Jul 16, 2024 at 12:13:10PM +0100, Andrew Sayers wrote:
> AVERROR messages should always be less than zero,
> and are usually based on three or four ASCII characters.
> 
> For error codes that aren't explicitly handled by error.c (e.g. FFERROR_REDO),
> print the ASCII code so the user has a little more information.
> 
> If a non-negative number somehow gets passed to this function,
> print a message saying this shouldn't happen.
[...]

> +    } else if (errnum >= 0) {
> +        snprintf(errbuf, errbuf_size, "Impossible: non-negative error number %d occurred, please report this bug", errnum);
>      } else {
>  #if HAVE_STRERROR_R
>          ret = AVERROR(strerror_r(AVUNERROR(errnum), errbuf, errbuf_size));
> @@ -126,7 +160,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 -0x%X occurred, please report this bug", -errnum);
>      }

I think this (asking for a report and pointing out to the user that
this isnt supposed to happen), is a good idea

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.

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

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

[Marton Balint]

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

[FFmpeg-devel] [PATCH v3 0/3] Protect against undocumented error codes

[Andrew Sayers]

I hadn't noticed av_fourcc2str() before - so long as it's patched to handle "!",
it would be a clear improvement over the earlier patches.

The example in the "!" patch is "BUG!", which is handled in error.c, but shows
that "!" is considered a valid FourCC character.

I think I understand the fate argument now, but this is my first fate patch -
please review it as such!

I still don't see the problem with asking for a bug report.  If this patch is
unacceptable, could you give an example of a code where we would reject a
request to add an error string?

_______________________________________________
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] [PATCH v3 1/3] avutil/utils: Allow "!" in FourCCs

[Andrew Sayers]

For example, AVERROR_BUG is "BUG!"
---
 libavutil/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavutil/utils.c b/libavutil/utils.c
index 94d247bbee..a94589e873 100644
--- a/libavutil/utils.c
+++ b/libavutil/utils.c
@@ -81,7 +81,7 @@ char *av_fourcc_make_string(char *buf, uint32_t fourcc)
         const int print_chr = (c >= '0' && c <= '9') ||
                               (c >= 'a' && c <= 'z') ||
                               (c >= 'A' && c <= 'Z') ||
-                              (c && strchr(". -_", c));
+                              (c && strchr(". -_!", c));
         const int len = snprintf(buf, buf_size, print_chr ? "%c" : "[%d]", c);
         if (len < 0)
             break;
-- 
2.45.2

_______________________________________________
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] [PATCH v3 2/3] avutil/error: Provide better feedback about unknown error codes

[Andrew Sayers]

AVERROR messages should always be less than zero, and are often FourCCs.

For error codes that aren't explicitly handled by error.c (e.g. undocumented
system error codes, or internal error codes that leaked to the public API),
print the FourCC code so the user has a little more information to work with.

If a non-negative number somehow gets passed to this function,
print a message saying this shouldn't happen.
---
 libavutil/error.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavutil/error.c b/libavutil/error.c
index 90bab7b9d3..0f748bd9e5 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"
@@ -119,6 +120,8 @@ int av_strerror(int errnum, char *errbuf, size_t errbuf_size)
     }
     if (entry) {
         av_strlcpy(errbuf, entry->str, errbuf_size);
+    } else if (errnum >= 0) {
+        snprintf(errbuf, errbuf_size, "Impossible: non-negative error number %d occurred, please report this bug", errnum);
     } else {
 #if HAVE_STRERROR_R
         ret = AVERROR(strerror_r(AVUNERROR(errnum), errbuf, errbuf_size));
@@ -126,7 +129,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 -0x%X (%s) occurred, please report this bug", -errnum, av_fourcc2str(-errnum));
     }
 
     return ret;
-- 
2.45.2

_______________________________________________
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] [PATCH v3 3/3] tests/fate/source-check: Check for AVERROR codes without error strings

[Andrew Sayers]

---
 tests/fate/source-check.sh | 8 ++++++++
 tests/ref/fate/source      | 1 +
 2 files changed, 9 insertions(+)

diff --git a/tests/fate/source-check.sh b/tests/fate/source-check.sh
index 4d7e175784..71f01cbdec 100755
--- a/tests/fate/source-check.sh
+++ b/tests/fate/source-check.sh
@@ -45,4 +45,12 @@ git grep -E 'av_clip *\(.*, *(-2 *, *1|-4 *, *3|-8 *, *7|-16 *, *15|-32 *, *31|-
 ' *, *33554431|-67108864 *, *67108863|-134217728 *, *134217727|-268435456 *, *'\
 '268435455|-536870912 *, *536870911|-1073741824 *, *1073741823) *\)'| grep -v fate/source
 
+echo "AVERROR_xxx constants with no associated error string:"
+git show HEAD:libavutil/error.c \
+    | sed -ne 's/.*AVERROR_\([^ ]*\).*/\1/p' libavutil/error.h \
+    | while read ERROR
+      do git grep -q "$ERROR" libavutil/error.c \
+         || echo "Please add ERROR_TAG($ERROR)"
+      done
+
 exit 0
diff --git a/tests/ref/fate/source b/tests/ref/fate/source
index 1703b36c02..b84cc88a6e 100644
--- a/tests/ref/fate/source
+++ b/tests/ref/fate/source
@@ -28,3 +28,4 @@ libavcodec/bitstream_template.h
 tools/decode_simple.h
 Use of av_clip() where av_clip_uintp2() could be used:
 Use of av_clip() where av_clip_intp2() could be used:
+AVERROR_xxx constants with no associated error string:
-- 
2.45.2

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

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

[Zhao Zhili]

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

Re: [FFmpeg-devel] [PATCH v3 1/3] avutil/utils: Allow "!" in FourCCs

[Paul B Mahol]

On Thu, Jul 18, 2024 at 12:48 PM Andrew Sayers <ffmpeg-devel@pileofstuff.org>
wrote:

> For example, AVERROR_BUG is "BUG!"
> ---
>  libavutil/utils.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavutil/utils.c b/libavutil/utils.c
> index 94d247bbee..a94589e873 100644
> --- a/libavutil/utils.c
> +++ b/libavutil/utils.c
> @@ -81,7 +81,7 @@ char *av_fourcc_make_string(char *buf, uint32_t fourcc)
>          const int print_chr = (c >= '0' && c <= '9') ||
>                                (c >= 'a' && c <= 'z') ||
>                                (c >= 'A' && c <= 'Z') ||
> -                              (c && strchr(". -_", c));
> +                              (c && strchr(". -_!", c));
>          const int len = snprintf(buf, buf_size, print_chr ? "%c" :
> "[%d]", c);
>          if (len < 0)
>              break;
> --
> 2.45.2
>
>
Are FourCC alowed to have "!" ?


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