Make WordPress Core

Opened 12 years ago

Closed 11 years ago

#22663 closed defect (bug) (fixed)

Performing a non-square rotate() with Imagick, then cropping, can result in incorrect Image

Reported by: kirasong's profile kirasong Owned by: nacin's profile nacin
Milestone: 3.7 Priority: normal
Severity: minor Version: 3.5
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

Note: This is only for *some* recent versions of ImageMagick. Older ones seem to be a bit more sane, and work fine without any patch whatsoever.

When ImageMagick performs certain operations, it sets its virtual page's x/y origin values to non-zero numbers and leaves them there, which can break some further operations that depend on the virtual page.

In this case, when rotating in non 90 degree increments, Imagick doesn't set the x/y, which means that core doesn't run into this bug, since the built-in image editor only allows for 90 degree rotates in either direction.

However, if you're a user of the API, and perform a "non-square" rotate, then a crop, you could end up with an image result that differs from what you'll get from GD.

There are two ways of fixing this:

  • Update the virtual page with each update_size()
  • Update the virtual page immediately after a rotate() only

The first is what I think we should land on eventually, because it will prevent plugins' methods from breaking core's accidentally (if they do not reset the page's values). However, that would require an additional error message to be added, which means a new string.

The second would fix the bug for anything core's APIs would do, but would depend on plugins handling resetting the page values appropriately in their own methods. It is, however, a bit safer (and wouldn't require a string change), if this is something that might land for 3.5.

I've attached patches for each method.

Attachments (5)

22663.update_size.diff (614 bytes) - added by kirasong 12 years ago.
22663.in_rotate.diff (815 bytes) - added by kirasong 12 years ago.
GD.jpg (16.3 KB) - added by kirasong 12 years ago.
Pre-Patch Example of 30 degree rotate, then crop.
Imagick.jpg (89.6 KB) - added by kirasong 12 years ago.
Pre-Patch Example (ImageMagick 6.7.7-6) of 30 degree rotate, then crop.
Imagick.2.jpg (24.2 KB) - added by kirasong 12 years ago.
Post-Patch Example (ImageMagick 6.7.7-6) of 30 degree rotate, then crop.

Download all attachments as: .zip

Change History (17)

@kirasong
12 years ago

Pre-Patch Example of 30 degree rotate, then crop.

@kirasong
12 years ago

Pre-Patch Example (ImageMagick 6.7.7-6) of 30 degree rotate, then crop.

@kirasong
12 years ago

Post-Patch Example (ImageMagick 6.7.7-6) of 30 degree rotate, then crop.

#1 @kirasong
12 years ago

Attached example rotate, then crop(s).
GD is the same as attached before and after patch.

#2 @nacin
12 years ago

I'm thinking this should wait until 3.6.

#3 @nacin
12 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Severity changed from normal to minor

Basically, this only comes into play when a plugin is using the rotation API outside of standard 90-degree increments. Good find, but punting.

#4 @markoheijnen
12 years ago

  • Milestone changed from Future Release to 3.6

#5 @markoheijnen
11 years ago

  • Keywords commit added

Patch 22663.in_rotate.diff​ is the fix for this ticket.

#6 @kirasong
11 years ago

Would still like to see this go in for 3.6.

Agreed with @markoheijnen on this, 22663.in_rotate.diff​ is the least heavy-handed solution.

#7 @markjaquith
11 years ago

  • Milestone changed from 3.6 to Future Release

#8 @kirasong
11 years ago

  • Milestone changed from Future Release to 3.7

#9 @ryan
11 years ago

22663.in_rotate.diff is still good to go?

#10 @kirasong
11 years ago

Yep, double-checked with tests. Good to go.

#11 @kirasong
11 years ago

Can we land 22663.in_rotate.diff for 3.7? It's pretty straight-forward, and would be great for it not to ride another release cycle.

#12 @nacin
11 years ago

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

In 25636:

Fix non-square rotations when using the Imagick image editor.

props DH-Shredder.
fixes #22663.

Note: See TracTickets for help on using tickets.