Don't read TIFF EXIF properties

Authored by Gilles on Nov 24 2016, 2:30 PM.


Maniphest Tasks
T151454: Thumbor hangs on some TIFF files
rTHMBREXT15270a44784c: Don't read TIFF EXIF properties
Patch without arc
git checkout -b D475 && curl -L | git apply

Refs T151454
They shouldn't affect thumbnailing and are currently causing
occasional exceptions on some files.

Diff Detail

rTHMBREXT Thumbor Plugins
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
Gilles retitled this revision from to Don't read TIFF EXIF properties.
Gilles updated this object.
Gilles edited the test plan for this revision. (Show Details)
Gilles added a reviewer: ori.
ori accepted this revision.Nov 24 2016, 7:16 PM

Oh, I see -- you don't need the EXIF data at all, so you can set the option unconditionally.

In my comment, I said that monkey-patching image.OPTIONS is fine. On reflection, I take that back. It's not hard to imagine a future update to wand using another option and having the code in wand break because you clobber image.OPTIONS.

At minimum, you should add items:

image.OPTIONS |= {'tiff:exif-properties', 'pdf:use-cropbox', 'jpeg:size'}

But ideally don't monkey-patch at all, and use wand.library.MagickSetImageOption instead.

I'm accepting this anyway because it works and it's your show, but -- think about it. :)

This revision is now accepted and ready to land.Nov 24 2016, 7:16 PM

I wasn't aware that you could do that with frozensets, cool. I went with monkey-patching because it made things more consistent with code that uses the options wand has picked as available. I don't get why they did that. I'm aware of calling the library directly, I do that in a couple of places, but I find that a bit uglier and verbose in actual usage.

Gilles added a comment.EditedNov 25 2016, 11:07 AM

As for ignoring the EXIF when resizing, we might have a bad surprise, I'm not sure. With only two test images it's hard to tell, TIFFs can be very varied. But we read and filter the EXIF fields we care about with exiftool separately anyway, which is why I thought it could be tried this way.

Gilles updated this revision to Diff 1258.Nov 25 2016, 11:12 AM

Monkey-patch OPTIONS in a more future-proof manner

This revision was automatically updated to reflect the committed changes.