Make WordPress Core

Opened 13 years ago

Closed 9 years ago

Last modified 7 years ago

#18532 closed defect (bug) (fixed)

resized image dimensions incorrectly floored instead of rounded

Reported by: _ck_'s profile _ck_ Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.1 Priority: normal
Severity: normal Version: 2.5
Component: Media Keywords: has-patch
Focuses: Cc:

Description (last modified by scribu)

Image dimensions are incorrectly floored instead of rounded to a proper number. Some attempt at "fixup" is done later in some processes to move them by a pixel, but still the way it's initially calculated is wrong.

For example:

A typical photo has a 3:2 (3/2 = 1.5) ratio.

If it's resized to 600px width, WP will make the height 399px - this is wrong.

It happens because of function wp_constrain_dimensions in media.php (~line 304) which floors the float using intval, which is wrong.

	$w = intval( $current_width  * $ratio );
	$h = intval( $current_height * $ratio );

I propose

	$w = round( $current_width  * $ratio );
	$h = round( $current_height * $ratio );

Earlier in the function, both $ratio and $current_width are already handled as numbers, so they have to be valid numbers, no need to use intval for that reason, it's just being used to floor, which is bad in this case.

Using round is not a performance issue because this function is not used in realtime output for templates but typically in admin area.

Optionally while you are at it, put a filter on wp_constrain_dimensions and pass the filter all the values passed to the function before the array is returned at the end.

Attachments (3)

18532.patch (1.0 KB) - added by SergeyBiryukov 13 years ago.
18532.2.patch (1.5 KB) - added by SergeyBiryukov 13 years ago.
18532k.diff (15.8 KB) - added by kitchin 10 years ago.
Note, has xxx comments and disables some Imagick tests.

Download all attachments as: .zip

Change History (17)

#1 @scribu
13 years ago

  • Description modified (diff)
  • Version set to 2.5

#2 @SergeyBiryukov
13 years ago

  • Keywords has-patch added

#3 @_ck_
13 years ago

Oops, I made an oversight in the first post (ticket).

The statement that compares the upper and lower bounds must also use round and not intval to test the result. This is the complete change:

	if ( round( $current_width * $larger_ratio ) > $max_width || round( $current_height * $larger_ratio ) > $max_height )
 		// The larger ratio is too big. It would result in an overflow.
		$ratio = $smaller_ratio;
		// The larger ratio fits, and is likely to be a more "snug" fit.
		$ratio = $larger_ratio;

	$w = round( $current_width  * $ratio );
	$h = round( $current_height * $ratio );

This is tested working on a very basic level, for example 3/2 images become 600:400 now instead of 600:399

However it has not been tested robustly.

#4 @SergeyBiryukov
13 years ago

Updated the patch.

#5 @wonderboymusic
10 years ago

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

#6 @kitchin
10 years ago

Note round() makes a float not an int.

#7 @wonderboymusic
10 years ago

The default precision for round() is zero, which means it will return an int:

#8 @wonderboymusic
10 years ago

  • Milestone changed from Future Release to 4.1
  • Owner set to SergeyBiryukov
  • Status changed from new to assigned

Patch is blowing up.

10 years ago

Note, has xxx comments and disables some Imagick tests.

#9 @kitchin
10 years ago

Patch 18532k.diff does this:

  1. Adjusts resize tests to match new behavior where it is off by one pixel from old behavior. I forgot to make a test to fail the bug report, so we need to do that still. The new numbers agree with IrfanView, Photoshop, any web browser, etc., of course. Because they round. This shows we need the patch.
  1. Changes a bunch of assertEquals() to assertSame() because the function should continue to return integers, not floats that look like integers.
  1. I put the same rounding fix in image_resize_dimensions() for cropping.
  1. I have some xxx comments that need to be resolved or deleted.
  1. Getting scary looking console errors on my CentOS 5 system cPanel with ImageMagick 6.2.8 (yum up to date), so I skipped-out the unit tests that try to load a PNG or GIF when Wordpress decides ImageMagick only supports JPG (iterator support). They try to load a WP_Error object back into ImageMagick. Instead I did markTestSkipped() when

5a. WP_Error reached in PNG or GIF test.

5b. Any time test_resize_bad_image() is called with ImageMagick.

We will probably want a patch without the (5b) change, or condition (5b) on PNG support. There is no need for the (5a) console errors at all, they need to be either assertions or skips.

I kept the assert on ImageMagick PNG support in general, so you still get one Fail. (That makes it skip the assert for GIF support.)

  1. Existing code could use some style cleanup.

Proposed changes for next patch:

  • Make a test this bug fails.
  • Condition (5b) Skip on no ImageMagick PNG support.
  • Fix xxx comments.
Last edited 10 years ago by kitchin (previous) (diff)

#10 @kitchin
10 years ago

Wordpress should protect ImageMagick and GD from non-string input. New ticket for that?

Last edited 10 years ago by kitchin (previous) (diff)

#11 @kitchin
10 years ago

  • Keywords 2nd-opinion added

I can do a clean patch with no xxx comments and minimal changes to the unit tests if that is better for review.

#12 @wonderboymusic
9 years ago

  • Keywords needs-refresh 2nd-opinion removed
  • Owner changed from SergeyBiryukov to wonderboymusic

#13 @wonderboymusic
9 years ago

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

In 30660:

Use round() instead of floor() when resizing image dimensions.

Updates unit tests.

Props SergeyBiryukov, kitchin.
Fixes #18532.

#14 @maniu
7 years ago

I just wanted to note that this fix breaks linking to some old images (old enough to use previous method of calculating size) when importing. It replaces the link but dimensions are different by one pixel and post content ends up with broken image.

Note: See TracTickets for help on using tickets.