Page MenuHomePhabricator

WebP output support
AcceptedPublic

Authored by Gilles on Mar 29 2018, 7:08 AM.

Details

Maniphest Tasks
T27611: Support optimized WebP thumbnails as alternative to JPEG, PNG
Reviewers
fgiunchedi
Imarlier
Commits
rTHMBREXTbd557b6c12c1: WebP output support
Patch without arc
git checkout -b D1016 && curl -L https://phabricator.wikimedia.org/D1016?download=true | git apply
Summary

Refs T27611

Diff Detail

Repository
rTHMBREXT Thumbor Plugins
Branch
T27611 (branched from master)
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 2903
Build 4844: differential-jessieJenkins
Build 4843: arc lint + arc unit

Event Timeline

Gilles created this revision.Mar 29 2018, 7:08 AM
Harbormaster completed remote builds in B2870: Diff 2666.
Gilles requested review of this revision.Mar 29 2018, 7:09 AM
Gilles updated this revision to Diff 2667.Mar 29 2018, 7:33 AM

Adding Swift storage fix

Gilles updated this revision to Diff 2672.Apr 3 2018, 9:42 AM

Rebase

Gilles updated this revision to Diff 2677.Apr 3 2018, 8:27 PM
  • Add comprehensive test coverage
  • Handle conversion in separate cwebp call, to get access to options the IM delegate can't use
  • Add support for lossless WebP, as a substitute for PNGs
fgiunchedi added inline comments.Apr 4 2018, 9:38 AM
wikimedia_thumbor/engine/imagemagick/imagemagick.py
461

Upstream might be interested in adding such a mode (output to stdout), if possible?

466–498

It seems to me run_operators is convert-specific, meaning I'm expecting it to only call/interact with convert. I think having webp conversion in a separate function would make the code more clear and easier to test, unless there's something I'm missing?

wikimedia_thumbor/engine/stl/stl.py
56

This change doesn't seem related to webp?

wikimedia_thumbor/result_storage/swift/swift.py
87

Ditto, not related to this change?

fgiunchedi added inline comments.Apr 4 2018, 9:40 AM
wikimedia_thumbor/engine/imagemagick/imagemagick.py
461

Now that I think of it, does using /dev/stdout as output file work?

Gilles updated this revision to Diff 2680.Apr 4 2018, 1:21 PM
  • Increase test coverage
  • Fix EXIF filtering by using JPEG with quality 100 as intermediary
Gilles added inline comments.Apr 4 2018, 1:26 PM
wikimedia_thumbor/engine/imagemagick/imagemagick.py
461

The issue is with the dumb IM delegate, which is extremely limited in what can be passed to the cwebp command. To have all webp parameters available to IM, we could need to compile IM with WebP support (the Debian version doesn't have it).

The actual cwebp command does support output to stdout, which is what I'm doing in the latest version.

466–498

Yeah I'm going to refactor things in my next pass, I was waiting to have all the parts working. The way webp supported is injected right now doesn't please me.

wikimedia_thumbor/engine/stl/stl.py
56

It's actually related, one of the WebP output files in the tests hits that execution path when its mime gets sniffed by this engine.

wikimedia_thumbor/result_storage/swift/swift.py
87

It is related. Without storing the content-type explicitly, when reading the object back Swift can't figure out that it's webp, and will serve it as octet stream, making the browser download the image instead of displaying it.

Gilles added inline comments.Apr 4 2018, 1:28 PM
wikimedia_thumbor/engine/imagemagick/imagemagick.py
461

And in the end as you'll see below, I need access to a whole bunch of parameters on the cwebp command, not just the ability to output to stdout.

Without calling the cwebp command directly, I wouldn't be able to use lossless mode, to keep the metadata, etc.

Gilles edited the summary of this revision. (Show Details)Apr 4 2018, 1:30 PM
Gilles retitled this revision from Add ability to output WebP to WebP output support.
Gilles updated this revision to Diff 2698.Apr 11 2018, 11:44 AM
  • Split tests into smaller focused files
  • Make EXIF tests only about EXIF
  • Add WebP output test coverage for every test outputting JPG or PNG
  • Switch reference visual thumbnail from MediaWiki output to perfect lossless reference thumbnail
  • Make test size assertions explicit instead of derived from reference thumbnails
  • Shrink with VIPS to a larger size, to avoid height rounding errors
  • Make jpeg:size hint target a larger size, to avoid height rounding errors
  • Make rounding strategy explicit to avoid python's default rounding to the nearest odd number on halves
  • Make IM resize parameter more strict to avoid off-by-one-pixel widths and ensure that the requested width is always respected
  • Remove dead code for reading/writing buffer data, which was used for an older version of the conditional sharpen filter
Imarlier added inline comments.Apr 13 2018, 12:14 PM
tests/integration/__init__.py
161

Where does this file path come from? Another test?

tests/integration/test_exif.py
57

.format() is preferred at this point. (Best to be consistent, though, if there's any string formatting elsewhere that uses the old style.)

wikimedia_thumbor/engine/imagemagick/imagemagick.py
98

I'm guessing there's a check elsewhere that errors in case both are zero?

Gilles updated this revision to Diff 2699.Apr 16 2018, 9:29 AM
Gilles marked 3 inline comments as done.
  • Remove leftover debugging file
  • Add tests for when no dimension is specified
Gilles added inline comments.Apr 16 2018, 9:30 AM
tests/integration/__init__.py
161

That's just something that made troubleshooting easier during the refactoring, I'll get rid of it.

tests/integration/test_exif.py
57

I've used that style consistently (unaware that it's "old") in thumbor plugins code, because it was the most common style seen in Thumbor itself (64 hits vs 18 for format).

Thumbor tends to be oldschool as I believe it's not python3-compatible yet.

wikimedia_thumbor/engine/imagemagick/imagemagick.py
98

This can't happen because hitting resize(), which then calls this, implies that at least one target dimension is specified.

I'll add a test case for when no dimension is defined (no resize) for completion's sake, even though this can't happen at all in the Wikimedia setup.

fgiunchedi accepted this revision.Apr 18 2018, 8:55 AM

Nit inline, the rest LGTM

wikimedia_thumbor/engine/imagemagick/imagemagick.py
252

quality is a string in line 263, I'd say it is more intuitive to have it as an int like you did here, either works tho

This revision is now accepted and ready to land.Apr 18 2018, 8:55 AM