ENH: Add support to save images in WebP format#21274
ENH: Add support to save images in WebP format#21274QuLogic merged 4 commits intomatplotlib:masterfrom kinshukdua:master
Conversation
There was a problem hiding this comment.
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.
|
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 |
|
@tacaswell Yes, I will add a test using ByteIO buffer. |
| 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.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thats the approach I would take, but maybe its more complex than that...
There was a problem hiding this comment.
Maybe we should leave it for a different PR then?
There was a problem hiding this comment.
I guess that depends on whether we think webp is used enough to merit a standalone method.
There was a problem hiding this comment.
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
matplotlib/lib/matplotlib/backend_bases.py
Lines 2236 to 2246 in a698063
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)
|
@kinshukdua Would you be willing to rebase this PR? We prefer to not merge the default branch back into feature branches. |
|
@tacaswell I've rebased the PR, I'm new to git so please let me know if I need to do something else. |
jklymak
left a comment
There was a problem hiding this comment.
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.
|
@tacaswell said that this is what we want, which is fine with me. Thanks for your patience! |
anntzer
left a comment
There was a problem hiding this comment.
Please consider clarifying the situation wrt. dpi. Anyone can dismiss this.
|
Thanks, anyone can merge after CI passes. Congrats on getting your first PR in :) |
|
Thanks @kinshukdua! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again. |
|
Thanks! It was really great contributing back to such an important open source project, I hope to continue contributing. |
ENH: Add support to save 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
pytestpasses).flake8on changed files to check).flake8-docstringsand 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).