Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode 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. |
| @@ -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.
Areall these pillow save formats the same? If so, I wonder if we just want a genericprint_pillow(format=pillow_format) rather than adding these one-by-one?
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 genericprint_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.
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...
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?
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 thinkwebp is used enough to merit a standalone method.
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 ina698063
| print_method=getattr(canvas,'print_%s'%format) |
matplotlib/lib/matplotlib/backend_bases.py
Lines 2236 to 2246 ina698063
| ifformatisNone: | |
| # get format from filename, or from backend's default filetype | |
| ifisinstance(filename,os.PathLike): | |
| filename=os.fspath(filename) | |
| ifisinstance(filename,str): | |
| format=os.path.splitext(filename)[1][1:] | |
| ifformatisNoneorformat=='': | |
| format=self.get_default_filetype() | |
| ifisinstance(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 inbackend_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. |
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.
@tacaswell said that this is what we want, which is fine with me. Thanks for your patience! |
Uh oh!
There was an error while loading.Please reload this page.
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.
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).