Page MenuHomePhabricator

Improve transform command compatibility
Closed, ResolvedPublic

Description

In command https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/PdfHandler/+/refs/heads/master/includes/PdfHandler.php#227 we are basically using

/usr/bin/convert -depth 8 -quality 95 -resize 40 - targetImage.jpg

where the - for piped image read-in is placed behind, right before the target image.

The command convert, part of ImageMagick, is deprecated since its version 7, where users are encouraged to port to command magick instead. On Unix-like systems, convert will become a symbolic link to the magick binary, which complains when using the legacy entrance but still recognizes - placed behind.

WARNING: The convert command is deprecated in IMv7, use "magick" instead of "convert" or "magick convert"

However, if one call magick directly as encouraged (for Windows user, this seems to be the only choice), there will be error:

magick: no images found for operation '-resize' at CLI arg 5 @ error/operation.c/CLIOption/5481.

where pictures won't generate correctly. It only accepts the following command instead:

/usr/bin/magick - -depth 8 -quality 95 -resize 40 targetImage.jpg

where the - is placed in the front, right after magick.

Checking the old and the new usage documentation, I noticed that even for the legacy convert command the recommended usage is the latter. That means, if we switch to the latter way of image producing, it would keep maximal compatibility for our external binary dependency.

On this topic I plan to submit a change on Gerrit.


In addition to this change, the hard dependency of PdfHandler to ImageMagick is kind of not so nice, as users have no other choices. For large PDFs with high resolusion (I run into problem with a 4000x9000 one), ImageMagick can be really slow and inefficiency, to an extent that I have to set $wgResponsiveImages = false; to reduce generated thumbnails.

In the long run it would be the best if PdfHandler has the capability like the Transformational Image Handler, where multiple scalers are supported. Support to libvips like already archived in Extension:Thumbro would be even better, as it is know for having superior performance.

Event Timeline

Change #1181121 had a related patch set uploaded (by Huajing; author: Huajing):

[mediawiki/extensions/PdfHandler@REL1_43] Improve transform command compatibility

https://gerrit.wikimedia.org/r/1181121

Change #1255798 had a related patch set uploaded (by Pskyechology; author: Huajing):

[mediawiki/extensions/PdfHandler@master] Improve transform command compatibility

https://gerrit.wikimedia.org/r/1255798

Similar to T419364. Changing the order has no effect on older versions, and makes it work on IM7.

Change #1255807 had a related patch set uploaded (by Pskyechology; author: Huajing):

[mediawiki/extensions/PdfHandler@REL1_45] Improve transform command compatibility

https://gerrit.wikimedia.org/r/1255807

Change #1181121 merged by jenkins-bot:

[mediawiki/extensions/PdfHandler@REL1_43] Improve transform command compatibility

https://gerrit.wikimedia.org/r/1181121

Change #1255807 merged by jenkins-bot:

[mediawiki/extensions/PdfHandler@REL1_45] Improve transform command compatibility

https://gerrit.wikimedia.org/r/1255807

Change #1255798 merged by jenkins-bot:

[mediawiki/extensions/PdfHandler@master] Improve transform command compatibility

https://gerrit.wikimedia.org/r/1255798

Merged into master, so I think this can be considered resolved.