Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#23102 closed defect (bug) (fixed)

Thumbnails and medium-sized images always show unconstrained dimensions in the Media panel size drop-down

Reported by: jond3r's profile jond3r Owned by: ryan's profile ryan
Milestone: 3.5.1 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

... though they can actually be constrained! This will not happen frequently, and might be resolved as a won't fix.

However, as I was partly responsible for the code fix in #22738, which led to this defect, and it is a regression compared to 3.4, I thought I should report it. So here goes ... (For more context, you may have a look at the longer article I wrote about this on jond3r.org.)

In [23096], the fix to #22738, thumbnails and medium-sized images were bypassed image_constrain_size_for_editor(), since it was thought that they were never constrained. However this can happen if the image size settings in the Media Settings administration panel have been decreased between upload and use of the images. If this is the case, image_constrain_size_for_editor() will constrain all the standard image sizes to those values that currently are in effect (through the Media Settings panel), which thus also will affect thumbnails and medium-sized images.

To reproduce:
Upload an image large enough for medium size to be created using the default medium size settings in the Media Settings panel (300px). Decrease the setting to 200px. Mark the previously uploaded image for insertion in the Insert Media panel. Choose medium size and look at the displayed dimension (300px). Insert the image and observe how the dimension has been decreased to 200px (through the width and/or height attributes). Thus the inserted image is constrained, even though it is not shown in the size drop-down.

The fix is simply to pass all image sizes (except Full) through image_constrain_size_for_editor() as the provided patch shows.

Performance worries have been put forward (#22598) calling image_downsize() repeatedly for many images. #22738 showed that it was enough to call image_constrain_size_for_editor(), which apparently is faster. The proposed fix implies that all processed thumbnails and medium-sized images will have to go through image_constrain_size_for_editor(), which leads to some increased execution time.

To quantify this I did some timing tests. The code used for the tests are shown in the file timing_code.diff, and the test results are shown in timing_dump.txt. The absolute execution times obtained are not that interesting, as they depend on the execution environment, which in this case is a few years old Windows desktop. In a production environment, considerably faster execution time is probable.

The results show an average execution time of 0.439 ms per image calling image_constrain_size_for_editor(), which multiplied by 80 for a full page of images, yields ca 35 ms in increased execution time, compared to the code in 3.5.0. This would hardly be noticeable, and is probably acceptable.

Out of curiosity I also tested to call image_downsize(), which was used in 3.4. A call to image_downsize() took on average 9.78 ms per image, which yields 782 ms for a full page, which is not that easy to ignore. Note that image_downsize() fetches the attachment meta data, which can be costly if the data is not cached, but as far as I can see that is the case, since the meta data is fetched earlier in the function wp_prepare_attachment_for_js(). Nevertheless a call to image_downsize() is 22 times more costly than a call to image_constrain_size_for_editor(), which gives some credit to the worry put forward in #22598, and to the solution proposed in #22738.

Criticism is welcome, especially of the performance tests.

Attachments (3)

23102.diff (902 bytes) - added by jond3r 11 years ago.
Patch
timing_code.diff (2.1 KB) - added by jond3r 11 years ago.
Code for performance test
timing_dump.txt (448 bytes) - added by jond3r 11 years ago.
Test results

Download all attachments as: .zip

Change History (8)

@jond3r
11 years ago

Patch

@jond3r
11 years ago

Code for performance test

@jond3r
11 years ago

Test results

#1 @nacin
11 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.5.1

Good catch. I knew that image_constrain_size_for_editor() only did the complex $content_width stuff for 'large' and additional sizes, but missed the checking of the thumbnail/medium_size_h/w options.

#2 @nacin
11 years ago

In 23264:

Media: Pass thumbnail and medium sizes to image_constrain_size_for_editor() to force constraints based on the current DB options for those sizes. History: see #22598, #22738.

props jond3r.
see #23102.
for trunk.

#3 @nacin
11 years ago

  • Keywords fixed-major added

Needs final flexing, then is good to go for 3.5.

#4 @jond3r
11 years ago

Thanks for accepting. (Gives me some fuel for more hunting, for better or worse ;) )

#5 @ryan
11 years ago

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

In 23282:

Media: Pass thumbnail and medium sizes to image_constrain_size_for_editor() to force constraints based on the current DB options for those sizes. History: see #22598, #22738.

props jond3r
fixes #23102
for 3.5

Note: See TracTickets for help on using tickets.