Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#48842 new defect (bug)

Fix calculation error when resampling images before resizing in class-wp-image-editor-imagick.php

Reported by: azaozz's profile azaozz Owned by:
Milestone: Future Release Priority: high
Severity: normal Version: 4.5
Component: Media Keywords: 2nd-opinion needs-testing
Focuses: Cc:

Description

Seems there is a calculation error when resampling large images before resizing them to a much smaller sub-size. Looking at #33642 and [36700], the idea is to efficiently reduce the size of the original image before resizing by using ImageMagick's sampleImage().

However in some cases the resampled image may be larger than the original. Caused by doing the resampling when the destination image is about 1/3 of the size of the original, and using a (hard-coded) $sample_factor = 5, see https://core.trac.wordpress.org/browser/tags/5.3/src/wp-includes/class-wp-image-editor-imagick.php#L333.

Example: for an original image of 1000x500 and destination image of 300x150, the resampled source image will be 1500x750 (the $resize_ratio in this case is 0.09).

Attachments (1)

48842.diff (1.5 KB) - added by azaozz 4 years ago.

Download all attachments as: .zip

Change History (19)

#1 @azaozz
4 years ago

Looking at:

/*
 * To be more efficient, resample large images to 5x the destination size before resizing
 * whenever the output size is less that 1/3 of the original image size (1/3^2 ~= .111),
 * unless we would be resampling to a scale smaller than 128x128.
 */
if ( is_callable( array( $this->image, 'sampleImage' ) ) ) {
	$resize_ratio  = ( $dst_w / $this->size['width'] ) * ( $dst_h / $this->size['height'] );
	$sample_factor = 5;

	if ( $resize_ratio < .111 && ( $dst_w * $sample_factor > 128 && $dst_h * $sample_factor > 128 ) ) {
		$this->image->sampleImage( $dst_w * $sample_factor, $dst_h * $sample_factor );
	}
}

The reason the resampling is done is to make creating much smaller sub-sizes more efficient. It is still another "operation" on the source that takes time and server resources to do. If I understand the initial intentions correctly,

whenever the output size is less that 1/3 of the original image size

should be

whenever the resampled source size is less that 1/3 of the original image size

Then the $resize_ratio calculation should be done with $dst_w * $sample_factor and $dst_h * $sample_factor. This will also be inline with ...unless we would be resampling to a scale smaller than 128x128.

@azaozz
4 years ago

#2 @azaozz
4 years ago

  • Keywords has-patch 2nd-opinion needs-testing added
  • Priority changed from normal to high

In 48842.diff: fix the calculation in WP_Image_Editor_Imagick::thumbnail_image() when resampling an image source before resizing it to a much smaller size.

With this change:

  • Resizing a 1000x500 image to 300x150 doesn't resample the source. The resampled image would be larger than the original: 1500x750, $resize_ratio is 2.25.
  • Resizing a 3000x1500 image to 300x150 doesn't resample the source. The resampled image would be half the size of the original, $resize_ratio is 0.25.
  • Resizing a 4800x2400 image to 300x150 resamples the source. The resampled image is less than 1/3 the size of the original, $resize_ratio is 0.09765625.

This seems like the cause for #48522 where the sample image (10315px) is up-scaled to 12800px before resizing it to 2560px.

Setting to high priority as this needs to be reviewed and committed asap :)

Last edited 4 years ago by azaozz (previous) (diff)

#3 follow-up: @vanyukov
4 years ago

@azaozz,

I think it's a bit wrong, at least, from the original intention. Images are upscaled up to smooth out the edges during resizing. If we have a 5x larger image - just skip resample and resize, if we don't - resample the original and then resize.

Resizing a 1000x500 image to 300x150 doesn't resample the source. The resampled image would be larger than the original: 1500x750, $resize_ratio is 2.25.

Exactly the point - we should resample the original to 1.5x the size.

Also, tetsted the above fix, and these sizes fail:

  • Resizing a 10315x7049 image to 150x150 does resample the source. The $resize_ratio is 0.0077361660674977. No need to resample, we have a huge original we can resize from.
  • Resizing a 10315x7049 image to 300x205 does resample the source. The $resize_ratio is 0.021145520584494. Same thing.
  • Resizing a 10315x7049 image to 2100x1435 does not resample the source. The $resize_ratio is 1.0361305086402

In my initial proposal, I've suggested having a dynamic $sample_factor. It doesn't have to be with do...while, but just not resample all the images to 5x size.

For example, 2100x1435 is just a bit off from being 5x smaller than 10315x7049.
(10315x7049)-(2100x1435*25) = 2627065, so we can just resample the original to x1.01791 the size and the resize from that image. Does that make sense?

Last edited 4 years ago by vanyukov (previous) (diff)

#4 in reply to: ↑ 3 @azaozz
4 years ago

Replying to vanyukov:

I think it's a bit wrong, at least, from the original intention. Images are upscaled up to smooth out the edges during resizing. If we have a 5x larger image - just skip resample and resize, if we don't - resample the original and then resize.

Hmm, I read the comment there differently :)

"To be more efficient..." sounds like this is done in order to help efficiency, i.e. create a much smaller sub-size faster and using less server resources. Not sure if the idea was to upscale the original in order to produce "smoother" sub-size. The existing code suggests that too. Also think Imagick::thumbnailImage() does something similar internally.

The Imagick::sampleImage() seems like a "fast" scaling method: https://imagemagick.org/api/resize.php#SampleImage.

SampleImage() scales an image to the desired dimensions with pixel sampling. Unlike other scaling methods, this method does not introduce any additional color into the scaled image.

I'm sure @mikeschroder and @joemcgill will know more.

Last edited 4 years ago by azaozz (previous) (diff)

#5 @vanyukov
4 years ago

