Make WordPress Core

Opened 16 years ago

Closed 11 years ago

Last modified 11 years ago

#6821 closed task (blessed) (fixed)

ImageMagick support

Reported by: matt's profile matt Owned by: kirasong's profile kirasong
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 12 years 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 kirasong 12 years ago.
Initial Suggestion for Interface/Class for Image Manipulation
benchmark_imagick.php (692 bytes) - added by lbr 12 years ago.
benchmark script using imagick exec|extension and graphicsmagick extension
6821.patch (21.0 KB) - added by markoheijnen 12 years ago.
First try to implement my class on github
ImagickHostAvailability.md (1.6 KB) - added by kirasong 12 years ago.
Initial Host ImagicK Availability [edit: unrelated notes removed]
6821.diff (43.2 KB) - added by kirasong 12 years ago.
Initial Abstraction of GD with WP_Image_Editor
6821.initialImagickSupport.diff (46.7 KB) - added by kirasong 12 years ago.
Initial Imagick support (module)
6821.2.diff (46.8 KB) - added by kirasong 12 years ago.
Includes fixes to Notices in Custom Header
6821.3.diff (46.3 KB) - added by kirasong 12 years ago.
Updated version for chat. To be edited further based on decisions made.
6821.4.diff (46.2 KB) - added by kirasong 12 years 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 12 years ago.
6821.6.diff (47.8 KB) - added by kirasong 12 years ago.
Now with Improved Exception Handling and more WP_Error than ever before!
6821.7.diff (48.3 KB) - added by kirasong 12 years ago.
Passes Unit Tests. On-Load Error Handling Improvements. Better i18n on Errors.
6821.8.diff (55.7 KB) - added by kirasong 12 years 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 kirasong 12 years ago.
Previous patch, plus make $implementation a static property for unit-testness
6821-unit-tests-4.patch (20.5 KB) - added by kurtpayne 12 years ago.
First round of unit tests
6821.9.2.diff (55.8 KB) - added by kirasong 12 years 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 12 years ago.
Adding realpath to be compatible with changes in r22008
6821.9.3.diff (57.4 KB) - added by kirasong 11 years ago.
Refresh, and more docs.
6821.check-errors.diff (2.1 KB) - added by markoheijnen 11 years ago.
Added and fix error checks
6821.fix-strict-standards.diff (2.4 KB) - added by markoheijnen 11 years ago.
6821.check-errors2.diff (377 bytes) - added by markoheijnen 11 years ago.
Return error when no editor is selected
xhprof-gd-jpg-10mp.png (209.9 KB) - added by bpetty 11 years ago.
xhprof-imagick-jpg-10mp.png (337.9 KB) - added by bpetty 11 years ago.
6812-fix-wp-error.patch (1.3 KB) - added by bpetty 11 years ago.
6821.10.diff (8.8 KB) - added by markoheijnen 11 years ago.
No caching and always run test
6821.11.diff (1.1 KB) - added by ryan 11 years ago.
Pass real attachment ID to _load_image_to_edit_path() from wp_crop_image()
6821.12.diff (2.2 KB) - added by ryan 11 years ago.
Allow URLs
6821.doc-fixes.diff (1.7 KB) - added by duck_ 11 years ago.
6821.13.diff (706 bytes) - added by ryan 11 years ago.
In wp_crop_image(), preserve both $src_file and $src.
6821-ut.diff (591 bytes) - added by ryan 11 years ago.
Unit test fixes
6821-ut.2.diff (2.2 KB) - added by ryan 11 years ago.
Basic wp_crop_image() tests.
6821.docs.src_abs.diff (1.6 KB) - added by kirasong 11 years ago.
src_abs Docs Fixes
6821.reabstract.diff (10.3 KB) - added by kirasong 11 years ago.
Re-add Abstract methods. Add lots of PHPDoc, and PHPDoc Fixes.
more_image_editor_docs.diff (2.0 KB) - added by kirasong 11 years ago.
Fix multi_resize docs to describe array properly. Fix one mention of imagecopyresampled().

Download all attachments as: .zip

Change History (164)

#1 @jacobsantos
16 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.

#2 @Denis-de-Bernardy
15 years ago

  • Component changed from General to Upload

#3 @ryan
14 years ago

  • Milestone changed from 2.9 to Future Release

#4 @scribu
13 years ago

Marked #11671 as duplicate.

#6 @azaozz
13 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+).

#7 @scribu
13 years ago

Marked #18539 as duplicate.

