Skip to content
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

feat: auto-rotate thumbnails based on metadata #366

Merged
merged 7 commits into from
Oct 9, 2022

Conversation

leon-richardt
Copy link
Contributor

@leon-richardt leon-richardt commented Oct 3, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

This PR fixes #336.

libvips is a powerful image processing library in C and govips provides Go bindings for most of its functions. #312 intends to replace lilliput with govips and the library looks promising to be useful when attacking other issues as well (#259, #347, and #363, to name a few), I decided implement this feature with it as well.
Thus, this PR can be seen as a first foray into using govips with a relatively simple and self-contained feature. This also means we can drop the nfnt/resize dependency.
Other than what @KararTY reported in #312, the libvips and libvips-dev packages in the Ubuntu 22.04 default repositories were sufficient for me to build this branch.

Since I have no way to way to test or develop this on Windows, this feature is limited to Linux. (But this is where I suspect the vast majority of usage to happen anyhow.)

Future Opportunities

As described above, I believe libvips/govips to be helpful in attacking multiple current and future issues. Most notably, if libvips/govips proves viable, #312 may allow us to serve animated thumbnails at a manageable memory cost and replace lilliput altogether.

Testing

When testing on different branches with the same link (e.g. https://files.catbox.moe/8c5ref.jpeg, courtesy of #336), make sure to either clear your cache or reupload the image elsewhere to circumvent caching.

@leon-richardt leon-richardt force-pushed the feat/autorotate-thumbnails branch from 9c90543 to e40637e Compare October 3, 2022 22:26
@KararTY
Copy link
Contributor

KararTY commented Oct 5, 2022

Other than what @KararTY reported in #312, the libvips and libvips-dev packages in the Ubuntu 22.04 default repositories were sufficient for me to build this branch.

For future reference, I was using Ubuntu 20.04 which had/has an older version of libvips. Any distro with libvips 8.12+ is good as older libvips versions are not as performant. Distros with libvips lower than 8.12 would need to build from source.

Older versions are just as problematic as lilliput for generating animated thumbnails because they use imagemagick: https://www.libvips.org/2021/11/14/What's-new-in-8.12.html

@leon-richardt leon-richardt force-pushed the feat/autorotate-thumbnails branch 9 times, most recently from 98db221 to 6304d88 Compare October 6, 2022 12:36
@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Merging #366 (36d7955) into master (effcbb9) will decrease coverage by 0.04%.
The diff coverage is 2.32%.

❗ Current head 36d7955 differs from pull request most recent head 5c4aca7. Consider uploading reports for the commit 5c4aca7 to get more accurate results

@@            Coverage Diff             @@
##           master     #366      +/-   ##
==========================================
- Coverage   48.91%   48.87%   -0.05%     
==========================================
  Files          98       98              
  Lines        3146     3159      +13     
==========================================
+ Hits         1539     1544       +5     
- Misses       1565     1573       +8     
  Partials       42       42              
Impacted Files Coverage Δ
cmd/api/main.go 0.00% <0.00%> (ø)
internal/resolvers/default/link_loader.go 55.95% <0.00%> (ø)
pkg/thumbnail/thumbnail.go 0.00% <0.00%> (ø)
pkg/thumbnail/thumbnail_unix.go 0.00% <0.00%> (ø)
internal/resolvers/default/thumbnail_loader.go 34.42% <20.00%> (+7.30%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@leon-richardt leon-richardt force-pushed the feat/autorotate-thumbnails branch from 36d7955 to cc4a28e Compare October 6, 2022 13:56
For file formats and metadata that support it (e.g. EXIF), thumbnails
will be automatically oriented upright with this commit.
This commit ensures that we only try to build animated thumbnails with
lilliput for image formats that support animation. In the other cases,
we immediately build the thumbnail using govips.
This name is more inline with `thumbnail.IsAnimatedThumbnailType`,
introduced in 0e256b1.
Courtesy of @Leppunen.

This was the most time-efficient way to make sure both lilliput
and govips can be used in the project since libivps >=8.12 is only
available since Ubuntu 22.04.
libvips 8.12 is only available since Ubuntu 22.04 so the workflow
runners are bumped to that version. However, this shouldn't be much of a
problem as GitHub plans to update ubuntu-latest to 22.04 later this year:
actions/runner-images#5998 (comment)
@leon-richardt leon-richardt force-pushed the feat/autorotate-thumbnails branch from cc4a28e to 5c4aca7 Compare October 6, 2022 14:01
@leon-richardt
Copy link
Contributor Author

Two points:

  1. So, this PR turned out to change a little more than I originally intended (namely, updating the Actions workflow to run on Ubuntu 22.04 and changing the Docker image).
    1. Regarding Ubuntu 22.04: GitHub announced they are planning to bump the ubuntu-latest image to 22.04 by the end of this year. So we would just be a little bit ahead of the curve.
    2. Regarding the Docker image (we discussed this in chat): The change from Alpine is only required in order to be able to run lilliput and govips at the same time. Since the overall intent is to replace lilliput eventually, we can switch back to an Alpine-based image once that migration is done.
  2. AFAICT, adding a test case for a successful thumbnail resolver response would require adding an actual file to generate a thumbnail from. I did not feel comfortable doing this as there were no examples to go off on yet. If desired, we can work towards such a test case anyway but I would like some guidance on how this should work.

@leon-richardt
Copy link
Contributor Author

leon-richardt commented Oct 6, 2022

❗ Current head 36d7955 differs from pull request most recent head 5c4aca7. Consider uploading reports for the commit 5c4aca7 to get more accurate results

I'm not sure how do that, I suppose codecov didn't re-run because we hit some quota limit.

@leon-richardt leon-richardt marked this pull request as ready for review October 6, 2022 15:00
@leon-richardt
Copy link
Contributor Author

leon-richardt commented Oct 6, 2022

Special thanks to @KararTY and @Leppunen for making it happen!

@pajlada pajlada merged commit bca86b5 into Chatterino:master Oct 9, 2022
@leon-richardt leon-richardt deleted the feat/autorotate-thumbnails branch October 9, 2022 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image thumbnails are rotated sometimes
3 participants