Image optimisation operations #69
Conversation
|
This looks good so far, thanks! I think it would be really nice if this could be fit in to an This has the advantage of being able to bypass Pillow/Wand if the image to be optimised is already saved as a JPEG file. But the example above suffers from some issues such as not being able to select output format or filename, and leaves choosing an optimiser up to Willow's routing (which might not be adequate for this use case). I'm happy for this to be done differently if it can't be fit into Image classes, so please just take this as some food for thought! |
|
That is exactly the type of feedback I was looking for. Thanks Karl. Will let it simmer for a day or two and come back it fresh |
zerolab
changed the title
willow/image.py
Outdated
| return | ||
|
|
||
| has_named_file = False | ||
| if isinstance(image_file, SpooledTemporaryFile): |
There was a problem hiding this comment.
The logic for this method is: create a NamedTemporaryFile if needed, optimize in place, and replace the passed image_file. If image_file is a string, we assume a path and try to optimize that directly
note: this is probably the most controversial bit.
Latest Wagtail passes on a SpooledTemporaryFile as of wagtail/wagtail#10284.
Given willow is used in non-Wagtail contexts, I wanted to support: BytesIO (which is what Wagtail passed prior to wagtail/wagtail#10284), NamedTemporaryFile, + string and bytes
The reason for string is that we want to support file paths (#108). I am unsure about bytes, though.
There was a problem hiding this comment.
This process is a little different to what I was expecting, but not too different. I suppose always converting to NamedTemporaryFile makes sense, as I suspect more optimization packages support file paths as inputs vs raw bytes (plus, it keeps the optimizer design simple). We can also avoiding loading full file contents into memory if we're careful (which it looks like we are being).
|
|
||
| WILLOW_OPTIMIZERS = "jpegoptim,optipng" | ||
| # or as a list of optimizer library names | ||
| WILLOW_OPTIMIZERS = ["jpegoptim", "optipng"] |
willow/image.py
Outdated
| return | ||
|
|
||
| has_named_file = False | ||
| if isinstance(image_file, SpooledTemporaryFile): |
There was a problem hiding this comment.
This process is a little different to what I was expecting, but not too different. I suppose always converting to NamedTemporaryFile makes sense, as I suspect more optimization packages support file paths as inputs vs raw bytes (plus, it keeps the optimizer design simple). We can also avoiding loading full file contents into memory if we're careful (which it looks like we are being).
willow/image.py
Outdated
| if hasattr(image_file, "seek"): | ||
| # rewind and replace the image file with the optimized version | ||
| image_file.seek(0) | ||
| with open(file_path, "rb") as f: | ||
| image_file.write(f.read()) | ||
|
|
||
| if hasattr(image_file, "truncate"): | ||
| image_file.truncate() # bring the file size down to the actual image size |
willow/optimizers/cwebp.py
Outdated
| return [ | ||
| "-q 80", # compression factor. 100 produces the highest quality. | ||
| "-m 6", # inspect all encoding possibilities for best file size | ||
| "-pass 10", # max number of passes | ||
| "-mt", # use multithreading if possible | ||
| file_path, | ||
| f"-o {file_path}", | ||
| ] |
There was a problem hiding this comment.
Suggestion: It would be nice if some of these defaults could be configured via settings/env vars. Would make tweaking much easier and possibly prevent a bit of "Why is this the default? I have a different opinion! Argh!" argy-bargy.
There was a problem hiding this comment.
I think the most we could offer is the quality setting, but I'd rather we do this once people get noisy about it :D incremental tweaks and so on
| def test_optimize_with_spooled_temporary_file( | ||
| self, mock_unlink, mock_named_temporary_file, mock_process | ||
| ): |
There was a problem hiding this comment.
The argument order here appears not to match the order of the patches.
There was a problem hiding this comment.
That is correct. You can think of
@a
@b
def func():
...as a(b(func()) so b passes its param first, then a. in our case unlink, then named temp file
if `WILLOW_OPTIMIZERS` is not set or is set to a false-y value, nothing gets registered When set to a true-ish value -> all valid optimizers will be registered. When set to a list or a comma-separated string of binary names, only valid optimizers from the list will be registered Co-Authored-By: Arnar Tumi Þorsteinsson <[email protected]>
willow/image.py
Outdated
| image_file.truncate() # bring the file size down to the actual image size | ||
|
|
||
| if named_file_created: | ||
| os.unlink(file_path) |
There was a problem hiding this comment.
A bit delayed on this one (sorry!), it's important this happen regardless of what issues might occur following temp file creation. So, I would recommend wrapping the above lines in a try:, and these couple in a finally:.
There was a problem hiding this comment.
good call. at first I wanted it to fail loudly, but then again that is probably not a good idea for auxiliary operations. done
tests/test_optimizers.py
Outdated
| with open(image_file, "rb") as f: | ||
| self.assertEqual(self.optimized_size, os.fstat(f.fileno()).st_size) | ||
|
|
||
| os.unlink(image_file) |
There was a problem hiding this comment.
The try/finally approach might be a good call in these tests too
@kaedroho this is a first pass at using image optimisation binaries.
Opening fairly early to get initial feedback.
On my To-Do list:
Future To-Do: