Page MenuHomePhabricator

Return any thumbnail ImageMagick managed to render
ClosedPublic

Authored by Gilles on Mar 14 2018, 8:59 AM.

Details

Maniphest Tasks
T179200: Some half-broken PNGs cannot be rendered with the Jessie IM version
Reviewers
fgiunchedi
Commits
rTHMBREXTf321adbfa13d: Return any thumbnail ImageMagick managed to render
Patch without arc
git checkout -b D1008 && curl -L https://phabricator.wikimedia.org/D1008?download=true | git apply
Summary

Even if the exit code is non-zero

Refs T179200

Diff Detail

Repository
rTHMBREXT Thumbor Plugins
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Gilles created this revision.Mar 14 2018, 8:59 AM

@fgiunchedi same here, busted CI

fgiunchedi added inline comments.Mar 14 2018, 6:07 PM
wikimedia_thumbor/engine/imagemagick/imagemagick.py
291

I think we should be validating that the output is actually a valid image (FSVO of valid, e.g. loadable? displayable?). The case I have in mind is where IM exits non-zero and with garbage in its output.

Also this sounds like a bug (or a feature request) for IM upstream, namely "partially rendered file" ? with a specific exit code.

Gilles added inline comments.Mar 15 2018, 9:03 AM
wikimedia_thumbor/engine/imagemagick/imagemagick.py
291

Actually the file it generates is fully rendered and valid, it's just whining about the original.

I'm not sure how we would verify that the output is valid, exiftool is likely to be very permissive, looking only at the headers. If we do a sort of no-op conversion with IM itself, maybe? When I did that locally on the resulting thumbnail, the error was gone, meaning that the thumbnail didn't inherit the issue of the original.

We could do that check only in case of output and non-zero exit code.

Gilles updated this revision to Diff 2645.Mar 15 2018, 12:13 PM

Check validity of generated thumbnail when convert exit code was non-zero

Build has FAILED

Test Name
tox -vBuild Details

tox -v log

Got exit code 1 from PY_COLORS=1 tox -v
2.5.0 imported from /usr/local/lib/python2.7/dist-packages/tox/__init__.pyc

  /srv/jenkins-workspace/workspace/phabricator-jessie-diffs$ /srv/jenkins-workspace/workspace/phabricator-jessie-diffs/.tox/py27/bin/pip install -r/srv/jenkins-workspace/workspace/phabricator-jessie-diffs/requirements.txt >/srv/jenkins-workspace/workspace/phabricator-jessie-diffs/.tox/py27/log/py27-1.log
  /srv/jenkins-workspace/workspace/phabricator-jessie-diffs$ /srv/jenkins-workspace/workspace/phabricator-jessie-diffs/.tox/py27/bin/pip freeze >/srv/jenkins-workspace/workspace/phabricator-jessie-diffs/.tox/py27/log/py27-2.log
py27 installed: backports-abc==0.5,certifi==2018.1.18,chardet==3.0.4,derpconf==0.8.2,futures==3.2.0,idna==2.6,libthumbor==1.3.2,olefile==0.45.1


Link to build: https://integration.wikimedia.org/ci/job/phabricator-jessie-diffs/1034/
See console output for more information: https://integration.wikimedia.org/ci/job/phabricator-jessie-diffs/1034/console

Build has FAILED

Test Name
tox -vBuild Details

tox -v log

Got exit code 1 from PY_COLORS=1 tox -v

flake8 installed: configparser==3.5.0,enum34==1.1.6,flake8==3.5.0,mccabe==0.6.1,pycodestyle==2.3.1,pyflakes==1.6.0
flake8 runtests: PYTHONHASHSEED='2037326284'
flake8 runtests: commands[0] | flake8
  /srv/jenkins-workspace/workspace/phabricator-jessie-diffs$ /srv/jenkins-workspace/workspace/phabricator-jessie-diffs/.tox/flake8/bin/flake8 
./wikimedia_thumbor/loader/video/__init__.py:86:17: E201 whitespace after '['
./wikimedia_thumbor/loader/video/__init__.py:86:39: E202 whitespace before ']'
ERROR: InvocationError: '/srv/jenkins-workspace/workspace/phabricator-jessie-diffs/.tox/flake8/bin/flake8'
py27 create: /srv/jenkins-workspace/workspace/phabricator-jessie-diffs/.tox/py27
  /srv/jenkins-workspace/works


Link to build: https://integration.wikimedia.org/ci/job/phabricator-jessie-diffs/1035/
See console output for more information: https://integration.wikimedia.org/ci/job/phabricator-jessie-diffs/1035/console
Gilles updated this revision to Diff 2649.Mar 22 2018, 8:28 AM

Rebasing

Gilles requested review of this revision.Mar 22 2018, 8:28 AM
fgiunchedi added inline comments.Mar 22 2018, 10:55 AM
wikimedia_thumbor/engine/imagemagick/imagemagick.py
291

Yeah noop pass of IM when exit code is non-zero and there's output sounds like a step in the right direction!

Gilles marked an inline comment as done.Mar 22 2018, 1:08 PM
Gilles added inline comments.
wikimedia_thumbor/engine/imagemagick/imagemagick.py
291

It's already implemented in this latest version

fgiunchedi accepted this revision.Mar 26 2018, 1:20 PM
This revision is now accepted and ready to land.Mar 26 2018, 1:20 PM
This revision was automatically updated to reflect the committed changes.