Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 3 years ago

#33642 closed task (blessed) (fixed)

Improve default Imagick compression settings

Reported by: joemcgill's profile joemcgill Owned by: joemcgill's profile joemcgill
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Media Keywords:
Focuses: Cc:

Description (last modified by kirasong)

This ticket is a placeholder for patches of proposed changes to Imagick settings in the RICG feature plugin.

The plugin's Imagick settings reduce file size by up to 50%.

These changes are being considered separately for merge because during development, it was found that the current methods require too much RAM usage for some shared hosts.

Tests for compared CPU and memory usage required for consideration/approval.

Details on optimal settings can be found in @dnewton's Efficient Image Resizing With ImageMagick.

Attachments (11)

33642.patch (584 bytes) - added by joemcgill 9 years ago.
Change default quality to 82
33642.2.patch (7.9 KB) - added by joemcgill 9 years ago.
add custom thumbnailImage() method
33642.3.patch (6.3 KB) - added by joemcgill 9 years ago.
add _resize() method
33642.4.patch (7.6 KB) - added by joemcgill 9 years ago.
33642.5.patch (7.4 KB) - added by joemcgill 9 years ago.
33642.6.patch (7.9 KB) - added by kirasong 9 years ago.
33642.7.patch (7.7 KB) - added by joemcgill 9 years ago.
33642.8.patch (605 bytes) - added by kirasong 9 years ago.
Fix typo in strip_meta() Exception handling.
33642.9.patch (4.0 KB) - added by joemcgill 9 years ago.
33642.10.patch (6.8 KB) - added by joemcgill 9 years ago.
33642.11.patch (8.2 KB) - added by kirasong 9 years ago.

Download all attachments as: .zip

Change History (74)

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

#2 @kirasong
9 years ago

  • Description modified (diff)
  • Owner set to DH-Shredder
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


9 years ago

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


9 years ago

#7 @wonderboymusic
9 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#8 @kirasong
9 years ago

  • Milestone changed from Future Release to 4.5
  • Owner changed from mikeschroder to joemcgill

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


9 years ago

#10 follow-up: @jared_smith
9 years ago

While I know you're focused on ImageMagick at the moment, I thought it would also be a good time to mention that GraphicsMagick is a binary compatible fork of ImageMagick, but is much more efficient than ImageMagick. It also happens to be what big sites like Flickr and Etsy use.

Anyhoo, just thought I'd mention it.

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


9 years ago

This ticket was mentioned in Slack in #feature-respimg by mike. View the logs.


9 years ago

This ticket was mentioned in Slack in #core-images by markoheijnen. View the logs.


9 years ago

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

#15 @kirasong
9 years ago

