-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Improve WebP encoder error handling #10039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -73,6 +73,36 @@ HandleMuxError(WebPMuxError err, char *chunk) { | |||||||
| return NULL; | ||||||||
| } | ||||||||
|
|
||||||||
| /* -------------------------------------------------------------------- */ | ||||||||
| /* WebP Encoder Error Handling */ | ||||||||
| /* -------------------------------------------------------------------- */ | ||||||||
|
|
||||||||
| #define WEBP_STR_HELPER(x) #x | ||||||||
| #define WEBP_STR(x) WEBP_STR_HELPER(x) | ||||||||
|
|
||||||||
| static const char *const kEncoderErrorMessages[VP8_ENC_ERROR_LAST] = { | ||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||
| "ok", | ||||||||
| "out of memory allocating objects", | ||||||||
| "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", | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the story with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's just C preprocessor expansion rules and standard procedure: a.cPreprocessing:This shows up in Pillow e.g. here (#4700): Lines 4372 to 4374 in 4b03987
|
||||||||
| "partition #0 is bigger than 512K", | ||||||||
| "partition is bigger than 16M", | ||||||||
| "picture writer returned an I/O error", | ||||||||
| "file would be bigger than 4G", | ||||||||
| "encoding aborted by user" | ||||||||
| }; | ||||||||
|
|
||||||||
| static PyObject * | ||||||||
| HandleEncoderError(WebPEncodingError error_code) { | ||||||||
| const char *message = error_code > VP8_ENC_OK && error_code < VP8_ENC_ERROR_LAST | ||||||||
| ? kEncoderErrorMessages[error_code] | ||||||||
| : "unknown error"; | ||||||||
| PyErr_Format(PyExc_ValueError, "encoding error %d: %s", error_code, message); | ||||||||
| return NULL; | ||||||||
| } | ||||||||
|
|
||||||||
| /* -------------------------------------------------------------------- */ | ||||||||
| /* Frame import */ | ||||||||
| /* -------------------------------------------------------------------- */ | ||||||||
|
|
@@ -647,18 +677,8 @@ WebPEncode_wrapper(PyObject *self, PyObject *args) { | |||||||
| ret_size = writer.size; | ||||||||
|
|
||||||||
| if (!ok) { | ||||||||
| int error_code = (&pic)->error_code; | ||||||||
| char message[50] = ""; | ||||||||
| if (error_code == VP8_ENC_ERROR_BAD_DIMENSION) { | ||||||||
| sprintf( | ||||||||
| message, | ||||||||
| ": Image size exceeds WebP limit of %d pixels", | ||||||||
| WEBP_MAX_DIMENSION | ||||||||
| ); | ||||||||
| } | ||||||||
| PyErr_Format(PyExc_ValueError, "encoding error %d%s", error_code, message); | ||||||||
| free(output); | ||||||||
| return NULL; | ||||||||
| return HandleEncoderError(pic.error_code); | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't, but it follows the form of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing that out. It could go either way, but my preference would be to inline it, since |
||||||||
| } | ||||||||
|
|
||||||||
| { | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#8322 was already doing this for one error, so could we include the word 'consistently' in here?