#8 follow-up: @GaryJ
12 years ago

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

#9 @nacin
12 years 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.

#10 @markoheijnen
12 years 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.

#11 @nacin
12 years 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.

#12 @Japh
12 years ago

  • Cc japh@… added

#13 @tomauger
12 years ago

  • Cc tomaugerdotcom@… added

#14 @kirasong
12 years 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.

#15 @mikeschinkel
12 years ago

  • Cc mikeschinkel@… added

#16 @tomauger
12 years 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.

#17 @Japh
12 years 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.

#18 @tomauger
12 years 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.

@tomauger
12 years ago

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

#19 @scribu
12 years ago

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

#20 @scribu
12 years 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();

#21 @kirasong
12 years 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 12 years ago by kirasong (previous) (diff)

#22 @Mamaduka
12 years ago

  • Cc georgemamadashvili@… added

#23 @kuemerle5
12 years ago

  • Cc kuemerle5@… added

#24 @westi
12 years ago

  • Owner tellyworth deleted
  • Status changed from new to assigned

#25 @kieranmasterton
12 years ago

  • Cc kieranmasterton added

#26 @markoheijnen
12 years 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.

@kirasong
12 years ago

Initial Suggestion for Interface/Class for Image Manipulation

#27 follow-up: @kirasong
12 years 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?

#28 @Dartanjan
12 years ago

  • Cc Dartanjan added

#29 in reply to: ↑ 27 ; follow-up: @lbr
12 years 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...

#30 @Japh
12 years 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 12 years ago by Japh (previous) (diff)

#31 in reply to: ↑ 29 @kirasong
12 years 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.

#32 @kirasong
12 years 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.

#33 @dd32
12 years 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.

#34 @lbr
12 years 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...

#35 @markoheijnen
12 years 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.

#36 @dd32
12 years 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.

#37 follow-up: @lbr
12 years 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 12 years ago by lbr (previous) (diff)

@lbr
12 years ago

benchmark script using imagick exec|extension and graphicsmagick extension

#38 in reply to: ↑ 37 ; follow-up: @Japh
12 years 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?

#39 in reply to: ↑ 38 @lbr
12 years 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...

#40 @kurtpayne
12 years ago

  • Cc kpayne@… added

#41 in reply to: ↑ 8 ; follow-up: @scribu
12 years 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.

@markoheijnen
12 years ago

First try to implement my class on github

#42 @markoheijnen
12 years 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.

#43 in reply to: ↑ 41 @kirasong
12 years 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.

#44 @scribu
12 years 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 12 years ago by scribu (previous) (diff)

#45 follow-up: @scribu
12 years 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 12 years ago by scribu (previous) (diff)

#46 in reply to: ↑ 45 @Japh
12 years 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.

#47 follow-up: @scribu
12 years ago

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

#48 in reply to: ↑ 47 @Japh
12 years 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?

#49 @kirasong
12 years 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.

#50 follow-up: @scribu
12 years 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?

#51 in reply to: ↑ 50 @kirasong
12 years 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.

#52 follow-up: @scribu
12 years 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 12 years ago by scribu (previous) (diff)

#53 in reply to: ↑ 52 @kirasong
12 years 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.

#54 @mbijon
12 years ago

  • Cc mike@… added

#55 @scribu
12 years ago

Related: #21668

@kirasong
12 years ago

Initial Host ImagicK Availability [edit: unrelated notes removed]

#56 @kirasong
12 years ago

Initial Host availability attached.

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

#57 @Viper007Bond
12 years ago

  • Cc Viper007Bond added

#58 @kirasong
12 years 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.

#59 @Japh
12 years 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.

@kirasong
12 years ago

Initial Abstraction of GD with WP_Image_Editor

#60 @kirasong
12 years 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!

@kirasong
12 years ago

Initial Imagick support (module)

#61 @kirasong
12 years 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.

@kirasong
12 years ago

Includes fixes to Notices in Custom Header

#62 @kirasong
12 years 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 12 years ago by kirasong (previous) (diff)

@kirasong
12 years ago

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

#63 @scribu
12 years 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?

#64 @markoheijnen
12 years 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.

@kirasong
12 years ago

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

#65 @markoheijnen
12 years 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 12 years ago by markoheijnen (previous) (diff)

@markoheijnen
12 years ago

@kirasong
12 years ago

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

#66 @kirasong
12 years ago

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

Last edited 12 years ago by kirasong (previous) (diff)

