WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 17 months ago

Last modified 16 months ago

#6821 closed task (blessed) (fixed)

ImageMagick support

Reported by: matt Owned by: DH-Shredder
Milestone: 3.5 Priority: normal
Severity: normal Version: 2.5
Component: Upload Keywords: has-patch needs-testing needs-codex
Focuses: Cc:

Description (last modified by scribu)

Our image handling and resizing functions should magically detect whether IM is available and usable (much like Gallery does) and use them where appropriate, as it's a lot more efficient than PHP's built-in GD functions.

Attachments (35)

class-wp-image.php.patch (2.8 KB) - added by tomauger 21 months ago.
INCOMPLETE - just a starting point for discussion around general high-level methods for library-independent image manipulation.
Initial_Proposal.php (2.1 KB) - added by DH-Shredder 21 months ago.
Initial Suggestion for Interface/Class for Image Manipulation
benchmark_imagick.php (692 bytes) - added by lbr 21 months ago.
benchmark script using imagick exec|extension and graphicsmagick extension
6821.patch (21.0 KB) - added by markoheijnen 21 months ago.
First try to implement my class on github
ImagickHostAvailability.md (1.6 KB) - added by DH-Shredder 20 months ago.
Initial Host ImagicK Availability [edit: unrelated notes removed]
6821.diff (43.2 KB) - added by DH-Shredder 20 months ago.
Initial Abstraction of GD with WP_Image_Editor
6821.initialImagickSupport.diff (46.7 KB) - added by DH-Shredder 20 months ago.
Initial Imagick support (module)
6821.2.diff (46.8 KB) - added by DH-Shredder 20 months ago.
Includes fixes to Notices in Custom Header
6821.3.diff (46.3 KB) - added by DH-Shredder 20 months ago.
Updated version for chat. To be edited further based on decisions made.
6821.4.diff (46.2 KB) - added by DH-Shredder 20 months ago.
Updated. Includes abstract classes for base, Merged WP_Image_Edit+Base, and other bug fixes.
6821.5.diff (46.0 KB) - added by markoheijnen 20 months ago.
6821.6.diff (47.8 KB) - added by DH-Shredder 20 months ago.
Now with Improved Exception Handling and more WP_Error than ever before!
6821.7.diff (48.3 KB) - added by DH-Shredder 19 months ago.
Passes Unit Tests. On-Load Error Handling Improvements. Better i18n on Errors.
6821.8.diff (55.7 KB) - added by DH-Shredder 19 months ago.
mime_type support, additional docs, and a whole lot of bug fixes [edit:No more short-form ternary]
6821.9.diff (55.8 KB) - added by DH-Shredder 19 months ago.
Previous patch, plus make $implementation a static property for unit-testness
6821-unit-tests-4.patch (20.5 KB) - added by kurtpayne 19 months ago.
First round of unit tests
6821.9.2.diff (55.8 KB) - added by DH-Shredder 19 months ago.
Fix Warning for files with no extension, and use trailingslashit. Props markoheijnen.
6821-unit-tests-4.2.patch (20.5 KB) - added by kurtpayne 19 months ago.
Adding realpath to be compatible with changes in r22008
6821.9.3.diff (57.4 KB) - added by DH-Shredder 19 months ago.
Refresh, and more docs.
6821.check-errors.diff (2.1 KB) - added by markoheijnen 19 months ago.
Added and fix error checks
6821.fix-strict-standards.diff (2.4 KB) - added by markoheijnen 19 months ago.
6821.check-errors2.diff (377 bytes) - added by markoheijnen 18 months ago.
Return error when no editor is selected
xhprof-gd-jpg-10mp.png (209.9 KB) - added by bpetty 18 months ago.
xhprof-imagick-jpg-10mp.png (337.9 KB) - added by bpetty 18 months ago.
6812-fix-wp-error.patch (1.3 KB) - added by bpetty 18 months ago.
6821.10.diff (8.8 KB) - added by markoheijnen 18 months ago.
No caching and always run test
6821.11.diff (1.1 KB) - added by ryan 18 months ago.
Pass real attachment ID to _load_image_to_edit_path() from wp_crop_image()
6821.12.diff (2.2 KB) - added by ryan 18 months ago.
Allow URLs
6821.doc-fixes.diff (1.7 KB) - added by duck_ 18 months ago.
6821.13.diff (706 bytes) - added by ryan 17 months ago.
In wp_crop_image(), preserve both $src_file and $src.
6821-ut.diff (591 bytes) - added by ryan 17 months ago.
Unit test fixes
6821-ut.2.diff (2.2 KB) - added by ryan 17 months ago.
Basic wp_crop_image() tests.
6821.docs.src_abs.diff (1.6 KB) - added by DH-Shredder 17 months ago.
src_abs Docs Fixes
6821.reabstract.diff (10.3 KB) - added by DH-Shredder 17 months ago.
Re-add Abstract methods. Add lots of PHPDoc, and PHPDoc Fixes.
more_image_editor_docs.diff (2.0 KB) - added by DH-Shredder 17 months ago.
Fix multi_resize docs to describe array properly. Fix one mention of imagecopyresampled().

Download all attachments as: .zip

Change History (164)

comment:1 jacobsantos6 years ago

I think it would be cool to include an image library that allow for plugins to extend the library to implement these extra features.

comment:2 Denis-de-Bernardy5 years ago

  • Component changed from General to Upload

comment:3 ryan4 years ago

  • Milestone changed from 2.9 to Future Release

comment:4 scribu3 years ago

Marked #11671 as duplicate.

comment:6 azaozz3 years ago

This is probably the oldest to-do on my list too. Postponed it 2 years ago as ImageMagick was relatively rare. Perhaps it's time to look into this again, would also need to reevaluate which serves us better IM or GD (that are available in PHP 5.2.4+).

