-
Notifications
You must be signed in to change notification settings - Fork 50
Improve the thumbnail service to support compression and WEBP #630
Conversation
This is such a great find Dhruv! I'm excited to review this tomorrow 🙂 |
Really exciting and a significant change. Can't wait to investigate this more. |
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.
This looks great! It sees quite fast too, which is rad. There aren't any pngs in the sample data to test with, but the JPEGs all look good 🙂 I have a few comments about logging
I'll test this soon, just want to note that we make sure to manually visually diff a few thumbnails against their current, production counterparts, to check for noteworthy quality differences or any other issues. The new library looks great! It would be an excellent enhancement to figure out how to serve webp images in the future. |
@zackkrida the |
Some more stats:
With compression and WEBP, we can reduce the size even more than the already compressed thumbnails. |
if "webp" in accept_header: | ||
params["type"] = "auto" # Use ``Accept`` header to determine output type. |
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.
This converts JPG and PNG to WEBP if the Accept
header contains it. Otherwise the format stays as it is. This is to prevent issues with Safari which before Big Sur, preferentially requests PNG. JPG images become quite large if converted to PNG.
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.
Looks really good! Just a few things to clean up/short-cut requests. This is really awesome though.
Co-authored-by: sarayourfriend <[email protected]>
Co-authored-by: sarayourfriend <[email protected]>
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.
LGTM! Tested locally and it works great.
Along with the full_size
test, can you add unit tests for the serializer and the validate
method specifically?
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.
LGTM, just curious about the release issue, whether we'll need to build an image ourselves from the latest code?
Just noting that the process of building and hosting a custom image is being worked out in a separate PR, #641. |
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.
Thanks for making those logging changes! I hope you get a response soon on the build issue, but if nothing else it's good to know we have another option.
* Define a workflow to build Docker image for imaginary * Use custom imaginary image in Docker Compose
Fixes
Fixes #531 by @zackkrida
Description
This PR replaces willnorris/imageproxy with h2non/imaginary, a faster, more feature-rich and more highly-rated library for image manipulations over HTTP. This library allows us to compress thumbnails and reduce bytes.
Now, we can control the dimensions of the thumbnail and the amount of compression using env vars (while also allowing the URL query params
full_size
andcompressed
override the defaults if needed).To spark joy, this PR adds API documentation for the thumbnail endpoints (both audio and video) with all query params documented.
Testing Instructions
full_size
andcompressed
query params.full_size
set to'true'
, the image should be larger.full_size
set to'false'
, the image will be smaller.full_size
, but can be overridden with thecompressed
param.Stats
Here are some randomly sampled cases.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin