-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Added "exact" option when saving WebP #6747
Conversation
This is currently failing on CentOS 7, because it is using libwebp 0.3.0. |
@radarhere Thanks for reviewing the pull request.
Let me know if I'm missing a possibility here. I'd be happy to work on 1, or 2, or even drop this altogether. |
Pillow does distribute Python wheels, but we're building from source in our CI presumably to support users who also build from source, and if you install libwebp-devel on CentOS 7, that's what you get. If you want to suggest that we deprecate support for older versions of libwebp, that's perfectly reasonable. It's just we would probably want to include a deprecation period, and I imagine you'd prefer this feature sooner rather than later. I think using |
Thanks @radarhere for the useful feedback. Turns out #define WEBP_ENCODER_ABI_VERSION 0x020f // MAJOR(8b) + MINOR(8b) The fix is a simple #if WEBP_ENCODER_ABI_VERSION >= 0x0209
// the exact flag is only available in libwebp 0.5.0 and later
config.exact = exact;
#endif I verified that Interestingly the unit test also passes. I think they started removing the transparent part at that exact version, so the tests pass. Let me know if there's anything else I could do. Thanks! |
Looking at https://github.com/webmproject/libwebp/blob/main/NEWS, version 0.5.0 was released in 2015, so I think we can also deprecate it (in another PR). We normally deprecate for at least a year and then drop in a major version. Re: https://github.com/python-pillow/Pillow/milestones If we deprecate now, Pillow 10 in July 2023 is too soon for removal, so Pillow 11 in October 2024 is the next one that can drop it. That's a nice long deprecation period, and CentOS 7 is EOL in June 2024.
Do you mean |
@hugovk , yeah, sorry, #if WEBP_ENCODER_ABI_VERSION >= 0x0209
// the exact flag is only available in libwebp 0.5.0 and later
config.exact = exact;
#endif should be correct. I'm probably not part of the deprecation conversation, but I think if there's no cost in maintaining support, it may be worth keeping around. |
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.
Let's add this API addition to the release notes:
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.
Thank you!
Oh one little change, I went ahead and committed it directly, hope that's okay: removed the code formatting from the title. This follows the Google developer docs style guide and is similar to, for example, |
I think you linked to the wrong part of Google's style guide, since the change you made isn't related to grammar. The part about headings and titles does say to "Avoid using code items in headings when possible.", but I think they mean you should not include code phrases at all, not that code phrases shouldn't be formatted, because the start of the next sentence is "If you must mention a code item in a heading, ..." implying that by following the first sentence you would not mention a code item in a heading. I didn't find anything in their style guide specifically about code formatting in titles, but if you think of this as a list rather than just titles, then there is a reference on the lists page: "If the item is entirely in code font, don't add end punctuation.". So code formatting in list items is allowed. In this case in particular I think "Added the exact encoding option for WebP" is really ambiguous without code formatting. If you don't know that the encoding option is actually called |
Right you are, I've undone that change! |
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
There's so much to learn from y'all. I committed the latest changes. Thanks for the suggestions. |
Thank you for your contribution! |
When encoding a transparent image with WebP, even if specified to be lossless, it'll drop the information that is not visible. This is normally fine, except for applications where you need to undo the mask or make additional changes later.
There's an option in the
WebPConfig
data structure namedexact
:This option is not exposed to the users. In this pull request, I make this option available.
The parameter
exact
is True/False at the python level but translated to0 or 1 (int)
to match that ofWebPConfig
.Changes proposed in this pull request:
src/_webp.c
.src/PIL/WebPImagePlugin.py
.docs/handbook/image-file-formats.rst
.Tests/test_file_webp_alpha.py
.I tried to follow the guidelines as much as possible. Let me know if I'm missing anything.
Thanks.