Posted by kaare on February 11, 2009 at 3:03am
| Project: | ImageAPI |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | reviewed & tested by the community |
Issue Summary
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.
| Attachment | Size |
|---|---|
| gd_rotate_new_size.patch | 382 bytes |
| im_rotate_new_size.patch | 1.7 KB |
Comments
#1
Fixed typo in title.
#2
Yeah, I identified the same a while ago.
I didn't come up with the imagemagick fix though.
Hope it works!
#3
I 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.
#4
Here'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.
<?php
function imageapi_imagemagick_image_rotate(&$image, $degrees, $bgcolor) {
if (is_int($bgcolor)) {
$bgcolor = '#'. str_pad(dechex($bgcolor), 6, 0);
}
else {
$bgcolor = str_replace('0x', '#', $bgcolor);
}
$image->ops[] = '-background '. escapeshellarg($bgcolor) .' -rotate '. (float) $degrees;
// Need to update the dimensions for later operations in the pipeline
// otherwise (eg) rotate then scale is badly wrong.
$dimensions = imageapi_imagemagick_calculate_rotated_dimensions($image->info['width'], $image->info['height'], $degrees);
$image->info['width'] = $dimensions['width'];
$image->info['height'] = $dimensions['height'];
return TRUE;
}
/**
* Calculate the new dimensions of a rotated image.
*
* Given the dimensions of a box, and an angle, return the dimensions of a new
* containing box.
*
* @return a named array containing values for width and height.
*/
function imageapi_imagemagick_calculate_rotated_dimensions($width, $height, $degrees) {
// There may be several maths short cuts, eg matrices,
// but doing it the long way like this is clear code and geometry.
// Compressing things just made it 'clever' in a bad way.
//
// @see stackoverflow.com/questions/622140/calculate-bounding-box-coordinates-from-a-rotated-rectangle-picture-inside
//
$theta = deg2rad($degrees);
// Set up a box.
$points = array(
array(0, 0),
array($width, 0),
array(0, $height),
array($width, $height),
);
$bounding_box = array(
'left' => 0,
'right' => 0,
'top' => 0,
'bottom' => 0,
);
// Rotate each point in the box
foreach ($points as $p => $point) {
$x = $point[0];
$y = $point[1];
$new_x = ($x)*cos($theta)+($y)*sin($theta);
$new_y = ($x)*sin($theta)+($y)*cos($theta);
// Note the bounds
$bounding_box['left'] = min($bounding_box['left'], $new_x);
$bounding_box['right'] = max($bounding_box['right'], $new_x);
$bounding_box['top'] = min($bounding_box['top'], $new_y);
$bounding_box['bottom'] = max($bounding_box['bottom'], $new_y);
}
// And how big is the new box?
$dimensions = array(
'width' => (int)abs($bounding_box['right'] - $bounding_box['left']),
'height' => (int)abs($bounding_box['bottom'] - $bounding_box['top']),
);
return $dimensions;
}
?>
#5
better title. i'll try to review this this afternoon.
#6
This 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.pngwith imagecache and different angles, I have collected data about the calculated and rendered dimensions with different rotation. This is what I got:Degrees | Calculated dimensions | Rendered dimensions
| dman | kaare |
--------------------------------------------------------------
0 | 1180 x 1350 | 1180 x 1350 | 1180 x 1350
10 | 1396 x 1534 | 1396 x 1534 | 1396 x 1534
20 | 1570 x 1672 | 1571 x 1672 * | 1570 x 1672
30 | 1696 x 1759 * | 1697 x 1759 * | 1696 x 1760
40 | 1771 x 1792 * | 1772 x 1793 * | 1772 x 1792
45 | 1788 x 1788 | 1789 x 1789 * | 1788 x 1788
50 | 1792 x 1771 * | 1793 x 1772 * | 1792 x 1772
60 | 1759 x 1696 * | 1759 x 1697 * | 1760 x 1696
70 | 1672 x 1570 | 1672 x 1571 * | 1672 x 1570
80 | 1534 x 1396 | 1534 x 1396 | 1534 x 1396
90 | 1350 x 1180 | 1350 x 1180 | 1350 x 1180
*) Has errors
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.
#7
Now 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.
#8
Until 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.
#9
dman,
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.
#10
Forgot to flush the imagecache actions during install and uninstall in #8. New version.
#11
What fun!
Ah well, worth a crack.
#12
Anyone else up for reviewing dmans patch? If not, i'm marking this tested and ready to be commited.
#13
This works for dman and me. See test results in #6 between implementations.
#14
Could you post a proper patch?
#15
Here goes.
This is dman's version of the rotate canvas size algorithm :-)