Make WordPress Core

Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#15989 closed defect (bug) (fixed)

image_resize_dimensions() mistakenly defaults crop center as image's center of gravity.

Reported by: jacof's profile jacof Owned by: nacin's profile nacin
Milestone: 3.4 Priority: normal
Severity: major Version: 3.1
Component: Post Thumbnails Keywords: has-patch commit
Focuses: Cc:

Description

WordPress add_image_size() routine calls ultimately for image_resize_dimensions(), which calculates the cropped image dimensions. (http://core.trac.wordpress.org/browser/tags/3.0.3/wp-includes/media.php#L333)

The function assumes that the cropping center is always the gravity center of the image, whereas the user must be able to select where and how the cropping will be made.

$s_x = floor( ($orig_w - $crop_w) / 2 );
$s_y = floor( ($orig_h - $crop_h) / 2 );

Due to this, custom thumbnails result cropped around wrong centers.

WordPress' Editor inside the Multimedia dialog doesn't affect post thumbnails defined by the user's theme, but only the thumbnails defined in the admin, as it doesn't regenerate them after rescaling the image. A temporary fix is regenerating all of the thumbnails using a third party plugin ("Regenerate Post Thumbnails"). This temporary fix generates (#Thumbs)*(#Images)*(#Resizes)+(#Images) in the Server, which is over demanding.

Fixing the Wordpress Editor to affect post-thumbnails (as it should) would not be a proper fix, since the user must not be constrained to reducing the dimensions of his/her image as a whole in order to produce a correct thumbnail.

The two solutions feasable for this are:

1- Correct the Wordpress Multimedia Editor to regenerate thumbnails after rescaling the image (short-term).

2- Modify image_resize_dimensions() to accept optional $x=0, $y=0 and allowing, ultimately, the selection of those values in the Multimedia Editor by the user, for each image, and for each thumbnail.

Attachments (11)

1.JPG (65.0 KB) - added by jacof 14 years ago.
2.JPG (8.0 KB) - added by jacof 14 years ago.
3.JPG (58.4 KB) - added by jacof 14 years ago.
4.JPG (7.4 KB) - added by jacof 14 years ago.
5.JPG (54.4 KB) - added by jacof 14 years ago.
6.JPG (9.1 KB) - added by jacof 14 years ago.
7.JPG (77.5 KB) - added by jacof 14 years ago.
test_crop_filter.php (2.9 KB) - added by buzz_matt 14 years ago.
Exercises media.php patch
media.diff (731 bytes) - added by buzz_matt 14 years ago.
Add filter to allow crop location alterations
15989.patch (747 bytes) - added by SergeyBiryukov 13 years ago.
media.php.patch (845 bytes) - added by ebababi 13 years ago.
Add filter to hook image_resize_dimensions functionality

Download all attachments as: .zip

Change History (35)

@jacof
14 years ago

@jacof
14 years ago

@jacof
14 years ago

@jacof
14 years ago

@jacof
14 years ago

@jacof
14 years ago

@jacof
14 years ago

#1 @nacin
14 years ago

I was looking for a filter to modify this a few months ago. None exists. It'd be nice if one did.

#2 @jacof
14 years ago

It would.
Wordpress should at least allow people to hook into this function. The current coding does not allow any correction from the users.

A 3rd way to solve this has come to my mind:

3- Correct Multimedia Editor so a user can choose where to place his thumbnails, internally resize the image accordingly, and regenerating the thumbnail. This method involves no need for changing the image_resize_dimensions() function, and it would be possible to do with a plugin.

#3 @jacof
14 years ago

  • Keywords dev-feedback 2nd-opinion added

#4 @markjaquith
14 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to markjaquith
  • Status changed from new to assigned

This would be fixed if we had just-in-time resizing, which would limit you only the image sizes you use.

@buzz_matt
14 years ago

Exercises media.php patch

@buzz_matt
14 years ago

Add filter to allow crop location alterations

#5 @SergeyBiryukov
14 years ago

  • Keywords has-patch added; Thumbnails Resizing Rescaling Cropping removed

#6 @scottconnerly
13 years ago

  • Cc scott@… added

#7 @ebababi
13 years ago

Hi there, this is my first comment and patch!

If it is for image_resize_dimensions to use a filter, why not having it affect the whole output, like on image_downsize?

Implementation on the patch above.

#8 @mgolawala
13 years ago

We are using the thumbnail API to generate our thumbnails from portrait images. When we noticed the cropping around the center was incorrectly causing the faces of the subject of the photograph to be lost, we investigated the API to see if there was any way to change the center from which the crop was taken. On searching trac, we came across this ticket and it would make a lot of sense for us to use a filter such as the one in the patch, instead of implementing/adding a thumbnail plugin just so we can crop from the top instead of the center.

Is there a plan to include this in core soon? Are there any steps our team can take to help move this along and have it included in core?

Thanks!

#9 @mintindeed
13 years ago

  • Cc gabriel.koen@… added

#10 @nacin
13 years ago

  • Milestone changed from Future Release to 3.4
  • Resolution set to fixed
  • Status changed from assigned to closed

I like the approach of the patch.

#11 @ocean90
13 years ago

  • Keywords dev-feedback 2nd-opinion removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

I like the approach of the patch.

But this doesn't resolve the ticket. :)

#12 @nacin
13 years ago

  • Owner changed from markjaquith to nacin
  • Status changed from reopened to accepted

Fat-fingered it.

#13 @cramdesign
13 years ago

  • Cc matt@… added

#14 @husobj
13 years ago

  • Cc ben@… added

#15 follow-ups: @nacin
13 years ago

  • Keywords commit added

I would only suggest that we move the filter above the two return false's.

#16 in reply to: ↑ 15 @SergeyBiryukov
13 years ago

Replying to nacin:

I would only suggest that we move the filter above the two return false's.

Done in 15989.patch.

#17 in reply to: ↑ 15 @ebababi
13 years ago

Replying to nacin and SergeyBiryukov:

I would only suggest that we move the filter above the two return false's.

I would vote against this suggestion for two reasons:

  1. The two return false's purpose is to validate the input of the function. I believe that input should be validated before reaching the filtering chain. Is there any case that negative numbers are valid input for original or destination image sizes?
  2. As input validation before filter chaining is already implemented on other cases, like image_downsize, shouldn't we do the same to provide a consistent API behaviour?

Based on the above, I would insist on the media.php.patch patch.

#18 follow-up: @nacin
13 years ago

You are correct.

The only thing I would change is the default value for the filter should be null. That would allow false to be returned (which is a valid return value for image_resize_dimensions).

@ebababi
13 years ago

Add filter to hook image_resize_dimensions functionality

#19 in reply to: ↑ 18 @ebababi
13 years ago

Replying to nacin:

The only thing I would change is the default value for the filter should be null. That would allow false to be returned (which is a valid return value for image_resize_dimensions).

The media.php.patch modified as requested.

#20 follow-up: @nacin
13 years ago

In the future, please do not replace existing patches, only upload new ones. It makes it difficult to follow the ticket and figure out why things progressed the way they did. Thanks!

#21 @nacin
13 years ago

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

In [20694]:

Add filter to image_resize_dimensions() so plugins can perform this calculation on their own. props ebababi. fixes #15989.

#22 in reply to: ↑ 20 ; follow-up: @ebababi
13 years ago

Replying to nacin:

In the future, please do not replace existing patches, only upload new ones.

Sorry, still a noob here...

Since this patch is now official I wrote the relevant documentation, including an example moving the cropping center to the top of the image.

#23 in reply to: ↑ 22 ; follow-up: @nacin
13 years ago

Replying to ebababi:

Since this patch is now official I wrote the relevant documentation, including an example moving the cropping center to the top of the image.

Great. It might be good to have the page reflect that it requires WordPress 3.4, though. Not sure the best way to do that on the Codex.

#24 in reply to: ↑ 23 @ebababi
13 years ago

Replying to nacin:

Great. It might be good to have the page reflect that it requires WordPress 3.4, though. Not sure the best way to do that on the Codex.

I updated the documentation to reflect the version requirement in the first sentence (it should be obvious).

Note: See TracTickets for help on using tickets.