Fall back to ImageMagick command line when the pecl imagic is not available on the server

[Hristo Sg]

The patch allows WordPress to fall back to the ImageMagick command line when the imagic pecl is not available on the server. Patch attached.

[Hristo Sg]

The patch for falling back to command line

[Hristo Sg]

Here are the performance test we've done so far:

CPU - Total number of CPU-seconds that the process used directly (in user mode), in seconds.
MaxRSS - Maximum resident set size of the process during its lifetime, in Kilobytes.

PHP 7.0 - no imagick loaded - MaxRSS: 93261 CPU user: 0.0204
PHP 7.0 - imagick loaded - MaxRSS: 118094 CPU user: 0.0742
PHP 7.1 - no imagick loaded - MaxRSS: 50444 CPU user: 0.0144
PHP 7.1 - imagick loaded: MaxRSS: 82350 CPU user: 0.0678
Curl requests to phpinfo comparison (1 request per second - 5 min):
PHP 7.0 (no imagick)- Average response time: 0.071818 ms |Number of requests: 300
PHP 7.0 (with imagick) - Average response time: 0.143573 ms |Number of requests: 300

Performed tests and added results for PHP 5.6
PHP 5.6 - no imagick loaded - MaxRSS: 60400 CPU user: 0.019
PHP 5.6 - imagick loaded - MaxRSS: 85870 CPU user: 0.080
Curl requests to phpinfo comparison (1 request per second - 5 min):
PHP 5.6 (no imagick)- Average response time: 0.059901 ms |Number of requests: 300
PHP 5.6 (with imagick) - Average response time: 0.134315 ms |Number of requests: 300

[dd32]

Just a note that i've removed the contents of the diff from the ticket description to make it easier to read.

Hi and welcome back to Trac.

Just to note that there was some discussion related to the usage of the external commands in 33:ticket:6821 of the original ticket.
My concerns still remain from that ticket - in that I don't think WordPress should be shelling out to perform commands.

Quick look at the patch points to issue in update_size() - not escaping the width/height variables, and shell_exec() doesn't throw exceptions.

I think this would be great as a plugin myself.

[danielkanchev]

@dd32 thanks for the quick reply and the feedback.

Regarding the update_size() function, the width and height are not variables in this case - this is how you call the identify binary in order to get the dimensions:

/usr/bin/identify -format '%[width]  %[height]' wp-content/uploads/2008/08/noimage-600x600.png
600  600

That is why these two should not be escaped in the code.

Regarding the shell_exec() exception I do agree that the exception handling code should be removed because shell_exec() does not return an exception. However, in this case there is no need to actually catch an exception at all.

I do believe that the focus of this enhancement request should be changed and we need to discuss if the way ImageMagick is used in the WP core could be improved.

Right now in the WP core ImageMagick is used just because it supports more file types then GD. That is why in WP 4.7 we can now generate thumbnails of PDF files. What actually happens is that when a PDF file is uploaded the ImageMagick part of the code kicks in and it generates an object which is loaded into memory and it stores a jpeg picture. After that GD is used for manipulating this picture. In other words right now ImageMagick is used only as a bridge so that PDF files could be displayed better in the media library. Judging by the used code probably the class for ImageMagick was copied over from the GD class and simply the functions that are used were changed.

This is all good because ImageMagick gives us more flexibility in terms of what could be done with images and other files (SVG, jpeg, png, pdf files, etc.). However, right now the ImageMagick is used only to rotate, crop, flip and resize images - all of these things are 100% supported by GD.

According to the performance tests that @Hristo Sg provided PHP is 2-3 times slower and uses 25% more memory when ImageMagick is loaded and used. @dd32 as you mentioned in 6821 some hosts have all the needed convert binaries but do not load the ImageMagick PHP PECL by default. On such hosts it will be nice to offer support for PDF thumbnail generation by using the binaries installed on the servers. Of course, the real problem here is that it need's to be decided if the WP core should be calling external commands. My personal opinion is that in this specific case this is not a problem because the potential benefits are actually more than the potential problems (which could be addressed with carefully checking the used code):

• Better performance and memory usage of WP core
• Getting the new feature provided by WP 4.7 available to more people
• Better support for different environments. The user doesn't care of the host has ImageMagick or not. The user just wants to see the thumbnail generated and when ImageMagick is not available this will not happen.

Of course, this is just my opinion and other people from the community should share their thoughts too and it would be great if we get more people look into this.

[gitlost]

Looking at supports_mime_type() in the suggested WP_Image_Editor_Imagick_External class and it explicitly only supporting PDFs prompts the question why use ImageMagick at all for the production of PDF previews - why not just shell out to GhostScript directly?

ImageMagick itself is as mentioned just a bridge, shelling out to GhostScript, and a klunky bridge at that as I found (#39216) when trying to fix issues with PDFs with CMYK color spaces, with as far as I could see no way via ImageMagick (due to the PAM intermediate format) to utilize the default ICM/ICC profile handling contained in GhostScript.

In producing PDF previews (see https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/image.php#L237), no manipulation is done using the Imagick editor that creates the "full" preview jpeg, it's just thrown away and the jpeg reopened in a new (Imagick) editor. So it would make sense to me to produce the "full" preview using a GhostScript-based editor dedicated to that (or indeed a direct conversion function, but that's another story). The following patch (only tested on Ubuntu) is a proof of concept of this.

The shared hosts overload issues mentioned in ticket:6821#comment:33 shouldn't be affected (and should presumably be lessened) by using GhostScript directly given that ImageMagick does it itself. The security issues mentioned may still need to be addressed, but should be surmountable I'd have thought with a careful enough implementation (no doubt more careful than in the attached patch).

[gitlost]

POC GhostScript shell image editor.

[kirasong]

Replying to danielkanchev:

Right now in the WP core ImageMagick is used just because it supports more file types then GD. That is why in WP 4.7 we can now generate thumbnails of PDF files.

<snip>

• Better support for different environments. The user doesn't care of the host has ImageMagick or not. The user just wants to see the thumbnail generated and when ImageMagick is not available this will not happen.

Hey there!

Wanted to address these two notes. Imagick/ImageMagick is used by default not because it supports more types (although this does help with PDF, for sure) but because it is better at all of the operations that GD can do, and is more customizable as to how it does those operations.

It enables smaller image creation at better qualities, and allows for maintaining EXIF/IPTC information, which includes color profiles.

In general, it's much better at handling images than GD.

In terms of expanding the support of PDF thumbnail creation, and the addition of an image editor that supports creation via the shell:
First, thanks so much for writing this! It's something that was considered early on, but was decided against, both due to time constraints, and due to not wanting to shell out.

I tend to agree with @dd32 on this point, and don't think that the increase in compatibility is worth adding something that shells out to core. This is mostly because at this point, I find that most hosts that put WordPress as a priority understand that Imagick is key for WordPress handing images as best as it can, and tend to include it with their plans.

I see the performance benefit, but given that this is only affected while users are uploading images, the additional surface seems like a bigger issue than reducing this load.

I would absolutely love to see this in plugin form! Part of the reason that PDF thumbnail creation, and, indeed, WP_Image_Editor was written as it is is so that plugins that add WP_Image_Editors like yours, or the one written by @gitlost can be included, and WordPress will choose to use it for image operations.

[kirasong]

Replying to Hristo Sg:

Here are the performance test we've done so far:

Thanks for running these tests!

Are these upload tests/tests that use WP_Image_Editor, or is it that you're seeing *all* requests with the Imagick module enabled be slower?

[Hristo Sg]

The tests are timing this <?php echo "OK"; ?> with and without imagick pecl loaded into PHP

So, yes *all* requests are being slowed down by just including imagick pecl into PHP

It doesn't matter if imagick functions are being used or not, every request takes the hit.

[kirasong]

Replying to Hristo Sg:

The tests are timing this <?php echo "OK"; ?> with and without imagick pecl loaded into PHP

So, yes *all* requests are being slowed down by just including imagick pecl into PHP

It doesn't matter if imagick functions are being used or not, every request takes the hit.

Based on that, I definitely think it makes sense to test this on some more platforms to see how it looks.

I don't like that all of the requests are slower, but feeling like, even if it does show it to be this much slower everywhere, the performance loss of 0.07ms (note ms, rather than seconds) per request is probably worth it to users having a better image experience.

I still love the idea of this in a plugin so that hosts that prefer that option (but still want to see the benefits of Imagick for their users) can choose it.

Would be interested to hear others' opinions on this as well.

[gitlost]

A plugin implementing the direct shell-out to Ghostscript, by-passing Imagick/ImageMagick, is now available in the WP repository as ​GS Only PDF Preview (thanks anonymous plugin reviewer!).

It contains a worked-up version of the WP_Image_Editor_GS class prototyped in 39262.patch (see ​GOPP_Image_Editor_GS), and claims to address the security concerns of shelling out.

[Hristo Sg]

That ticket could definitelly get some love especially now when the Health Check further recommends imagick

[adamsilverstein]

This ticket is quite stale and it looks like the WP_Image_Editor_Imagick_External class was offered as part of a plugin. PHP/Image editing has grown stronger and this feels like more of a plugin feature than something core needs to support directly at this point.

I am closing this as "wontfix" - @hristo-sg please feel free to re-open this ticket if you feel it is still needed in core.