Conversation
Follows up on python-pillow#5471 and python-pillow#9993. Instead of reporting just "error 6", this PR adds human-readable error messages for the WebP encoder errors. The error messages are based on the WebP header. The memory-hungry `test_write_encoding_error_message` test case (that tested "partition is bigger than 512k") is no longer necessary, since `test_write_encoding_error_bad_dimension` covers the same code path.
| #define WEBP_STR_HELPER(x) #x | ||
| #define WEBP_STR(x) WEBP_STR_HELPER(x) | ||
|
|
||
| static const char *const kEncoderErrorMessages[VP8_ENC_ERROR_LAST] = { |
There was a problem hiding this comment.
|
I'm reluctant to remove the test, since it is guarding against regression for the specific case of #5461. |
| "out of memory re-allocating byte buffer", | ||
| "NULL parameter passed to function", | ||
| "configuration is invalid", | ||
| "image size exceeds WebP limit of " WEBP_STR(WEBP_MAX_DIMENSION) " pixels", |
There was a problem hiding this comment.
What is the story with WEBP_STR and WEBP_STR_HELPER? Why do they need to be defined separately, and why are there two of them?
There was a problem hiding this comment.
That's just C preprocessor expansion rules and standard procedure:
a.c
#define WEBP_MAX_DIMENSION 123
// Doesn't work like we want
#define WEBP_STR_1(x) #x
WEBP_STR_1(WEBP_MAX_DIMENSION)
// Works like we want
#define WEBP_STR_HELPER(x) #x
#define WEBP_STR_2(x) WEBP_STR_HELPER(x)
WEBP_STR_2(WEBP_MAX_DIMENSION)
Preprocessing:
$ gcc -E a.c
# 1 "a.c"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 466 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "a.c" 2
"WEBP_MAX_DIMENSION"
"123"
This shows up in Pillow e.g. here (#4700):
Lines 4372 to 4374 in 4b03987
| PyErr_Format(PyExc_ValueError, "encoding error %d%s", error_code, message); | ||
| free(output); | ||
| return NULL; | ||
| return HandleEncoderError(pic.error_code); |
There was a problem hiding this comment.
Why does HandleEncoderError need to be a separate function?
There was a problem hiding this comment.
It doesn't, but it follows the form of HandleMuxError, above. Would you prefer it inlined?
AISI, the issue there is the WebP encoder not having reported even an error code? As noted in the PR description, with this PR that user will get "encoding error 6: partition #0 is bigger than 512K" (instead of "encoding error 6" as of #5471). Since the remaining test covers the entire error handling code, IMO this PR covers that case too but without having to allocate a large image the WebP encoder can't fit in partition #0 (whatever that means 😅). I'm having a hard time seeing the way this'd regress. Someone would need to explicitly change the error handling in the WebP encoder for worse? EDIT: #9936 will enable the knob to tweak |
Follows up on #5471 and #9993.
Instead of reporting just "error 6", this PR adds human-readable error messages for the WebP encoder errors. The error messages are based on the WebP header.
The memory-hungry (675 MB allocated for test image)
test_write_encoding_error_messagetest case (that tested "partition is bigger than 512k") is no longer necessary, sincetest_write_encoding_error_bad_dimensioncovers the same code path.That error message is improved too, though: