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(engines/pil.read_multiple): Use Pillow to save multipage images as GIF #1643

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

scorphus
Copy link
Member

@scorphus scorphus commented Jan 23, 2024

The functionality provided by the images2gif module is part of Pillow since 2015. It also renders gifsicle unnecessary in this context.

…as GIF

The functionality provided by the images2gif module is part of Pillow
since 2015 [1]. It also renders `gifsicle` unnecessary in this context.

  1. https://pillow.readthedocs.io/en/stable/releasenotes/3.0.0.html
@scorphus
Copy link
Member Author

With this change, we're free to remove the version constraints for Pillow:

thumbor/setup.py

Lines 153 to 154 in 812ed4c

# TODO: Pillow version 10.1.0 is raising a PIL.Image.DecompressionBombError on tests
"Pillow==10.*,<10.1.0",

@coveralls
Copy link

coveralls commented Jan 23, 2024

Pull Request Test Coverage Report for Build 7762176489

  • 0 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+2.3%) to 88.927%

Totals Coverage Status
Change from base Build 7748021860: 2.3%
Covered Lines: 3879
Relevant Lines: 4362

💛 - Coveralls

@scorphus scorphus force-pushed the images2gif branch 3 times, most recently from a6177fe to e197a0c Compare January 23, 2024 23:09
@scorphus scorphus marked this pull request as draft January 23, 2024 23:11
@scorphus scorphus marked this pull request as ready for review January 25, 2024 10:40
@scorphus scorphus force-pushed the images2gif branch 3 times, most recently from c70db60 to 8c1824d Compare January 27, 2024 22:06
@scorphus
Copy link
Member Author

Please, check #1644 before commenting on lint failure.

thumbor/engines/pil.py Outdated Show resolved Hide resolved
@scorphus
Copy link
Member Author

Thanks for your review, @guilhermef.

While doing some benchmarks of those changes and seeing a small but welcomed performance improvement of around 5%, I encountered cases where the gifsicle sub process hangs indefinitely.

I already have a draft solution to that which I'll probably propose in a separate PR – as they're apparently unrelated issues.

However, merging this as it is forbids us from drafting a new release. So, let's wait until we have both changes ready.

#trickyone

thumbor/engines/pil.py Outdated Show resolved Hide resolved
thumbor/engines/pil.py Outdated Show resolved Hide resolved
@scorphus scorphus force-pushed the images2gif branch 3 times, most recently from 6e1b47f to 378ab80 Compare January 31, 2024 16:06
@scorphus
Copy link
Member Author

scorphus commented Jan 31, 2024

There we go, @guilhermef. I believe we're much better off having no gifsicle in pil.read_multiple. I'd appreciate another round of review.

Copy link

sonarcloud bot commented Feb 2, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@scorphus scorphus merged commit 1431995 into thumbor:master Feb 2, 2024
23 checks passed
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.

None yet

3 participants