@azaozz , the "to be more efficient" was for resource handling. When debugging the issue with large sizes not being generated, I went back to a 4 or 5 years back discussion in trac and there was a reference to this article:
https://www.smashingmagazine.com/2015/06/efficient-image-resizing-with-imagemagick/

I can see that many image optimization settings are similar in WordPress, so I assumed a lot was taken from that article. Here's a quote:

This means that if we were resizing an image to be 500 pixels wide, -thumbnail would first resize it to 2,500 pixels wide using -sample; the result might be blocky and pixelated, as we saw in the examples above, but the operation would be fast and would produce a result with a small file size. Then, ImageMagick would resize this image from 2,500 pixels wide to 500 pixels wide using -resize. This smooths out the blockiness, but the file size stays pretty low.

#6 @vanyukov
4 years ago

Another observation - the upload process is really long. It's taking anywhere from 2 to 5 minutes on a test site. With WordPress 5.2.4 it was 30-40 seconds.

On a VVV install after 38+- seconds I get this UI notice: Post-processing of the image failed. If this is a photo or a large image, please scale it down to 2500 pixels and upload it again., async-upload.php fails with a 502 Bad Gateway and WebKitFormBoundary error.

#7 follow-up: @azaozz
4 years ago

  • Keywords has-patch removed
  • Milestone changed from 5.3.1 to 5.4

Replying to vanyukov:

...and there was a reference to this article:
https://www.smashingmagazine.com/2015/06/efficient-image-resizing-with-imagemagick/

I can see that many image optimization settings are similar in WordPress, so I assumed a lot was taken from that article.

Right, the enhancements in WP are (mostly) based on https://github.com/nwtn/php-respimg by the same author which seem based on the ImageMagick -thumbnail option: https://github.com/ImageMagick/ImageMagick/blob/master/MagickCore/resize.c#L4491.

Looking at the ImageMagick code, it also has a hard-coded scale factor of 5 but does the resampling for a bit smaller sub-sizes, $resize_ratio <= 0.1. This still upscales when images are resized to between 32% to 20% of the original.

On the other hand quickly testing this with the sample 10MB JPEG image in ImageMagick 7.0.9 (PHP 7.3), the sub-size images that were resized "directly" without the 5x scaling are either identical or a little smaller in file size, up to about 2% smaller. This will need a lot more testing but seems that ImageMagick perhaps handles JPEGs better in newer versions?

Thinking we should look at preventing the upscaling when using the above technique, at least for very large source images. Seems the "when to start scaling" value is set to a more or less a rule-of-thumb at about 1/3 of the original. Using that only when the required image is less than 20% of the original may make sense.

Also tested the edge case when the upscaling makes an image larger than the allowed image dimensions in the ImageMagick policy settings (see https://github.com/ImageMagick/ImageMagick/blob/master/config/policy.xml). This fails with "width or height exceeds limit" as expected.

Moving this to the 5.4 milestone as it seems to be the expected behaviour, and will need a lot more testing and considerations to change.

#8 in reply to: ↑ 7 @vanyukov
4 years ago

Replying to azaozz:

Looking at the ImageMagick code, it also has a hard-coded scale factor of 5 but does the resampling for a bit smaller sub-sizes, $resize_ratio <= 0.1. This still upscales when images are resized to between 32% to 20% of the original.

Yeah, looks like WordPress implementation is almost identical to the ThumbnailImage in ImageMagick.

On the other hand quickly testing this with the sample 10MB JPEG image in ImageMagick 7.0.9 (PHP 7.3), the sub-size images that were resized "directly" without the 5x scaling are either identical or a little smaller in file size, up to about 2% smaller. This will need a lot more testing but seems that ImageMagick perhaps handles JPEGs better in newer versions?

The difference is probably due to a different filter being used. It looks like the default one for ImageMagick is FILTER_MITCHELL, while WordPress is using FILTER_TRIANGLE

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


4 years ago

#10 @antpb
4 years ago

  • Milestone changed from 5.4 to 5.5

I saw in @azaozz ’s comment that this needs a bit of consideration and time. It was previously moved to the 5.4 milestone but with the fast approaching March 31 5.4 release, I’m going to move this out of the milestone. If anyone wants to own this and bring it across the line before then, feel free to bring it back into 5.4.

#11 @kirasong
4 years ago

Thanks so much for the work on this! I agree it's late to land for 4.5, unfortunately.

As a quick note, before it lands in core, in addition to time, I'd recommend testing file size and DSSIM between Current GD + Imagick, and the new proposed implementation, just to make sure we don't inadvertently regress on them.

In case it's helpful, there are some notes on the choice of FILTER_TRIANGLE here: https://core.trac.wordpress.org/ticket/33642#comment:32

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


4 years ago

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


4 years ago

#14 @antpb
4 years ago

  • Milestone changed from 5.5 to 5.6

Considering the work still pending here, I'm going to go ahead and move this to 5.6 as we are nearing Beta 1 of 5.5. Please feel free to move it back if this is incorrect. Thanks!

#15 @hellofromTonya
3 years ago

5.6 Beta 1 is quickly approaching (ie in 11 days). Gentle reminder and nudge to see if we have capacity to complete the pending work in this ticket for it land in 5.6.

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


3 years ago

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


3 years ago

#18 @antpb
3 years ago

  • Milestone changed from 5.6 to Future Release

Rather than kick this can down the road, we should probably more realistically milestone this for future release until someone has the bandwidth to do the testing still pending on this ticket in the comments above. If anyone feels like this should still be in 5.6, please feel free to move it back into the milestone.

Comment mentioned: https://core.trac.wordpress.org/ticket/48842#comment:11

Last edited 3 years ago by antpb (previous) (diff)
Note: See TracTickets for help on using tickets.