imageapi_image_rotate()
will not report back the resulting width and height after rotation. In the gd toolkit it's an easy fix (patch applied), but due to the parameter pipeline nature of imagemagick toolkit, more care needs to be taken.
For imageapi_imagemagick_image_rotate()
I've tried to apply the calculation used in imagerotate.inc, but it doesn't calculate the exact same size as the ones imagemagick produces. Lot of off-by-one errors. Still, I think this is a better approach than not reporting anything. This patch is the first attempt at creating a canvas calculation function for rotation based on this code.
Comment | File | Size | Author |
---|---|---|---|
#15 | imageapi-372497-15.patch | 2.77 KB | kaare |
#10 | icrotfix-372497-10.tgz | 1.95 KB | kaare |
#8 | icrotfix.tar_.gz | 1.88 KB | kaare |
im_rotate_new_size.patch | 1.7 KB | kaare | |
gd_rotate_new_size.patch | 382 bytes | kaare |
Comments
Comment #1
kaareFixed typo in title.
Comment #2
dman CreditAttribution: dman commentedYeah, I identified the same a while ago.
I didn't come up with the imagemagick fix though.
Hope it works!
Comment #3
drewish CreditAttribution: drewish commentedI committed the GD portion to HEAD and DRUPAL-6--1 since that was straight forward. The ImageMagick bit I want to look at a bit more.
Comment #4
dman CreditAttribution: dman commentedHere's my overcommented version of the same thing.
Looks like kaare's one would have worked just fine, but for no good reason I ended up doing it myself also. I missed/forgot that there was this patch pending :-/
I'm building a test suite for all our actions, finally having a go at imagemagick, and this issue got in my face again. It is a bug, and I've failed to find a better answer than just doing the maths ourselves.
Turns out the logic I came up with independently is basically the same as the previous patch :-} I guess that's a good sign.
Comment #5
drewish CreditAttribution: drewish commentedbetter title. i'll try to review this this afternoon.
Comment #6
kaareThis bug is currently a showstopper for a module i'm working on, so I decided to debug/review some more. Now I have tested both dman and my own canvas calculation algorithms. Both approaches has off-by-one errors here and there, and I think we always will have this until we can get the rendered size from ImageMagick directly, or copy the algorithm for calculating the canvas size directly from Imagmagick's source code. Using
imagecache_sample.png
with imagecache and different angles, I have collected data about the calculated and rendered dimensions with different rotation. This is what I got:I only tested the first quarter (0 - 90°), as this pattern repeats for the rest of the segments.
As you can see, dman's results are lower than the rendered version, though are more often correct, whereas my results are higher, and more often wrong. But it works. Most importantly, both versions are always 100% correct for 0, 90, 180 and 270 degrees, which are used in 99% of image rotations. dman's algorithm seems to be correct also for 45° (and thereby also 135, 225 and 315 degrees), increasing the accuracy to 99.9% of all use cases ;-)
I think dman's code is good enough to commit, and unless the imageapi_imagemagick module fundamentally alters the operation pipeline, we need a way to calculate the canvas size like this issue tries to solve. I also think we should add simple tests for 0, 90, 180 and 270 degrees, as these don't need special calculation.
This bug is most easily visible in imagecache, when rotation comes before scale on portrait or landscape pictures. The aspect ratio will stay the same as the original image, so rotating a landscape picture 90° will still be rendered in landscape, though the content will be very wide.
Again, dman's patch will solve 99.9% of the rotation cases and the 0.1% cases left will have only off-by-one errors.
Comment #7
dman CreditAttribution: dman commentedNow that's thorough test results! :-B
Good job!
Just a thought, do you think using round() instead of (int) in the last step would stamp out the last off-by-ones? As I seem to occasionally be 1 pixel under, that's probably where it's going.
I could live with off-by-one in images of that scale, but it's nice to get it right.
Comment #8
kaareUntil this issue is fixed, this imagecache action module will look for rotation actions and update the canvas size. Add this action immediately after any rotation action. See README.txt for more info.
Comment #9
kaaredman,
I tried with
round()
in your version, and that resulted in the exact same errors as I had. Still no cigar.I agree with you that this is close enough for everyday usage. I'm excited to hear what drewish think.
Comment #10
kaareForgot to flush the imagecache actions during install and uninstall in #8. New version.
Comment #11
dman CreditAttribution: dman commentedWhat fun!
Ah well, worth a crack.
Comment #12
kaareAnyone else up for reviewing dmans patch? If not, i'm marking this tested and ready to be commited.
Comment #13
kaareThis works for dman and me. See test results in #6 between implementations.
Comment #14
drewish CreditAttribution: drewish commentedCould you post a proper patch?
Comment #15
kaareHere goes.
This is dman's version of the rotate canvas size algorithm :-)