#67 @prettyboymp
12 years ago

  • Cc mpretty@… added

@kirasong
12 years ago

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

#69 @kirasong
12 years 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.

#70 follow-up: @griffinjt
12 years ago

  • Version changed from 2.5 to trunk

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.

Last edited 12 years ago by griffinjt (previous) (diff)

#71 @SergeyBiryukov
12 years ago

  • Version changed from trunk to 2.5

#72 in reply to: ↑ 70 @kirasong
12 years 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

@kirasong
12 years ago

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

#73 @kirasong
12 years 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).

@kirasong
12 years ago

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

#74 @kirasong
12 years ago

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

@kurtpayne
12 years ago

First round of unit tests

@kirasong
12 years ago

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

@kurtpayne
12 years ago

Adding realpath to be compatible with changes in r22008

@kirasong
11 years ago

Refresh, and more docs.

#75 @ryan
11 years 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

#76 @ocean90
11 years ago

  • Cc ocean90 added

#77 @scribu
11 years ago

Related: #22073

#78 @navjotjsingh
11 years ago

  • Cc navjotjsingh@… added

#79 @Viper007Bond
11 years ago

Well done gentlemen!

#80 @ryan
11 years 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

#81 @nacin
11 years 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.

@markoheijnen
11 years ago

Added and fix error checks

#82 @markoheijnen
11 years ago

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

#83 @ryan
11 years ago

In [22119]:

Avoid Strict Standards warnings. Props markoheijnen. see #6821

#84 @nacin
11 years 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.

#85 follow-up: @scribu
11 years 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.

#86 in reply to: ↑ 85 @kirasong
11 years 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.

#87 @ryan
11 years ago

In [22192]:

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

Props markoheijnen
see #6821

@markoheijnen
11 years ago

Return error when no editor is selected

#88 @markoheijnen
11 years ago

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

#89 @ryan
11 years ago

In [22243]:

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

#90 @bpetty
11 years 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?

#91 @scribu
11 years ago

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

#92 @bpetty
11 years ago

  • Cc bpetty added

#93 @bpetty
11 years 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

#94 @matt
11 years 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.

#95 @bpetty
11 years 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.

#96 follow-up: @bpetty
11 years 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.

#97 @ryan
11 years ago

In 22421:

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

Props mpvanwinkle77
fixes #22308
see #6821

#99 in reply to: ↑ 96 @bpetty
11 years ago

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

#100 @nacin
11 years ago

What is left here?

#101 @bpetty
11 years 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.

@markoheijnen
11 years ago

No caching and always run test

#102 @markoheijnen
11 years 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

@ryan
11 years ago

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

#103 @ryan
11 years 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.

#104 @ryan
11 years 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

#105 @ryan
11 years 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

@ryan
11 years ago

Allow URLs

#106 @duck_
11 years ago

In 22511:

Minor WP_Image_Editor documentation fixes. See #6821.

#107 @ryan
11 years 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

#108 @nacin
11 years ago

In 22549:

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

@ryan
11 years ago

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

#109 @ryan
11 years 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().

@ryan
11 years ago

Unit test fixes

@ryan
11 years ago

Basic wp_crop_image() tests.

#110 @ryan
11 years 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

@kirasong
11 years ago

src_abs Docs Fixes

#112 @kirasong
11 years ago

Some more docs fixes attached.

#113 @kirasong
11 years 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?

#114 @rachelbaker
11 years ago

  • Cc rachelbaker added

@kirasong
11 years ago

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

#115 @kirasong
11 years 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​​.

#116 @ryan
11 years ago

In 22619:

Add abstract methods back to WP_Image_Editor and refresh phpdoc.

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

#117 follow-up: @scribu
11 years 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.

#118 in reply to: ↑ 117 @kirasong
11 years 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.

#119 @nacin
11 years ago

  • Owner set to DH-Shredder

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

#120 @alexvorn2
11 years ago

A new ticked relevant to this one - #22512

Last edited 11 years ago by SergeyBiryukov (previous) (diff)

#121 @alexvorn2
11 years ago

  • Cc alexvornoffice@… added

#122 @nacin
11 years ago

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

#123 @nacin
11 years ago

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

#124 @nacin
11 years 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.

#125 @macbrink
11 years 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 );

#126 @scribu
11 years 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.

#127 @scribu
11 years 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

@kirasong
11 years ago

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

#128 @nacin
11 years ago

In 23038:

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

Note: See TracTickets for help on using tickets.