WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 7 months ago

#18532 closed defect (bug) (fixed)

resized image dimensions incorrectly floored instead of rounded

Reported by: _ck_ Owned by: 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 4 years ago.
18532.2.patch (1.5 KB) - added by SergeyBiryukov 4 years ago.
18532k.diff (15.8 KB) - added by kitchin 9 months ago.
Note, has xxx comments and disables some Imagick tests.

Download all attachments as: .zip

Change History (16)

comment:1 @scribu4 years ago

  • Description modified (diff)
  • Version set to 2.5

@SergeyBiryukov4 years ago

comment:2 @SergeyBiryukov4 years ago

  • Keywords has-patch added

comment:3 @_ck_4 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;
	else
		// 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.

@SergeyBiryukov4 years ago

comment:4 @SergeyBiryukov4 years ago

Updated the patch.

comment:5 @wonderboymusic12 months ago

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

comment:6 @kitchin12 months ago

Note round() makes a float not an int.

comment:7 @wonderboymusic9 months ago

The default precision for round() is zero, which means it will return an int:
http://php.net/manual/en/function.round.php#refsect1-function.round-examples

comment:8 @wonderboymusic9 months ago

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

Patch is blowing up.

@kitchin9 months ago

Note, has xxx comments and disables some Imagick tests.

comment:9 @kitchin9 months 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 9 months ago by kitchin (previous) (diff)

comment:10 @kitchin9 months ago

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

Last edited 9 months ago by kitchin (previous) (diff)

comment:11 @kitchin9 months 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.

comment:12 @wonderboymusic7 months ago

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

comment:13 @wonderboymusic7 months 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.

Note: See TracTickets for help on using tickets.