-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: Add support to save images in WebP format #21274
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
Conversation
All reactions
-
👍 1 reaction
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 for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers
or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Sorry, something went wrong.
All reactions
It is nice how simple this patch is to pick up a new format! Could you add a test that saves an image to ByteIO buffer? That will be enough to exercise the code that you added. Given that this is using the Agg backend we definitely do not need to run it against the full test suite as we would then just be testing pillow. It looks like |
All reactions
Sorry, something went wrong.
@tacaswell Yes, I will add a test using ByteIO buffer. |
All reactions
Sorry, something went wrong.
@@ -594,6 +594,29 @@ def print_tif(self, filename_or_obj, *, pil_kwargs=None): | |||
|
|||
print_tiff = print_tif | |||
|
|||
@_check_savefig_extra_args | |||
def print_webp(self, filename_or_obj, *, pil_kwargs=None): |
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.
Are all these pillow save formats the same? If so, I wonder if we just want a generic print_pillow(format=pillow_format)
rather than adding these one-by-one?
Sorry, something went wrong.
All reactions
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.
As far as I can tell, it is the same for all the fully supported formats- BMP, DDS, DIB, EPS, GIF, ICNS, ICO, IM, JPEG, JPEG 2000, MSP, PCX, PNG, PPM, SGI, SPIDER, TGA, TIFF, WebP, XBM.
I think adding a generic print_pillow
does make a lot of sense especially if we want to support all of these formats. The tests however will have to be created for each format, as they all have their specific options that need to be tested.
Sorry, something went wrong.
All reactions
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.
Thats the approach I would take, but maybe its more complex than that...
Sorry, something went wrong.
All reactions
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.
Maybe we should leave it for a different PR then?
Sorry, something went wrong.
All reactions
-
👍 1 reaction
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.
I guess that depends on whether we think webp
is used enough to merit a standalone method.
Sorry, something went wrong.
All reactions
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.
I suspect the way we would have to do this in general would be some mild-meta-programming as
matplotlib/lib/matplotlib/backend_bases.py
Line 2250 in a698063
print_method = getattr(canvas, 'print_%s' % format) |
matplotlib/lib/matplotlib/backend_bases.py
Lines 2236 to 2246 in a698063
if format is None: | |
# get format from filename, or from backend's default filetype | |
if isinstance(filename, os.PathLike): | |
filename = os.fspath(filename) | |
if isinstance(filename, str): | |
format = os.path.splitext(filename)[1][1:] | |
if format is None or format == '': | |
format = self.get_default_filetype() | |
if isinstance(filename, str): | |
filename = filename.rstrip('.') + '.' + format | |
format = format.lower() |
print_foo
method for every format.
However, we can write ourselves a print function factory function and in backend_agg.py
do something like:
for fmt in LIST_OF_PILLOW_FORMATS:
setattr(BackendAgg, f'print_{fmt}', pillow_printer_factor(fmt)
Sorry, something went wrong.
All reactions
@kinshukdua Would you be willing to rebase this PR? We prefer to not merge the default branch back into feature branches. |
All reactions
Sorry, something went wrong.
@tacaswell I've rebased the PR, I'm new to git so please let me know if I need to do something else. |
All reactions
Sorry, something went wrong.
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.
I think we need to decide whether we want a new webp method, a general "pass to pillow" method or a factory. If in the end, we don't want a new webp method, I dont think we can add this.
I'm not against a webp method if it really is something many folks want, but it seems obscure to me, and maybe just a general method to pillow takes care of the obscure use cases.
I'm blocking until we can sort this out, but if another core dev wants to support a standalone webp method, feel free to dismiss my review.
Sorry, something went wrong.
All reactions
@tacaswell said that this is what we want, which is fine with me. Thanks for your patience! |
All reactions
Sorry, something went wrong.
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.
Please consider clarifying the situation wrt. dpi. Anyone can dismiss this.
Sorry, something went wrong.
All reactions
Thanks, anyone can merge after CI passes. Congrats on getting your first PR in :) |
All reactions
-
❤️ 1 reaction
Sorry, something went wrong.
Thanks @kinshukdua! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again. |
All reactions
-
🎉 1 reaction
Sorry, something went wrong.
Thanks! It was really great contributing back to such an important open source project, I hope to continue contributing. |
All reactions
-
🎉 3 reactions
Sorry, something went wrong.
ENH: Add support to save images in WebP format
tacaswell
jklymak
github-actions[bot]
anntzer
timhoffm
Successfully merging this pull request may close these issues.
[ENH]: saving images in webp format
PR Summary
Fixes #21162
This adds support to save images in WebP format.
This works fine without any problem since Pillow supports WebP and the code is analogous to the support for tiff images added in PR #15193.
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).