comment:7 scribu3 years ago

Marked #18539 as duplicate.

comment:8 follow-up: GaryJ2 years ago

A note for a utopia future where PHP 5.3 is a minimum version for WP - related: https://github.com/avalanche123/Imagine

comment:9 nacin21 months ago

  • Keywords blessed removed
  • Milestone changed from Future Release to 3.5
  • Type changed from enhancement to task (blessed)

I would really like to see Imagemagick support added to 3.5, to complement our focus on the media, upload, and gallery user experience.

This ticket needs someone (or a few someones) to grab it by the horns and run with it. If your strong suit is API and backend development but want to contribute to the media/upload project in 3.5, this is definitely a great way to help.

comment:10 markoheijnen21 months ago

  • Cc markoheijnen added

I'm curious in how we should implement this. The implementation would be something like how it is done with the HTTP API.
I'm also curious if this will effect server load like in case of WordPress.com.

comment:11 nacin21 months ago

See also #18543 - wrapping our GD functions in methods that can allow for using stream wrappers (CDN integration, etc.).

Yes, I do picture an API that looks a lot like the HTTP API. (This could be done as an interface, though an abstract base class would probably make sense, in case there is any shared functionality.) We would support both GD and IM in core, with a preference toward IM, and replace all of our direct GD function calls with this new API.

comment:12 Japh21 months ago

  • Cc japh@… added

comment:13 tomauger21 months ago

  • Cc tomaugerdotcom@… added

comment:14 DH-Shredder21 months ago

  • Cc mike.schroder@… added

Chatted with nacin, Japh, TomAuger, and MarkoHeijnen on this in dev chat today, and doing some research into our use of GD for the best way to build an API to accomplish this.

This will likely turn out to be something very similar to the HTTP API, as nacin has suggested.

comment:15 mikeschinkel21 months ago

  • Cc mikeschinkel@… added

comment:16 tomauger21 months ago

So, what's the best way to inventory all the current usages of GD library functions in core? Just iterating through all the functions available on http://www.php.net/manual/en/ref.image.php seems like a pretty brute force way to go.

comment:17 Japh21 months ago

There are 103 functions listed under "GD and Image Functions" on PHP.net, and we have used 23 of them across 7 files:

  • ./wp-admin/custom-header.php
  • ./wp-admin/includes/image.php
  • ./wp-includes/deprecated.php
  • ./wp-includes/functions.php
  • ./wp-includes/media.php
  • ./wp-admin/includes/image.php
  • ./wp-admin/includes/image-edit.php

Specific functions used are:

  • getimagesize
  • imagealphablending
  • imagesavealpha
  • imageantialias
  • imagetruecolortopalette
  • imagecolorstotal
  • imagecopyresampled
  • imagecopy
  • imagecreatefromjpeg
  • imagecreatefrompng
  • imagecreatefromgif
  • imagecreatefromstring
  • imagecreatetruecolor
  • imagedestroy
  • imagegif
  • imageistruecolor
  • imagejpeg
  • imagepng
  • imagerotate
  • imagesx
  • imagesy
  • imagetypes
  • iptcparse

It might be worth checking through ImageMagick functions to see if there can be any direct mapping.

comment:18 tomauger21 months ago

Woot! Awesome work. I'll get started on the NetPM and IM mappings to try to see whether we can design a versatile interface. Then it will just be a matter of creating some adaptors and then replacing all the function calls with method calls.

tomauger21 months ago

INCOMPLETE - just a starting point for discussion around general high-level methods for library-independent image manipulation.

comment:19 scribu21 months ago

  • Description modified (diff)
  • Summary changed from Netpbm and Imagemagick support to ImageMagick support

comment:20 scribu21 months ago

Had a chat in IRC with TomAuger, DH-Shredder and MarkoHeijnen about the class architecture.

Here's my suggestion:

interface WP_Image_Editor {
  function __construct( $file_path );
  function resize( ... );
  function rotate( ... );
  function save( ... );
}

class WP_Image_Editor_IM implements WP_Image_Editor {
  ...
}

class WP_Image_Editor_GD implements WP_Image_Editor {
  ...
}

function wp_get_image_for_editing( $file_path ) {
  // check file path

  $class = function_exists( ... ) ? 'WP_Image_IM' : 'WP_Image_GD';

  return new $class( $file_path ); 
}

Usage example:

$image = wp_get_image_for_editing( $file_path );
$image->resize( $w, $h );
$image->save();

comment:21 DH-Shredder21 months ago

Chatted about this in IRC today:
https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2012-07-27&sort=asc#m428904

We're currently deciding whether it's best to go with a WP_Image (image container) abstraction, or more of a WP_Image_Editor abstraction.
I'm leaning towards WP_Image_Editor, like scribu has suggested.

We'll be going through the code further over the weekend, checking in on Monday, and meeting on Tuesday at Dev Chat time (20:00 UTC), so that we have a proposal (at the very least) ready for next dev chat.

