Image rotate dimensions callback can calculate new dimensions for every angle

Updating tags per http://drupal.org/node/1517250

Great! Can you rework the patch?

Well, thanks a lot @fietserwin for the extensive research on this issue.

IMHO, I think we should have 'our truth' for what we expect to be the dimensions resulting from a rotation. It's a tradeoff: is it preferable to go after each single toolkit / version to match their algorithms and adapt code and tests every time a change occurs, or rather have 'our truth' and always adapt the image to this truth which also means consistent image dimensions across toolkits/versions?

I vote for the latter: this would mean to:

1. determine 'our truth' dimensions (having decided to use one of the three algorithms)
2. execute the toolkit rotate operation
3. execute on the resulitng image a resize operation to the 'our truth' dimensions

Some code review:

+++ b/core/lib/Drupal/Component/Utility/Image.php
@@ -63,4 +63,286 @@ public static function scaleDimensions(array &$dimensions, $width = NULL, $heigh
+  public static function rotateDimensions54(array $dimensions, $degrees) {

this method is never used

+++ b/core/modules/image/src/Plugin/ImageEffect/RotateImageEffect.php
@@ -7,6 +7,7 @@
+use Drupal\Component\Utility\Image;

I's suggest to alias it to ImageUtility to avoid confusion with the other Image class

+++ b/core/modules/image/src/Tests/ImageDimensionsTest.php
@@ -200,27 +200,6 @@ function testImageDimensions() {
-    // Rotate to a non-multiple of 90 degrees.
-    $effect = array(
-      'id' => 'image_rotate',
-      'data' => array(
-        'degrees' => 57,
-        'random' => FALSE,
-      ),
-      'weight' => 7,
-    );
-
-    $effect_id = $style->addImageEffect($effect);
-    $style->save();
-    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" alt="" />');
-    $this->assertFalse(file_exists($generated_uri), 'Generated file does not exist.');
-    $this->drupalGet($url);
-    $this->assertResponse(200, 'Image was generated at the URL.');
-    $this->assertTrue(file_exists($generated_uri), 'Generated file does exist after we accessed it.');
-
-    $effect_plugin = $style->getEffect($effect_id);
-    $style->deleteImageEffect($effect_plugin);
-

Instead of removing this, if we follow my general suggestion, we might keep this with its own expected width and height attributes on the img tag

+++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Rotate.php
@@ -74,14 +74,40 @@ protected function execute(array $arguments) {
+        \Drupal::logger('image')->notice('The image %file could not be rotated because imagerotate() failed.', array('%file' => $this->getToolkit()->getImage()->getSource()));

$this->logger->notice(....

+++ b/core/tests/Drupal/Tests/Component/Image/ImageUtilityTest.php
@@ -161,4 +161,102 @@ public function providerTestScaleDimensions() {
+  public function providerTestRotateDimensions() {

I am dubious about using GD functions to determine the expected output of Image::rotateDimensions

In fact we would make the expected results dependent on the GD version - or am I missing something?

Leaving in NR status as I believe more eyes are needed here.

Needs a reroll, and the feedback in #15 should be addressed.

Rerolled

Fixed the exceptions in #17.

Patch is looking good, some small code style issues.

1. +++ b/core/lib/Drupal/Component/Utility/Image.php
   @@ -62,5 +62,285 @@ public static function scaleDimensions(array &$dimensions, $width = NULL, $heigh
   +        $newWidth = $width;
   +        $newHeight = $height;
   

   don't use camelCase for local variables.

2. +++ b/core/lib/Drupal/Component/Utility/Image.php
   @@ -62,5 +62,285 @@ public static function scaleDimensions(array &$dimensions, $width = NULL, $heigh
   +        $sinImprecision = $sin < 0 ? -$imprecision : $imprecision;
   +        $cosImprecision = $cos < 0 ? -$imprecision : $imprecision;
   

   don't use camelCase for local variables.

A recap and a new proposal (patch).

Recap:

1. The resulting dimensions of an image rotation differ by different GD or other toolkit implementations. PHP 5.4 and PHP 5.5 show different results, and within PHP 5.5 we can have even have different results based on different interpolation algorithms that can be used. ImageMagick provides also different results from GD and they also can be influenced by setting different interpolation algorithms.
2. We want in this issue the RotateImageEffect::transformDimension method to be able to calculate the expected dimensions resulting from a rotate operation for any angle, and at the image effect level we need such results to be implementation agnostic, i.e. independent from toolkit or from version. We do not take interpolation algorithms in the equation at the moment, so our only input variables are image dimensions and degrees of rotation.
3. For consistency, we need to ensure that an toolkit rotate operation always delivers a rotated image whose dimensions are the same as the ones that RotateImageEffect::transformDimension would return.

Proposal:

• This patch implements a Rectangle component class that calculates in abstraction from any implementation the bounding width and height of a rotated image. For the good and for the bad, it uses the PHP 5.4 GD algorithm – taken from @fietserwin excellent patch in #14. PHP 5.4 because its current results are in fact embedded in many tests.
• Provides a PHPUnit test to ensure that the calculation performed by the Rectangle class yields the same results as PHP 5.4. A data sample of input/output values has been built with direct calls to PHP GD functions. More details in code.
• RotateImageEffect::transformDimensions() is altered so that it now also delivers dimensions for non 90 degree multiples.
• ImageDimensionsTest is altered so that it now checks that rotation for non 90 degree multiples ends up in the height and width attributes of the img tag.
• ImageToolkit\Operation\gd\Rotate is altered so that if the dimensions of the image after rotation do not coincide with the expected ones from the Rectangle class calculation, the image is also resized to expected dimensions.
• The PHP version dependent code in ToolkitGdTest has been removed as now we no longer need to care about it.

No interdiff as in fact it would not make much sense.

Thoughts?

#24: thanks @fietserwin for review.

To be honest I did not give too much thought about 'what' to do if the actual rotated image dimensions differ from the expected ones. I was just thinking that differences would be minimal, and somehow consistent between widht and height, so that a resize would not distort the final result.

But your points are valid, so setting to NW to do that.

Two questions pop up to me now:
a) what if width is smaller and height is bigger than expected (or viceversa)?
b) #24 describes a situation in GD toolkit where we have the possibility to compare rotation results to expected, taking the resource's width and width after the rotation. But what if, like in ImageMagick, we can only determine the expected dimensions and have to force the image to be consistent? Would resize be OK in that case?

More in general - I think we need an update to the issue summary, and probably we need also screenshots to compare PHP 5.4 and PHP 5.5 results based on this patch.

PHP requirements have been raised to 5.5, so it does not make sense to keep an algorithm that calculates rotated dimensions based on 5.4.

This patch now has an algorithm that mimicks the calculation of dimensions done in PHP 5.5+ (after some hair pulling to understand the imprecisions issue as pointed out in #14, thanks again @fietserwin for posting that patch!). Again the logic of that algorithm may be questionable, but I believe we need to be pragmatic and just adopt the one logic that will be most commonly used.

Here I have dropped the changes to the GD rotate operations discussed between #23 and #26 - AFAICS all the different interpolation methods used in GD 5.5 for image rotation rely on the same algorithm to determine the dimensions of the canvas before starting to copy the pixels at destination. We may want to explore in a spin-off issue if the potential use of the imagesetinterpolation() function in contrib could affect dimensions, but that seems quite an edge case.

Note: do not be scared by the size of the patch - most of it is test data for PHPUnit... :)

Forgot to change another instance of rotate test :(

#29 is green

Found some flaws in the algorithm matching PHP GD calculations. Working on a patch.

I could not sort out how to deal with +/- 1 pixel differences in some cases, due to imprecision differences between the use of C floats internally in GD, and of C doubles by PHP.

Here I am taking a different approach - an utility class called by RotateImageEffect actually creates a new GD image via the GD toolkit, rotates it, and returns the dimensions after rotation. It's much simpler, but has a small performance impact and makes the calculation dependent on the installed PHP version.

Cannot do any better ATM, if we find an algorithm that can match in PHP the same results that GD provides we can go back to it. The PHP unit test data sample is taken from PHP 5.5 GD (I added more samples for float and negative angles); that one can be used to test any alternative algorithm.

EDIT: do not consider this.

Fixed the Rectangle class to cope with negative and float rotation angles. Added more test data in the PHPUnit test class to deal with negative, float and multiple of 30 degrees rotations. The latter one are the trickiest to deal with as it's where imprecision correction kicks in, really. There are 2000 test samples just for these.

#37 is green.

1. +++ b/core/lib/Drupal/Component/Utility/Rectangle.php
   @@ -0,0 +1,221 @@
   + * Different versions of PHP for the GD toolkit, and alternative toolkits,
   + * use different algorithms to perform the rotation of an image and result
   + * in different dimensions of the output image.
   + * This prevents predictability of the final image size for instance by the
   + * image rotate effect, and by the image toolkit rotate operation.
   

   The 2 paragraphs need to be separated with a line OR merged into one paragraph?

2. +++ b/core/lib/Drupal/Component/Utility/Rectangle.php
   @@ -0,0 +1,221 @@
   + * Here we are using an algorithm that produces the same results as PHP 5.5+
   

   Is this utility class needed only for PHP <5.5? If so, we can drop it because Drupal 8 requires >=5.5. Maybe I misunderstood the phrase.

3. +++ b/core/tests/Drupal/Tests/Component/Utility/RectangleTest.php
   @@ -0,0 +1,4299 @@
   +   * @return array
   

   @return array[]?

Looks good to me otherwise.

#42 - fixed all three points.

@claudiu.cristea - thanks for review. We need this utility class regardless of the PHP version. Its purpose is to calculate rotated image dimensions in abstract, without calling GD. This way, RotateImageEffect::transformDimensions can calculate expected dimensions after rotation on an algorithmic basis without calling GD primitives.

ImageMagick contrib also needs this (there's a copy of it in the contrib module), because since ImageMagick does not operate on an image in memory but produces a sequence of command line arguments for the convert binary, we need to be able to calculate dimensions without having the possibility to access an image resource. See #1551262: Rotate should alter dimensions.

Updated IS.

@mondrake, yes, that I understood. But if I remember well there's something related to precision in PHP 5.5 GD. So, based on the former comment, I thought, on first read, that the class is a 5.5 fix only. Now it makes more sense. Thank you.

+1 for the test :)

bot failure. back to RTBC per #45

Added beta eval.

#51 all done, thanks @fietserwin for review.

#51.2 - I missed that gd_rotate normalises angle to 0-360. This means that to have results for Rectangle that match gd_rotate, we should have test data that uses the GD toolkit instead of imagerotate() directly. This greatly simplifies the Rectangle class algorithm as most of the precision issues are related to negative angles multiple of 30° that are passed directly to imagerotate(). Still, our Rectangle utility class needs to return results for negative angles, so I kept test data for negative angles in the PHPUnit test.

There's an additional use case for this in #2583041: GD toolkit & operations should catch \Throwable to fail gracefully in case of errors.

Apparently, bot failure in #56

bots're dizzy today

Quite extensive test coverage in this patch!

Since this adds to the API and additional functionality, targeting against 8.1.x per https://www.drupal.org/core/d8-allowed-changes#minor. I'm also not entirely clear on how this is a bug (even after reading #2420751-9: ImageEffectBase::transformDimensions() should have a sane default implementation comments 9 and 12, but leaving that as-is for now).

Thanks everyone!

Also tagging for an issue summary update to help explain in more detail why this is a bug.

Hidden old files.

#63: in fact the most sensible description of the bug I can find is in #2420751-13: ImageEffectBase::transformDimensions() should have a sane default implementation:

Responsive images cannot output images in a srcset in combination with sizes, if the width is unknown, it would cause problems in the browser. Responsive images can work with images without a known dimension when the mapping is using a plain image effect.

Updated the IS accordingly. Whether this still qualifies as a bug I do not know, but it's definitely a feature needed by ImageMagick (see #2620786: Remove the Rectangle class and #2568619: Plan for ImageMagick 8) and potentially in core too (#2583041: GD toolkit & operations should catch \Throwable to fail gracefully in case of errors).

Given that this is a behaviour change I think putting this only in 8.1.x makes sense. I've ummed and ahhed about whether a change record is required - given that the outcome of this change is that rotated images always have dimensions if the angle is not random and imagemagick does not need the class I'm not sure that it is required.

I think it is a shame we don't have Drupal\Component\Image but making that now does not make a lot of sense since Color and Image already exist in Utility...

I'm not sure this can be ported to Drupal 7 since image dimension will change (to be consistent with PHP 5.5 GD regardless of which version) - this does not affect Drupal 8 because it only supports PHP5.5 and up.

Committed 1a6baa4 and pushed to 8.1.x. Thanks!

@alexpott re #66:

I have created a draft CR https://www.drupal.org/node/2645822, please review and publish if it's OK.
It takes into account #2420789: Make RotateImageEffect's random rotation deterministic, so ::transformDimensions() can return the actual dimensions, which will remove the last case of missing width/height attributes for random rotation angles.

Theoretically porting to D7 is still possible, since in the patch in #23 the Rectangle class can provide dimension calculation for PHP <= 5.4 while the one that was committed is for PHP 5.5 and above. So an implementation in D7 could check the PHP version in use and then use one or the other version to make the calculation. Not sure it's worth the effort, though.