It seems like for this ticket to move forward, the most important bit is to separate out the changes proposed by @dnewton, profile them, and decide which ones make sense to core. Then, build a core-appropriate patch for them, since unsure whether we want to extend Imagick or not (and that's how it is done in the plugin).

On my initial tests, posterization was the majority of the processing time.
The rest of the items don't seem to significantly increase CPU time/cause breakage in tests so far.

#16 in reply to: ↑ 10 @kirasong
9 years ago

Replying to jared_smith:

I thought it would also be a good time to mention that GraphicsMagick is a binary compatible fork of ImageMagick, but is much more efficient than ImageMagick. It also happens to be what big sites like Flickr and Etsy use.

The reason we're not using Gmagick is that it's not a popular (currently, anyway) PHP module available across hosting providers. I'd be open to the idea of optionally instantiating a Gmagick object (when available) if it can be shown to be "compatible enough." However: Even within the Imagick/Imagemagick versions, though, there is so much variety in compatibility of functions and behaviors that we'd likely end up with a separate WP_Image_Editor for very few users' benefit.

In which case it'd make the most sense to use the Gmagick Plugin.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

#18 @markoheijnen
9 years ago

I'm wondering if host using graphicsmagick-imagemagick-compat. Then they would be able to support both I guess.

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


9 years ago

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


9 years ago

@joemcgill
9 years ago

Change default quality to 82

#22 @joemcgill
9 years ago

  • Keywords has-patch added; needs-patch removed

33642.patch simply changes the default quality setting in WP_Image_Editor from 90 to 82, which is the quality setting for ImageMagick suggested by @dnewton in his Smashing Magazine article to closely match the output of Photoshop's 'high' quality setting for JPEG images.

After experimenting with his suggested settings, I noticed that the most noticeable difference in file size came from changing this quality setting, while many of the settings resulted in no change in file size.

I resized several different images using this setting and compared the image quality using Kornel Lesiński's implementation of DSSIM to calculate perceived differences between the current default quality and images compressed using a quality of 82. I found both Imagick and GD produced DSSIM scores between 0.0014 and 0.0023 in my test cases, which is well under the score (0.015) shown to be acceptable in research released in 2014 (requires signup to view, summary here).

Further enhancements to compression is possible (e.g., stripping some metadata, playing with different compression algorithms, etc.) but changing the quality alone would be a nice enhancement.

@joemcgill
9 years ago

add custom thumbnailImage() method

#23 @joemcgill
9 years ago

33642.2.patch implements most of the custom compression settings for JPEGs from the RICG Responsive Images Plugin by adding a custom implementation of Imagick's thumbnailImage() method so that we can pass the Imagick::FILTER_TRIANGLE filter to the internal resizeImage() method, which is not possible through the default Imagick implementation (see related Github issue).

I've separated the functions that strip image profiles out to its own strip_profiles() method, which could be used by itself alongside the current scaleImage() method for resizing images.

#24 @menkom
9 years ago

Hi Joe i agree with you, 92 is way to high, there is noticeable gain in compression by only 10 points and very little difference in image loss.

But instead of just having a default image compression which may or may not suite every person, i propose to also have a simple slide style option in admin to set your own custom compression say between 50-90, i wouldn't go below 50 as that will just render the images useless, and above 90-100 will increase file size unnecessarily.

Thanks and i hope my input is evaluated.

#25 follow-up: @simonrcodrington
9 years ago

I'm up with setting the default quality closer towards 80. The difference between 90 and 80 in regards to size is pretty impressive considering the quality difference is negligible.

On a similar topic (and discussed in this other post: https://make.wordpress.org/core/2016/02/22/proposal-increase-the-default-image-compression-in-wordpress/#comments) I'm not sure why this can't be a setting in the "media" options area, along with the other media options?

Some people are freaking out, saying that users shouldn't have to worry about this and other "technical" elements. My experience with our clients is that options which directly make sense (such as image sizs, number of blog items to show etc) can be easily managed on a client by client basis.)

Something as simple as a range slider and everyone is happy. We could even have a description under it with a sample image being compressed with live feedback so they can see "ohh that's what compression will do to my site". I'm always up for options which are useful and provide higher levels of control.

#26 @kirasong
9 years ago

In 36615:

Media: Reduce default image compression quality to '82'.

Changes default image compression quality from '90' to '82'.

This reduces generated image file sizes by ~25% while
keeping DSSIM < 0.0023, with both Imagick and GD.

Props @joemcgill, @dnewton.
See #33642.

#27 in reply to: ↑ 25 @joemcgill
9 years ago

Replying to simonrcodrington:

On a similar topic (and discussed in this other post: https://make.wordpress.org/core/2016/02/22/proposal-increase-the-default-image-compression-in-wordpress/#comments) I'm not sure why this can't be a setting in the "media" options area, along with the other media options?

Thanks for the feedback @simonrcodrington. While I agree that many users may find it helpful to set compression settings via an option on the media settings page, the trend has been to remove options and not add new ones as much as possible. I think it would make a great feature plugin for anyone who wants the ability to edit compression settings (or other hidden media settings) from the admin screen, rather than by code in a functionality plugin or a function.php file in a theme. A separate plugin would allow for more experimentation with different UI controls as well.

#28 follow-up: @menkom
9 years ago

@joemcgill "the trend has been to remove options and not add new ones as much as possible" that trend is probably debatable depending on who you talk to. For me i completely dont see a point in removing and not adding, i dont see how moving forward and removing are aligned ?

The only reason i see removing is an option is to remove bloat and to optimize more, however having to install a multitude of plugins also comes with it own caveats. If there is a plugin, it should be something supplied by the core team and not just for the image compression settings it should be something like a swiss army knife for an advanced features of WP as you mentioned.

Either that or there needs to be a SHOW ADVANCED OPTIONS within admin somewhere that unlocks these features that way you satisfy both parties.

#29 @kirasong
9 years ago

If we want to do more in 4.5, I’d prefer to see an initial version of thumbnailmage() land as soon as it can (ideally before beta), so it can get tested appropriately.

First glance at 33642.2.patch:

  • Check to be sure we're not duplicating existing resize logic in WordPress (e.g. image_resize_dimensions()).
  • The image object, rather than WP_Image_Editor needs to be checked in method_exists( $this, 'deleteImageProperty' ).
  • thumbnailImage() and strip_profiles() are probably better protected, to avoid plugins calling them directly.

#30 @kirasong
9 years ago

In 36616:

Media: Update unit tests after change to default image quality.

Updates unit tests to reflect new default quality setting of '82' after [36615].

See #33642.

#31 in reply to: ↑ 28 @kirasong
9 years ago

Re: @menkom @simonrcodrington

I don't think it is a good idea to add a UI for default compression quality.

To me, this is an area where the WordPress philosophy of “Decisions, not Options” is pretty clear. End users should not have to worry about the technical details of this default. It’s up to us as a community to do the research (as @dnewton, @joemcgill, and others have) and make a decision that’s appropriate for the majority of sites, so that users don't need to.

Plugins can (and do) modify this value or present a UI as they would like.

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

@joemcgill
9 years ago

add _resize() method

#32 @joemcgill
9 years ago

33642.3.patch reworks 33642.2.patch by adding a new protected _resize() method to WP_Image_Editor_Imagick that is used in both the resize() and crop() methods in place of Imagick::scaleImage().

The _resize() method is more efficient than the basic Imagick::resizeImage() method in that it:

  1. Strips profile and metadata from the image (except for color profiles) before resizing.
  2. Resamples really large images to 5x the output size before resizing (when applicable).
  3. Resizes the image using the Imagick::resizeImage() method with the Imagick::FILTER_TRIANGLE
  4. Updates several quality settings after the resize has happened.

In my benchmarks, this is performing well and is achieving an average of 60% file reduction over previous defaults with average DSSIM scores of 0.0019 compared with previous defaults.

#33 @kirasong
9 years ago

Thanks @joemcgill!

Took a look at 33642.3.patch; some feedback:

  • If we're going to have another function for resizes (which I agree is the cleanest solution), I prefer the naming for thumbnail_image(), since that it is protected is already clear, even without a preceding _.
  • Would like to see strip_meta() marked protected because there's not an analogue for GD. We can have a filter for stripping this data, though, as long as it's clear that it is effective only for Imagick in docs.
  • Need feature/function detection for: unsharpMaskImage(), getImageAlphaChannel(), setImageDepth(), setInterlaceSchema(), setColorspace(). Can check in test() if the compatibility goes back as far as 2.2, but safest is to feature check directly in the thumbnailing function, since these aren't "required" for resizes to work.
  • Unless there's a reason I'm missing, I'd prefer to not explicitly set color space and image depth to arbitrary values. If depth is getting set higher than the original (which looks to be the case in #30402), then we can detect and use the same bit depth. Would like to respect the original image's color profile/space/bit depth whenever possible.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


9 years ago

#35 follow-up: @menkom
9 years ago

@joemcgill What sort of meta data specifically gets stripped are you talking exif ? or something else

@joemcgill
9 years ago

#36 @joemcgill
9 years ago

33642.4.patch renames the _resize() method from 33642.3.patch to thumbnail_image().

I've added feature detection for getimagedepth(), setimagedepth(), setinterlacescheme(), and unsharpmaskimage() to in the test() method and a conditional check for the Imagick::getImageAlphaChannel() method before using it since it's not available on older versions ImageMagick and the functionality is not a requirement for the editor to work.

This version no longer sets an sRGB colorspace, since retaining colors is one of the benefits of using ImageMagick over GD.

I've kept the limit to 8bits on resized files since this is only being applied to our resized images and seems to be the default way a few image editors (including Photoshop) handle optimizing images for web (even PNG24s). However, this patch checks to see if the bit depth is greater than 8 before applying the bit depth limit so we don't increase the bit depth of grayscale images that are only using 2–4 bits per channel.

I've also added a new filter, image_strip_meta to override the stripping of image profiles and properties from resized images. This filter will only apply to sites using Imagick since GD always strips profiles/properties from images when resizing.

Last edited 9 years ago by joemcgill (previous) (diff)

@joemcgill
9 years ago

#37 @joemcgill
9 years ago

33642.5.patch is the same as above but adds a feature test for the Imagick::setimageproperty() and removes an unnecessary whitespace change from 'src/wp-includes/class-wp-image-editor.php' which had snuck into the other patches.

I also added some clarifying inline docs for the step in thumbnail_image() that sets undefined alpha channels to opaque, which doesn't apply to most images, but can be a nice file savings with no visual difference when applicable.

#38 in reply to: ↑ 35 @joemcgill
9 years ago

Replying to menkom:

@joemcgill What sort of meta data specifically gets stripped are you talking exif ? or something else

The current strip_meta() method will remove all image profiles except for ICC and ICM data from resized images, so EXIF or other vendor profiles would be removed. However, any EXIF or other data set as an image property would remain (which seems common in my tests). The specific list of image properties that we explicitly remove in this step include:

  • comment
  • Thumb::URI
  • Thumb::MTime
  • Thumb::Size
  • Thumb::Mimetype
  • software
  • Thumb::Image::Width
  • Thumb::Image::Height
  • Thumb::Document::Pages

The current patches propose that we include a filter to override this behavior, with the assumption that most users would benefit from the reduced file size and sites that require keeping all image metadata on resized images could override this setting fairly easily.

The original, full-size images that are uploaded are not affected by this process.

@kirasong
9 years ago

#39 @kirasong
9 years ago

In 33642.6.patch:

  • Check result in each case with thumbnail_image() & strip_meta(), since they can return WP_Error.
  • Make sure that we have at least one filter defined in test()

Open, but could happen after first pass:

  • Don't use any local functions inside try-catch block
  • Make sure custom filter is defined before using

#40 @joemcgill
9 years ago

33642.7.patch adds sanity checks for Imagick filter constants before setting our filter value in thumbnail_imamge(). I also decided to simplify the naming scheme for those filters so we just use the same name as the constant.

This ticket was mentioned in Slack in #core by mike. View the logs.


9 years ago

@joemcgill
9 years ago

#42 @kirasong
9 years ago

In 36700:

Media: Optimize Imagick settings for quality and filesize

  • Resamples and sharpens larger images before resize.
  • Uses imagick::FILTER_TRIANGLE for a smoother resize.
  • Introduces WP_Image_Editor_Imagick::thumbnail_image() protected method to efficiently resize images. Similar to the functionality of Imagick's thumbnailImage().
  • Introduces WP_Image_Editor_Imagick::strip_meta() protected method and image_strip_meta filter that, by default, strip image profiles to reduce file size, while leaving color profiles intact.

See: #33642, #30402, #28634.
Props: joemcgill, dnewton, mikeschroder.

#43 @kirasong
9 years ago

  • Keywords has-patch removed
  • Type changed from enhancement to task (blessed)

This ticket was mentioned in Slack in #core-images by mike. View the logs.


9 years ago

#45 @kirasong
9 years ago

In 36742:

Media: Correct "Exception" typo in WP_Image_Editor_Imagick::strip_meta().

Exceptions are caught better if they're not excpeted.

Props joemcgill.
See #33642.

@kirasong
9 years ago

Fix typo in strip_meta() Exception handling.

#46 follow-up: @summoner
9 years ago

Hello guys, have to confess it is not 100% clear for me whether you are planning to implement a slider or some UX component to let the users change the default 80%. In some cases image quality is simply vital even if it consumes more bandwith.

Furthermore once i had a site to sell goods. Images were generated with GD2 at the beginning. In case of normal-sized images it was good enough but below 200px the generated images tended to loose on sharpness. A lot. Even when GD was set to 100% quality... That was why i changed to imagick. Have to admit i used 95% quality but even the tiniest thumbnail was cristal clear that way. Even so filesizes were smaller than that of GD2-generated thumbs.

So all i want to point on that thumbnail image sizes might be a bit more tricky than normal sized images, and you can not spare much bandwith on thumbnails. That is why i think maybe you could set a higher default quality for images that are smaller than 200px or so.

Last edited 9 years ago by summoner (previous) (diff)

#47 in reply to: ↑ 46 ; follow-up: @markoheijnen
9 years ago

So I looked over the code and my note would be to move the strip_meta() call from thumbnail_image(). For example currently it doesn't work if you only flip an image. Also as mentioned in #28634 I would make strip_meta public. I would suggest a property in WP_Image_Editor that could be set with set_strip_meta(). Even for GD it could be set to false and we could do something like Custom GD Editor

Replying to summoner:

Hello guys, have to confess it is not 100% clear for me whether you are planning to implement a slider or some UX component to let the users change the default 80%. In some cases image quality is simply vital even if it consumes more bandwith.

That is not the plan since there are already plugins out there doing this. The percentage who want to change the quality isn't that high for WordPress to integrate it in core.

#48 in reply to: ↑ 47 @joemcgill
9 years ago

Replying to markoheijnen:

So I looked over the code and my note would be to move the strip_meta() call from thumbnail_image(). For example currently it doesn't work if you only flip an image. Also as mentioned in #28634 I would make strip_meta public.

Thanks for the feedback @markoheijnen. Having the strip_meta() functionality only run when images are resized and not when an image is modified in another way was somewhat intentional in that the idea was to mimic Imagick's thumbnailImage() functionality, which strips metadata when the image is resized. While I think it's probably an acceptable optimization for most sites that we wouldn't save all the metadata on our custom image sizes, I wouldn't want to strip metadata from originals when other edits are applied (at least by default).

I'm not opposed to making it a public function, which is why I initially separated it out into its own method originally. I also think there are probably enhancements that could be made to the internals of the strip_meta() method based on the discussion from #28634. I think the key question there is to decide which profile types should be protected and we should also allow users to filter that list.

#49 @markoheijnen
9 years ago

Currently the strip functionality is to vague since you need to guess when strip_meta() is being used or not. So if a plugin only want to flip an image but does want to stip the meta data then it needs to do a resize to make that happen.

Also your reasoning is incorrect since the original is always untouched from what core does. When you use the editor then a 2nd set of images are being created.

I don't think we should allow users to filter the list. It's everything or nothing (color profiles as exception). If they want specifics then they need to build their own custom editor.

#50 @kirasong
9 years ago

From a chat with @joemcgill, two things stand out as left here:

  • Move requirements for stripping/advanced resize out of test() so that we don't reduce support for Imagick
  • Decide whether we should fail silently if strip_meta() fails, so that an image doesn't fail resizes due to lack of proper ImageMagick support for the installed version of Imagick.

#51 @markoheijnen
9 years ago

When I was looking at how to move the strip_meta() functionality, I also removed returning an WP_Error when strip_meta() fails. It doesn't need to be there. It's a bonus if it works.

#52 @DrewAPicture
9 years ago

In 36846:

Docs: Improve the hook doc summary for the image_strip_meta filter, introduced in [36700].

See: #33642, #30402, #28634. See #35986.

#53 @DrewAPicture
9 years ago

In 36847:

Docs: Improve DocBlock syntax and add a missing @return notation for WP_Image_Editor_Imagick::strip_meta(), introduced in [36700].

See: #33642, #30402, #28634. See #35986.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

@joemcgill
9 years ago

#55 @joemcgill
9 years ago

33642.9.patch returns WP_Image_Editor_Imagick::test() to it's pre 4.5 state and moves feature tests for new optimizations down to where they are used so each optimization can be applied as a progressive enhancement without changing requirements for anyone who is currently using Imagick.

This also falls back to the original scaleImage() method for resizing images if Imagick's resizeImage() method isn't available or if an appropriate filter constant is not set, since the default scaleImage() method produces better results in terms of image quality over resizeImage() when no filter is available.

@joemcgill
9 years ago

#56 @joemcgill
9 years ago

33642.10.patch Fixes some mistakes in 33642.9.patch and switches to using is_callable() for feature checking our optimizations instead of method_exists(). The test() method should now be identical to how it was in 4.4.

This patch also includes the improvements to the strip_meta() method from 28634.7.diff (see #28634) which strips all image profile data from resized images, except the following:

  • 'icc' – Color profiles
  • 'iptc' – Copyright info
  • 'exif' – Mainly for orientation data, but all other Exif data would also be maintained.
  • 'xmp' – Rights usage data

In the future, we may want to be strip more metadata by default or allow for filtering of individual properties. For now sites can use the new image_strip_meta filter to turn the stripping functionality off there is other data that needs to be maintained.

#57 @markoheijnen
9 years ago

Patch looks fine.

The only thing I would change is to move the is_callable( array( $this->image, 'removeImageProfile' ) ) check outside the foreach loop and to start with that in strip_meta(). You probably also want to check for getImageProfiles. But then again, they are provided at the same version of Imagick.

@kirasong
9 years ago

#58 @kirasong
9 years ago

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

In 36891:

Media: Progressive enhancement for Imagick; add profiles to whitelist.

  • Progressive enhancement for optional compression improvements and stripping meta.
  • Whitelist IPTC and XMP profiles to maintain Copyright and Rights Usage Terms.
  • Whitelist EXIF profile to maintain orientation information. If handled on upload in the future, it can be stripped as well.

Fixes #33642. See #28634.
Props joemcgill, juliobox, ahockley, markoheijnen, adamsilverstein, wonderboymusic, mikeschroder.

#59 @ocean90
9 years ago

In 36968:

Media: Merge two error messages and use sprintf() for the method names.

See #33642.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

#61 follow-up: @awrmales
8 years ago

Is the only (practical) way to change the ImageMagick resize resampling filter to subclass the image editor? The default triangle filter causes our graphical PNG images to be blurry compared to those created before this change.

#62 in reply to: ↑ 61 @virgodesign
8 years ago

Replying to awrmales:

Is the only (practical) way to change the ImageMagick resize resampling filter to subclass the image editor? The default triangle filter causes our graphical PNG images to be blurry compared to those created before this change.

I've opened a new ticket #40415 related to this enhancement request.

Last edited 8 years ago by virgodesign (previous) (diff)

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.