(edited to use Scribu's class names)

Last edited 21 months ago by DH-Shredder (previous) (diff)

comment:22 Mamaduka21 months ago

  • Cc georgemamadashvili@… added

comment:23 kuemerle521 months ago

  • Cc kuemerle5@… added

comment:24 westi21 months ago

  • Owner tellyworth deleted
  • Status changed from new to assigned

comment:25 kieranmasterton21 months ago

  • Cc kieranmasterton added

comment:26 markoheijnen21 months ago

I was busy with creating something on github (https://github.com/markoheijnen/WP_Image). Hopefully I can get something working in the next 24 hours. When I'm fine with a first version I will post it here.

I do also work on a WP_Image class since I do believe it's value. Even if it is in the begin a simple layer for using an attachment ID instead of the file path. Since we will toughing a lot of the image generating code I also hope we can get something working that you can do add_image_size with an extra argument if it should be auto generated.

I was thinking if we also should add a way to create @2x images. Since I believe there isn't a good way at this moment to do so if you want to add @2x to the filename.

DH-Shredder21 months ago

Initial Suggestion for Interface/Class for Image Manipulation

comment:27 follow-up: DH-Shredder21 months ago

Posted Initial_Proposal.php as a topic for conversation.

I'm currently still working on an implementation of resize for GD (then ImageMagick) as a proof-of-concept, but wanted to post this in the meantime.

It's very much a work in progress, since going from a completely image-in-memory-then-save process is significantly different than avoiding any images in memory, and doing as few system calls as possible, for something like convert.

It does have one characteristic of an image container, and accepts a file as initial input for instantiation of the class. This was done so that wp_load_image can use it as a replacement or creation of a new image, and should help with integration into current code.

It's posted as an interface, but will likely end up as an abstract base class, so that common content and decisions on what the best Editor to use can be made appropriately.

I'd thought about including functions like truecolor_to_palette(), but decided against it because not all Editors need to make this an extra step. In ImageMagick, it's just another parameter for file conversion/creation that can go along with a resize, for instance.

Questions/Ponderings:

  • Streams are currently being handled by changing what is output by the functions, which isn't great. Any suggestions as to how to handle this case the best?
  • Another option to having an Output File as an optional parameter would be to defer all changes until .save() is called, which is (I believe) what Scribu had initially suggested. What seems best / thoughts on an even better way to do it?
  • It's easy enough to default to the "best" class, and use that, but unsure on the best approach to detect/automatically use secondary tools for only specific methods. Ideas?

comment:28 Dartanjan21 months ago

  • Cc Dartanjan added

comment:29 in reply to: ↑ 27 ; follow-up: lbr21 months ago

Replying to DH-Shredder:

Posted Initial_Proposal.php as a topic for conversation.

I'm currently still working on an implementation of resize for GD (then ImageMagick) as a proof-of-concept, but wanted to post this in the meantime.

It's very much a work in progress, since going from a completely image-in-memory-then-save process is significantly different than avoiding any images in memory, and doing as few system calls as possible, for something like convert.

are you sure that really need to execute system calls? http://www.php.net/manual/en/book.imagick.php

I'd thought about including functions like truecolor_to_palette(), but decided against it because not all Editors need to make this an extra step. In ImageMagick, it's just another parameter for file conversion/creation that can go along with a resize, for instance.

In my opinion, one essencial feature would be the option to set a default quality to image
(http://www.imagemagick.org/script/command-line-options.php#quality) then, the user administrator could enable/set a value to the optimization...

one last thing, why not graphicsmagick? has better performance and less problems than imagemagick...

comment:30 Japh21 months ago

While we're rewriting the image functions and writing the ImageMagick API, it would be good to keep in mind making the functions testable ( https://unit-tests.trac.wordpress.org/ticket/104 by nbachiyski ).

We should also make sure that we have test coverage for functions that we aren't deprecating and will use the new API.

Last edited 21 months ago by Japh (previous) (diff)

comment:31 in reply to: ↑ 29 DH-Shredder21 months ago

Replying to lbr:

Replying to DH-Shredder:

Posted Initial_Proposal.php as a topic for conversation.

I'm currently still working on an implementation of resize for GD (then ImageMagick) as a proof-of-concept, but wanted to post this in the meantime.

It's very much a work in progress, since going from a completely image-in-memory-then-save process is significantly different than avoiding any images in memory, and doing as few system calls as possible, for something like convert.

are you sure that really need to execute system calls? http://www.php.net/manual/en/book.imagick.php

If the host has ImagicK installed, then we can use it.
Most hosts have convert installed, but not ImagicK so that's something we should support.

I'd thought about including functions like truecolor_to_palette(), but decided against it because not all Editors need to make this an extra step. In ImageMagick, it's just another parameter for file conversion/creation that can go along with a resize, for instance.

In my opinion, one essencial feature would be the option to set a default quality to image
(http://www.imagemagick.org/script/command-line-options.php#quality) then, the user administrator could enable/set a value to the optimization...

Definitely. Image quality settings are something that we still want to be settable -- and from the docs, it looks like that's something that can be done with other image types as well with ImageMagick

one last thing, why not graphicsmagick? has better performance and less problems than imagemagick...

Sure, we'll either detect for it, or just allow you to manually specify the path to convert, since the GraphicsMagick binary is compatible with convert.

As far as supporting GMagicK itself, it shouldn't be complicated to either add it later, or create a plugin that introduces support, once WP_Image_Editor is available.

comment:32 DH-Shredder21 months ago

We met about this at WCSF -- a few notes on decisions and reasons from our discussions:

  • We'll focus on WP_Image_Editor, and leave WP_Image for later, unless it turns out that it's significantly easier to abstract GD while using it.
  • We also need a batch resizer method, to aid with thumbnail generation
  • We'll work initially on Marko's GitHub account, with a first draft of abstract classes to be posted on the ticket before this Wednesday's dev chat for discussion.
  • Initially, Editor classes will be chosen by rough order rather than by function (i.e. resize/crop/rotate), and we're still working out how this could be done elegantly. Suggestions welcome.
  • It will likely be the case that any existing public/API functions that are accepting or returning GD images in memory will need to be deprecated, as to avoid breaking current plugins. In theses cases, we'll call the new Class's methods instead.

comment:33 dd3221 months ago

If the host has ImagicK installed, then we can use it. Most hosts have convert installed, but not ImagicK so that's something we should support.

For what it's worth IMO , due to the security implications of executing a process outside of PHP, command-line graphic applications should be left for implementation by a plugin. Core should support the Imagemakic PHP Extension, as well as have an extensible API to allow for others to be plugged into it, but WordPress Core shouldn't be calling an external CLI program to proceess images.. It could have the ability to overload any shared hosts with stricter memory limitations as well.

comment:34 lbr21 months ago

agree with dd32, i think that if the host doesn't have imageMagick, it should use GD, if none then show a kind of alert...

comment:35 markoheijnen21 months ago

I partly agree with it. I can understand why using the extension would make sense but what if running the CLI is way faster?
Also I don't think overload a shared hosting is still the issue when running the extension.

comment:36 dd3221 months ago

I can understand why using the extension would make sense but what if running the CLI is way faster?

For a process which isn't run interactively/blocking a pageload, this is of little consequence, we can afford an extra second if that's what it takes. While we should attempt to use the least amount of processing time, some operations are limited by memory and processing power available..

Also I don't think overload a shared hosting is still the issue when running the extension.

AFAIK, Correct, because it will be running within the PHP Process, it'll be limited to the resources allocated to PHP.


Upon looking into it further, i might be misunderstanding how the Imagemagick extension works - I was thinking it's responsible for the processing, however, it seems like it still requires a ImageMagick installation separately on the server which does the processing by executing convert, so it looks like my points might be moot if we want support for Imagemagick in core at all.

comment:37 follow-up: lbr21 months ago

I've found this benchmark that uses GD, imagemagick's php extension and exec command to process some images. The result isnt very different.

I also done a small script in php to measure both cases, and graphicsmagick (this extension is in beta) - it's attached (benchmark_imagick.php).

Last edited 21 months ago by lbr (previous) (diff)

lbr21 months ago

benchmark script using imagick exec|extension and graphicsmagick extension

comment:38 in reply to: ↑ 37 ; follow-up: Japh21 months ago

Replying to lbr:

I've found this benchmark that uses GD, imagemagick's php extension and exec command to process some images. The result isnt very different.

I just wanted to note that performance isn't the only reason for wanting to switch from GD to ImageMagick. Better visual results are also a contributing factor.

I also done a small script in php to measure both cases, and graphicsmagick (this extension is in beta) - it's attached (benchmark_imagick.php).

Do we have any stats on hosting support for graphicsmagick / ImageMagic / ImagicK?

comment:39 in reply to: ↑ 38 lbr21 months ago

Japh, exactly. imagemagick is a great improvement in wp - and thats why i'm here trying to help :)

about the usage by hostings, i have no idea. but, there's a small problem that many shared servers doesnt supports exec (system) call, some doesnt supports the extension imagemagick...

comment:40 kurtpayne21 months ago

  • Cc kpayne@… added

comment:41 in reply to: ↑ 8 ; follow-up: scribu21 months ago

Replying to GaryJ:

A note for a utopia future where PHP 5.3 is a minimum version for WP - related: https://github.com/avalanche123/Imagine

The only PHP 5.3 feature the Imagine lib seems to be using is namespaces. Otherwise, that chainable API is perfectly implementable in PHP 5.2.

The problem though is that Imagine is also pretty young and doesn't support all the drivers we want, so it doesn't make much sense to attempt a port.

I do like how saving is a separate method. In the interface that DH-Shredder posted, each method has an $output_file parameter, which is not very orthogonal.

markoheijnen21 months ago

First try to implement my class on github

comment:42 markoheijnen21 months ago

Trying to change some of the WordPress functions to use the WP_Image_Editor class. It still missing methods like save and abstraction of crop.

comment:43 in reply to: ↑ 41 DH-Shredder21 months ago

Replying to scribu:

I do like how saving is a separate method. In the interface that DH-Shredder posted, each method has an $output_file parameter, which is not very orthogonal.

I agree that it's not ideal. It was done because I was having difficulty coming up with a design pattern with regards to output that solved all of the issues, and would definitely be open to suggestions as to a better way to solve the output problem.

The reason that this wasn't done with a single required save() method for output is twofold:

  • I expect that we'll be using the same input image to do multiple operations, with output to different filenames.
  • We're moving from holding images in RAM to using input files directly in the case of convert/gm, which makes either an output filename or temporary file necessary, for changes not done in-place.

Do you think it'd be better for convert/gm cases to perform all actions to a temporary file, then "saved (copied)" to a final destination (whether it be on top of the current file, or another location)? I think, in particular for cases where we're thumbnailing, that would probably cause more I/O load than we want to do.

comment:44 scribu21 months ago

I think creating a temporary file and then having save() just move it around is acceptable.

Also, I don't see why the following workflow would be a problem:

$image->crop( ... );
$image->save( 'path/file1.jpg' );
$image->scale( ... );
$image->save( 'path/file2.jpg' );
Last edited 21 months ago by scribu (previous) (diff)

comment:45 follow-up: scribu21 months ago

Some interesting discussions happened today in IRC with regards to choosing a specific implementation. Basically, it boils down to two approaches:

A) Pick the best/available implementation before executing each operation.

This offers ultimate flexibility: it allows using libraries that implement only a subset of required operations, but is more complex to implement and test.

A stab at this approach can be seen here: https://github.com/markoheijnen/WP_Image/blob/d41803a446978d34bddf47d28c8fa313f5a0ba80/class-wp-image-editor.php#L11

B) Pick the best/available implementation at the beginning and stick to it.

This is how the HTTP component in WP works. It requires that all implementations support the entire set of required operations, but is easier to implement.

Made a quick proof of concept: https://gist.github.com/5ee61a934a8c38fe8e2b

Last edited 21 months ago by scribu (previous) (diff)

comment:46 in reply to: ↑ 45 Japh21 months ago

These two approaches were discussed quite a bit at the WCSF 2012 Developer Hack Day too. 'A' seems like the best approach, but as you say, is more complex.

Marko's proof of concept in his GitHub repo has an element of the 'A' approach implemented too. Your Gist looks good.

comment:47 follow-up: scribu21 months ago

Would be nice if there was a summary of those discussions posted somewhere.

comment:48 in reply to: ↑ 47 Japh21 months ago

Replying to scribu:

Would be nice if there was a summary of those discussions posted somewhere.

It certainly would be! I'm hoping DH-Shredder might post a summary?

comment:49 DH-Shredder21 months ago

The major decisions regarding workflow got summarized on the day of (check it out above).

As far as that decision, specifically -- we'd talked about that we'd really like to do A) but had not settled on a best approach.

From the comment: "Initially, Editor classes will be chosen by rough order rather than by function (i.e. resize/crop/rotate), and we're still working out how this could be done elegantly. Suggestions welcome."

As scribu mentioned in wp-dev chat, the problem with that is with starting operations with a rotate in jpegtran or ImagicK, then batch thumbnailing with 'convert'.

This is most difficult when switching between GD and other Editor classes, since GD holds its images in RAM.

comment:50 follow-up: scribu21 months ago

Yeah, I had read that comment, but I guess it didn't really ring a bell.

Anyway, what about this one: "We also need a batch resizer method, to aid with thumbnail generation".

What does a "batch resizer method" do?

comment:51 in reply to: ↑ 50 DH-Shredder21 months ago

Replying to scribu:

Anyway, what about this one: "We also need a batch resizer method, to aid with thumbnail generation".

The idea there would be to be able to pass it a ton of sizes at once, and it generates them all, to avoid duplicated effort where possible.

This would be important during initial thumb generation, and more-so when we hit retinaization for content.

comment:52 follow-up: scribu21 months ago

I see. Well, WP_Image_Editor could have a default implementation for multi_resize() that just calls resize() a bunch of times.

Then, implementations like 'convert' can overwrite it to do it in a more optimal way.

Last edited 21 months ago by scribu (previous) (diff)

comment:53 in reply to: ↑ 52 DH-Shredder20 months ago

Replying to scribu:

I see. Well, WP_Image_Editor could have a default implementation for multi_resize() that just calls resize() a bunch of times.

Then, implementations like 'convert' can overwrite it to do it in a more optimal way.

Agreed. That'd been my thought on the matter as well. Was just a note that we need to add it to the proposed interface.

comment:54 mbijon20 months ago

  • Cc mike@… added

DH-Shredder20 months ago

Initial Host ImagicK Availability [edit: unrelated notes removed]

comment:56 DH-Shredder20 months ago

Initial Host availability attached.

Additional work on this ticket has been happening on the fork here:
https://github.com/markoheijnen/WordPress

comment:57 Viper007Bond20 months ago

  • Cc Viper007Bond added

comment:58 DH-Shredder20 months ago

Now moved to a topic branch: https://github.com/markoheijnen/WordPress/tree/ticket-6821-imagemagick

We're close to the point where everything is working with GD, with an audit of GD core code in use and filter run left.

Nacin mentioned in dev chat yesterday that we shouldn't worry about handling compatibility with old filters that accept GD Resources, but probably just use new ones:
https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2012-08-29&sort=asc#m447299

If there are any suggestions/comments on this, they are welcome.

comment:59 Japh20 months ago

I'm very much looking forward to seeing a first patch and jumping in from there. I can understand why we've opted for GitHub on this one for now, but it'd be great to post patches here periodically too.

DH-Shredder20 months ago

Initial Abstraction of GD with WP_Image_Editor

comment:60 DH-Shredder20 months ago

  • Keywords has-patch needs-testing added

Attached is an initial abstraction of GD with WP_Image_Editor.
A partially built Imagick Editor Class is also included, but isn't functioning yet.

Comments and testing welcome! At the moment, there remain a few things:

  • Clear out and further abstract some of the remaining GD functionality in core
  • Audit entire code base for further necessary changes to use WP_Image_Editor (this is started)
  • Refine filters for new class, making sure that we're not limiting users in extending Image Editing in core
  • Finish Imagick class first, then finally convert (if there's time). Imagick first only because it's a lot more similar in functionality to the GD Editor class, and is the low-hanging fruit for "next editor."
  • Performance and quality testing on both editors.

Please note if you'd like to help with any of this, or have any questions!

DH-Shredder20 months ago

Initial Imagick support (module)

comment:61 DH-Shredder20 months ago

Initial Imagick (module) support added in 6821.initialImagickSupport.diff.

Still very rough, but basically working.
Updating here to keep everyone up to date.

Would love to see help and input per the above notes still.

DH-Shredder20 months ago

Includes fixes to Notices in Custom Header

comment:62 DH-Shredder20 months ago

6821.2.diff attached with fixes to Notices when uploading an image via Appearance->Custom Header

In case it wasn't clear, the patches are a group effort between markoheijnen and myself, with assistance from scribu (and additional API planning earlier with Japh).

edit: known issue with cropping and streaming Animated GIFs that we're currently working on.

Last edited 20 months ago by DH-Shredder (previous) (diff)

DH-Shredder20 months ago

Updated version for chat. To be edited further based on decisions made.

comment:63 scribu20 months ago

Conclusions from IRC discussion:

  • the WP_Image_Editor class should just be a function, or be merged with WP_Image_Editor_Base
  • the wp-includes/editors directory is unnecessary
  • add abstract make_image() method in the base class, if it's a public method, so that all implementations have it

Undecided:

  • rename WP_Image_Editor to WP_Image_For_Editing; what do we call the implementation classes then?
  • use encapsulation instead of inheritance, i.e. WP_Image_For_Editing contains a reference to a particular implementation class

My question:

  • do we really need both 'wp_editor_use_' . $editor AND 'image_editor_class' hooks?

comment:64 markoheijnen20 months ago

I removed the 'wp_editor_use_' . $editor hook. That is almost the same as 'wp_editors'.

I think image_editor_class has it place since that is the only way to change editor when there is one selected.

DH-Shredder20 months ago

Updated. Includes abstract classes for base, Merged WP_Image_Edit+Base, and other bug fixes.

comment:65 markoheijnen20 months ago

Just added a new patch with the following GIT commited messages:

  • getsource Constructor should be protected. Props scribu.
  • kurtpayne Use instance of instead of is_resource. Deprecate GD specific functions.
  • kurtpayne Implement backward compatible behavior to default to jpeg filenames and jpeg output
  • getsource Don't instantiate WP_Image_Editor_GD directly from image_resize.
  • getsource Load on construct.
  • getsource GD: Make $image protected
  • markoheijnen Don't say you can use WP_Image_Editor_GD in a deprecated function message
  • markoheijnen Move make_image to WP_Image_Editor and replaced call_user_func for call_user_func_array
  • markoheijnen Fix notice by adding destiny filename as required argument for make_image
  • markoheijnen Use make_image for Imagick
Last edited 20 months ago by markoheijnen (previous) (diff)

markoheijnen20 months ago

DH-Shredder20 months ago

Now with Improved Exception Handling and more WP_Error than ever before!

comment:66 DH-Shredder20 months ago

Attached patch includes complete exception handling for Imagick, and WP_Error all around.

Last edited 20 months ago by DH-Shredder (previous) (diff)

comment:67 prettyboymp19 months ago

  • Cc mpretty@… added

DH-Shredder19 months ago

Passes Unit Tests. On-Load Error Handling Improvements. Better i18n on Errors.

comment:69 DH-Shredder19 months ago

New Patch attached. Includes fixes for some unit test issues (namely, s/JPG/jpg/), fixes on load error handling, errors on resizing an image to its current size, better i18n on Errors, and other misc fixes. Includes some fixes from kurtpayne and markoheijnen.

comment:70 follow-up: griffinjt19 months ago

  • Version changed from 2.5 to trunk

Just as a note, on the most recent patch, you need to apply to jpeg_quality filter to L226 inside the wp_stream_image function.

Version 0, edited 19 months ago by griffinjt (next)

comment:71 SergeyBiryukov19 months ago

  • Version changed from trunk to 2.5

comment:72 in reply to: ↑ 70 DH-Shredder19 months ago

Replying to griffinjt:

Just as a note, on the most recent patch, you need to apply the jpeg_quality filter to L226 inside the wp_stream_image function.

Thanks! Makes enough sense to add it.
At the moment, that line's the same as it is in trunk:
http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/image-edit.php#L206

DH-Shredder19 months ago

mime_type support, additional docs, and a whole lot of bug fixes [edit:No more short-form ternary]

comment:73 DH-Shredder19 months ago

Updated patch, with included contributions from Markoheijnen and lots of testing assistance from Kurtpayne.

Many unit tests created by Kurt that will be posted/added soon (Many Thanks!).

Also, removed Short-form Ternary per scribu (no 5.2 support for short-form).

DH-Shredder19 months ago

Previous patch, plus make $implementation a static property for unit-testness

comment:74 DH-Shredder19 months ago

Updated patch to make $implementation a static property, so that we can make unit tests to test selection of Editors. Props Kurtpayne.

kurtpayne19 months ago

First round of unit tests

DH-Shredder19 months ago

Fix Warning for files with no extension, and use trailingslashit. Props markoheijnen.

kurtpayne19 months ago

Adding realpath to be compatible with changes in r22008

DH-Shredder19 months ago

Refresh, and more docs.

comment:75 ryan19 months ago

In [22094]:

Introduce WP_Image_Editor, WP_Image_Editor_Imagick, and WP_Image_Editor_GD. Abstracts image editing API and adds support for ImageMagick.

Props DH-Shredder, kurtpayne, markoheijnen
see #6821

comment:76 ocean9019 months ago

  • Cc ocean90 added

comment:78 navjotjsingh19 months ago

  • Cc navjotjsingh@… added

comment:79 Viper007Bond19 months ago

Well done gentlemen!

comment:80 ryan19 months ago

PHP Strict Standards:  Static function WP_Image_Editor::test() should not be abstract in /.../wp-includes/class-wp-image-editor.php on line 79

PHP Strict Standards:  Static function WP_Image_Editor::supports_mime_type() should not be abstract in /.../wp-includes/class-wp-image-editor.php on line 81

comment:81 nacin19 months ago

An interface can have abstract static methods, but not a class. There's no concept of "overriding" a static method. The closest there is is late-static binding in PHP5.

markoheijnen19 months ago

Added and fix error checks

comment:82 markoheijnen19 months ago

Added two patches for fixing error handeling and the PHP strict standard notices

comment:83 ryan19 months ago

In [22119]:

Avoid Strict Standards warnings. Props markoheijnen. see #6821

comment:84 nacin19 months ago

[22119] just means that non-static methods will get called in a static context.

As far as I can tell, both test() and supports_mime_type() should indeed be static methods. We just need to remove them from the base class, as they are unused there.

If we decide there is a set of specific methods an editor class must have (test() and supports_mime_type() obviously being two of them), then the base class should implement an interface.

We also need to figure out what happens if we ever want to add a method. It seems to me like the current API would not allow for that. What if get_instance() takes a second parameter that can be an array of method names, and all method names must exist for the class to be returned? Obviously we would not be able to add a method to the interface or as an abstract method to the base class, because that would cause fatal errors. They would be frozen in time. Fun times.

With regards to what this was based on, WP_Http, we won't need to add another "method" there. It's simple — we send an HTTP request. But I could absolutely see other image editing operations we might want to add in the future.

comment:85 follow-up: scribu19 months ago

What if get_instance() takes a second parameter that can be an array of method names, and all method names must exist for the class to be returned?

That could work, as long as the second parameter is optional.

comment:86 in reply to: ↑ 85 DH-Shredder19 months ago

Replying to scribu:

What if get_instance() takes a second parameter that can be an array of method names, and all method names must exist for the class to be returned?

That could work, as long as the second parameter is optional.

That would also allow us to choose specific editors for a specific task, which I can see being useful.

comment:87 ryan19 months ago

In [22192]:

Check for WP_Error return from WP_Image_Editor::get_instance().

Props markoheijnen
see #6821

markoheijnen18 months ago

Return error when no editor is selected

comment:88 markoheijnen18 months ago

Seems I forgot the most important one. WP_Image_Editor::get_instance() should return an error when no implementation could be selected.

comment:89 ryan18 months ago

In [22243]:

Return WP_Error when no editor is selected. Props markoheijnen. see #6821

comment:90 bpetty18 months ago

Seems like the filter "wp_editors" is very generic of a name for filtering the image editing backend used. Could this be more descriptive like "wp_image_editors" at least?

comment:91 scribu18 months ago

+1 for 'wp_image_editors', but is there a use-case for it that isn't covered by 'image_editor_class' ?

comment:92 bpetty18 months ago

  • Cc bpetty added

comment:93 bpetty18 months ago

I recently made it around to installing php5-imagick on my local testing environment, and have actually noticed that it's actually much slower at batch uploading the same set of images compared to using GD. I haven't done any profiling yet and don't have any numbers, but I wanted to see if anyone has already collected some numbers regarding speed and memory usage now, and if there's also some specific settings and factors I should be considering when running tests or configuring WordPress.

On an unrelated note, ImageMagick has already come ahead of GD in regards to #13461

comment:94 matt18 months ago

We found the same slower performance from IM on WordPress.com, which is why we never switched to it. We're now running GMagick in production though.

bpetty18 months ago

comment:95 bpetty18 months ago

(TLDR - Performance looks good with ImageMagick)

I performed some pretty thorough XHProf tests, and generally speaking, it looks like the IM backend does consistently still perform both faster than GD, and uses slightly less memory when handling both JPEG and PNG images. It turns out that it's only actually slower when working with GIF images (which is what I was working on with #13461), but even in the case of GIF images, it's doesn't use more memory, and it's not significantly slower (typically by about 15%).

I've uploaded a couple of the results specifically with a 10 megapixel JPEG photo:

In all of the tests, IM always took slightly longer to read in the image and initialize than GD did, but you can also see that the Imagick instance is only created once per upload (not once per thumbnail size), so there's nothing to optimize there either.

bpetty18 months ago

comment:96 follow-up: bpetty18 months ago

Please see 6812-fix-wp-error.patch for a quick fix to missing "new" calls for WP_Error usage in WP_Image_Editor_GD.

comment:97 ryan18 months ago

In 22421:

Call Imagick::queryformats() non-statically to preserve back compat with older versions of Imagick.

Props mpvanwinkle77
fixes #22308
see #6821

comment:99 in reply to: ↑ 96 bpetty18 months ago

6812-fix-wp-error.patch was applied in r22420. (nacin just referenced the wrong ticket in the commit)

comment:100 nacin18 months ago

What is left here?

comment:101 bpetty18 months ago

I think there's still some work on the roadmap for unit test improvements, but nothing in regards to core that I am aware of or can see on this ticket.

markoheijnen18 months ago

No caching and always run test

comment:102 markoheijnen18 months ago

The latest patch are thechanges that are still open. No caching anymore because of require methods option. The patch isn't tested yet,

The unit tests do need some work and I will trying to get that done tonight if possible. That is the reason why patch isn't tested yet

ryan18 months ago

Pass real attachment ID to _load_image_to_edit_path() from wp_crop_image()

comment:103 ryan18 months ago

I'm trying to get this working with image replication plugins where the image does not necessarily exist on disk. 6821.11.diff fixes a bug in wp_crop_image() where the attachment id in src_file was being stomped before being passed to _load_image_to_edit_path(). With this fixed $src_file is now a url to the image. However, the load() methods for WP_Image_Editor_GD and WP_Image_Editor-Imagick do the following:

if ( ! is_file( $this->file ) )
			return new WP_Error( 'error_loading_image', __('File doesn’t exist?'), $this->file );

This breaks environments that rely on url fopen to deal with images that only exist remotely.

comment:104 ryan18 months ago

With those checks commented out the image seems to be cropped and saved correctly (testing this with custom headers), but this warning is generated:

Warning: imagejpeg() [<a href='function.imagejpeg'>function.imagejpeg</a>]: Unable to open 'http://test.files.wordpress.com/2009/05/cropped-3470555880_4a2e9c8625_b.jpg' for writing: No such file or directory in /.../wp-includes/class-wp-image-editor.php on line 259

comment:105 ryan18 months ago

In 22510:

WP_Image_Editor improvements.

  • Make test() and supports_mime_type() static.
  • Add required_methods argument to get_instance(). Allows requesting an implementation that has certain methods/capabilities.
  • Whitespace cleanup

Props markoheijnen
see #6821

ryan18 months ago

Allow URLs

duck_18 months ago

comment:106 duck_18 months ago

In 22511:

Minor WP_Image_Editor documentation fixes. See #6821.

comment:107 ryan18 months ago

In 22538:

Pass an attachment ID, not a file path, to _load_image_to_edit_path() from wp_crop_image(). This fixes handling of attachments that require url fopen to access the image.

Allow passing urls instead of just file paths to WP_Image_Editor_Imagick::load() and WP_Image_Editor_GD::load() so that attachments requiring URL fopen can be handled.

see #6821

comment:108 nacin17 months ago

In 22549:

Revert [22421]. see #6821. see #22308. see #22419.

ryan17 months ago

In wp_crop_image(), preserve both $src_file and $src.

comment:109 ryan17 months ago

6821.13.diff​ fixes all issues I've seen so far with url fopen compat in the custom header workflow. $src_file needs to be preserved as the attached file name even when url fopen is used so that we don't try to save to a filename based on the url.

Todo:

Add wp_crop_image() unit tests.

Consider adding URL handling to WP_Image_Editor::generate_filename(). Saving to a URL generates warnings like "failed to open stream: HTTP wrapper does not support writeable connections". generate_filename() could detect that ->file is a URL and instead save to wp_upload_dir().

ryan17 months ago

Unit test fixes

ryan17 months ago

Basic wp_crop_image() tests.

comment:110 ryan17 months ago

In 22553:

In wp_crop_image(), preserve both src_file and src. src_file must be preserved even when url fopen is used so that we don't try to save to a filename based on the url.

see #6821

DH-Shredder17 months ago

src_abs Docs Fixes

comment:112 DH-Shredder17 months ago

Some more docs fixes attached.

comment:113 DH-Shredder17 months ago

Had a few comments from others regarding their dislike for removing the abstract class definitions in [22510]

The arguments amounted to a few things:

  • We're no longer enforcing the proper parameters on the required methods for default editors
  • The definition of an "editor" is a lot less strict, calling into question if there's too much flexibility in this model
  • New users of WP_Image_Editor won't have auto-complete or PHPDoc for the new default methods

It's been noted that while we can't add abstract classes in the future (which was part of the reason for the change) that we *can* remove them in the future, giving us time for a perhaps better solution.

I'm on the fence on this -- I think that the current setup gives a ton more flexibility to plugin developers (in terms of not having to create an entire Image Editor for a basic feature), but the above criticisms are valid.

Questions, comments, concerns? Maybe those concerned can pop in and expand if I misrepresented your argument?

comment:114 rachelbaker17 months ago

  • Cc rachelbaker added

DH-Shredder17 months ago

Re-add Abstract methods. Add lots of PHPDoc, and PHPDoc Fixes.

comment:115 DH-Shredder17 months ago

Attached a patch that re-adds the abstract methods, and includes a number of PHPDoc additions and fixes.

The consensus (per markoheijnen, kurtpayne, nacin, and I) was that we are not enforcing enough structure in the current code.
We can always move back to the way the code is currently (without abstract methods), but will not have a chance to enforce the structure if we don't do so now.

The patch includes the PHPDoc fixes previously submitted under 6821.docs.src_abs.diff​​.

comment:116 ryan17 months ago

In 22619:

Add abstract methods back to WP_Image_Editor and refresh phpdoc.

Props DH-Shredder, markoheijnen, kurtpayne, nacin
see #6821

comment:117 follow-up: scribu17 months ago

Why is load() a required method? The point at which the file is loaded into memory (if it's a separate operation at all) seems like an implementation detail to me, not something which the caller should be concerned with.

comment:118 in reply to: ↑ 117 DH-Shredder17 months ago

Replying to scribu:

Why is load() a required method? The point at which the file is loaded into memory (if it's a separate operation at all) seems like an implementation detail to me, not something which the caller should be concerned with.

That's how we handle the issue of receiving WP_Error(s) on file load in get_instance(), since we can't receive the load status from the constructor.

comment:119 nacin17 months ago

  • Owner set to DH-Shredder

I made a number of relevant comments here: #22356. Beyond that, what's left?

comment:120 alexvorn217 months ago

A new ticked relevant to this one - #22512

Last edited 17 months ago by SergeyBiryukov (previous) (diff)

comment:121 alexvorn217 months ago

  • Cc alexvornoffice@… added

comment:122 nacin17 months ago

Great work. New tickets for everything else, please. Most of the remaining issues have ended up in #22512.

comment:123 nacin17 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:124 nacin17 months ago

In 22817:

WP_Image_Editor: the last stand.

  • Have wp_get_image_editor() rather than WP_Image_Editor::get_instance(). Having static factory methods would be less confusing if there weren't also static methods tied to individual editor implementations.
  • Lazy-load the WP_Image_Editor base class and editor implementations.
  • Have WP_Image_Editor_GD::supports_mime_type() actually check which types it supports.
  • Deprecate gd_edit_image_support() in favor of wp_image_editor_supports().

props DH-Shredder, scribu, markoheijnen. fixes #22356. see #6821.

comment:125 macbrink17 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The filter 'wp_image_editor_class' now only returns the chosen class name. Plugins have no way in choosing another class if the path to the original image, or the mime type is not given.

please change filter to
$implementation = apply_filters( 'wp_image_editor_class', _wp_image_editor_choose( $args ), $args );

comment:126 scribu17 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

That's why we also have the 'wp_image_editors' filter.

Also, as nacin said above, please open new tickets for further issues.

comment:127 scribu17 months ago

Actually, I thought $args was passed to 'wp_image_editors', but that's not the case.

Anyway, I opened a dedicated ticket for this issue: #22538

DH-Shredder17 months ago

Fix multi_resize docs to describe array properly. Fix one mention of imagecopyresampled().

comment:128 nacin17 months ago

In 23038:

Image editor doc fixes. props DH-Shredder. see #6821.

Note: See TracTickets for help on using tickets.