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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kaare’s picture

Title: rotate() don't update width and height in $image » rotate() doesn't update width and height in $image

Fixed typo in title.

dman’s picture

Yeah, I identified the same a while ago.
I didn't come up with the imagemagick fix though.
Hope it works!

drewish’s picture

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.

dman’s picture

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.


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;
}
drewish’s picture

Title: rotate() doesn't update width and height in $image » imageapi_imagemagick_image_rotate() doesn't update width and height in $image

better title. i'll try to review this this afternoon.

kaare’s picture

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.png with 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.

dman’s picture

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.

kaare’s picture

FileSize
1.88 KB

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.

kaare’s picture

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.

kaare’s picture

FileSize
1.95 KB

Forgot to flush the imagecache actions during install and uninstall in #8. New version.

dman’s picture

What fun!
Ah well, worth a crack.

kaare’s picture

Anyone else up for reviewing dmans patch? If not, i'm marking this tested and ready to be commited.

kaare’s picture

Status: Needs work » Reviewed & tested by the community

This works for dman and me. See test results in #6 between implementations.

drewish’s picture

Could you post a proper patch?

kaare’s picture

Version: 6.x-1.3 » 6.x-1.x-dev
FileSize
2.77 KB

Here goes.

This is dman's version of the rotate canvas size algorithm :-)