Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#13556 closed defect (bug) (fixed)

wp_get_attachment_image() displays larger images than necessary with an array

Reported by: tomthewebmaster Owned by:
Milestone: 3.0 Priority: normal
Severity: blocker Version: 3.0
Component: Media Keywords:
Focuses: Cc:


WordPress is not giving me the most optimized image when I use an array with the wp_get_attachment_image() function in the WordPress 3.0 builds.

I used the following code on a page: <?php echo wp_get_attachment_image(60, array(200,200), false); ?>. While the correct sized image is displayed on the page, WordPress is using the full size image to scale from, instead of scaling from the medium size image. I am given the most optimized image (medium) as expected in WordPress 2.9.2.

See these examples:

WordPress 3.0 nightly build, where the full size image is used: http://jingletom.com/beta/image-test-2/ WordPress 2.9.2, where the medium image is used: http://tomlany.net/use-the-attachment-image/image-test/

In both cases, you want to look at the top image on the page. Right click on the images to see the different sized images that are being used. In both cases, the medium image is set to be the default 300x300 in the administration panel.

Let me know if anyone has any questions. Thanks!

Attachments (4)

13556.diff (980 bytes) - added by markjaquith 8 years ago.
media.php.patch (1.0 KB) - added by jorbin 8 years ago.
13556.off-by-one-pixel.diff (3.1 KB) - added by markjaquith 8 years ago.
13556.off-by-one-pixel.002.diff (3.1 KB) - added by markjaquith 8 years ago.

Download all attachments as: .zip

Change History (27)

#2 @tomthewebmaster
8 years ago

Isn't WordPress supposed to scale images down from the next sized cached thumbnail that exists already, though? For example, for a 200x200, shouldn't it echo the 300x300 medium image, not the full sized image? This is what is does in 2.9.2.

#3 @filosofo
8 years ago

Only if the thumbnail already exists.

#4 @tomthewebmaster
8 years ago

In the case that I referenced above, a 199x300 image exists (http://jingletom.com/beta/wp-content/uploads/2010/05/IMGP00061-199x300.jpg), but the full size image is still being used.

#5 @jorbin
8 years ago

  • Cc aaron@… added
  • Keywords close added

The issue is actually with image_get_intermediate_size, which looks for the correct image. It won't use a non thumbnail cropped image that has a different aspect ratio (which I think is a good thing). Therefore calling a

It also won't up-size an image, so trying to get a 200x200 image will never get the 199x300 size since the width is smaller then the one you are trying to get.

I think we should close this as wontfix.

#6 @jorbin
8 years ago

The change to the relevant functions was made in http://core.trac.wordpress.org/changeset/13145

#7 @tomthewebmaster
8 years ago

Thanks for the note, jorbin. When both ratios are smaller (170x170, for instance) the full size image will still be used.

8 years ago

#8 @markjaquith
8 years ago

  • Keywords close removed

Proposed patch attached. Seems that [13145] used 0 and 1 of the result when it should have used 4 and 5.

#9 @markjaquith
8 years ago

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

(In [14979]) Use the correct array members of image_resize_dimensions() result when calculating best match for dynamic HTML image resize. fixes #13556

8 years ago

#10 @tomthewebmaster
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Unfortunately, after I made the changes in the diff to the file on my install the problem still seems to be present. See: http://jingletom.com/beta/image-test-2/


#11 @nacin
8 years ago

(In [14981]) Add some educational tips. Suggest using add_image_size() in the inline docs for image_get_intermediate_size() and wp_get_attachment_image(). props jorbin. see #13556.

#12 @markjaquith
8 years ago

tomthewebmaster — what are the sizes of the original image, the sizes of all your defined resizes, and the size requested?

#13 @tomthewebmaster
8 years ago

Original image - 465x700 (http://jingletom.com/beta/wp-content/uploads/2010/05/IMGP00062.jpg) Requested - 170x170

Size of crunched images:

Thumbnail - 150x150 (http://jingletom.com/beta/wp-content/uploads/2010/05/IMGP00062-150x150.jpg) Medium - 199x300 (http://jingletom.com/beta/wp-content/uploads/2010/05/IMGP00062-199x300.jpg) Large - None (due to size of full image). Full size - original image

Code I used to request the image: <?php echo wp_get_attachment_image(70, array(170,170), false); ?>

#14 @markjaquith
8 years ago

Thanks. It's a rounding issue. Instead of 199x300, we're getting a result of 199x299, which doesn't match. I'm looking into it.

#15 @tomthewebmaster
8 years ago

The same image works properly in 2.9.2 (see: http://tomlany.net/use-the-attachment-image/image-test/), so I would imagine this likely involves a newer change.

#16 @markjaquith
8 years ago

  • Severity changed from normal to blocker

13556.off-by-one-pixel.diff does the following:

Instead of always using the smaller of the height/height, width/width ratios, it tries to see if the larger one will fit. If it does, great — it is likely the more "snug" fit. If it doesn't, it falls back to the smaller ratio.

In either case, it then checks the dimensions that were resized, and for each, checks to see if the result is within one pixel of the max. If it is, it bumps up the resulting resize by one pixel (i.e. to the max).

For example, before this patch, 465x700 in a 177x177 box was 117x176... a pixel short. After the patch, that 176 would become 177. One of the dimensions should equal one of the maxima. This seems to mostly (if not completely) eliminate the effect whereby an image constrained to a box, and then constrained to the result of that constraint, would become progressively more cropped. You'd get a stepping effect where with each constraint operation, a pixel or two would be shaved off. e.g. 100x199 might become 99x199, which might become 99x198, etc. That causes a mismatch in the "next biggest image of the same ratio" check, which causes the full res image to be used instead (the whole point of this bug).

To compensate for this possible one-pixel inflation, the "next biggest size that matches the ratio" check now has a -1 pixel area of forgiveness. This is so that a crops generated in earlier versions will match constraints done in 3.0. So a 200x199 image in 2.9 will be used if 3.0 is looking for a 200x200 version of that image.

Setting as a blocker, because I don't want an RC/final going out until this is resolved. Please test the patch!

#17 @tomthewebmaster
8 years ago

The patch seems to be working for me. I get some PHP errors with WP_DEBUG turned on, though. See: http://jingletom.com/beta/image-test-2/. I will be back in about 4 hours or so and can test it more then.


#18 @jorbin
8 years ago

I've tested this patch with the following orignal image sizes and cropped image calls. Everything seems to be working fine. No errors.

Originals: 940 x 726 2048 x 1536 940 x 198

New Size Calls: 100 x 200 100 x 100 100 x 111 111 x 133 150 x 113 151 x 123 230 x 80 300 x 200 300 x 100 300 x 90 300 x 250 300 x 200 700 x 400 1000 x 1000 1200 x 1000

#20 @nacin
8 years ago

Initializing $did_width = $did_height = false; might be more in line with our style, but it fixes the notices for me.

#21 @tomthewebmaster
8 years ago

Yes, the new patch fixes the errors for me. This looks great. Thanks, all!

#22 @automattor
8 years ago

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

(In [15002]) Tweaks to wp_constrain_dimensions() for better fits and consistency. fixes #13556

#23 @markjaquith
8 years ago

(In [15003]) Initialize instead of using isset(). props nacin. see #13556

Note: See TracTickets for help on using tickets.