Ensure autoOrient occurs before non-90 rotation #4425

- Separate orient vs rotate ordering logic
- Simplify EXIF auto-orient by using only rotate and/or flop

docs/src/content/docs/changelog/v0.34.4.md

@@ -3,5 +3,8 @@ title: v0.34.4 - TBD
 slug: changelog/v0.34.4
 ---
 
+* Ensure `autoOrient` occurs before non-90 angle rotation.
+  [#4425](https://github.com/lovell/sharp/issues/4425)
+
 * Ensure `autoOrient` removes existing metadata after shrink-on-load.
   [#4431](https://github.com/lovell/sharp/issues/4431)

lib/constructor.js

@@ -230,7 +230,8 @@ const Sharp = function (input, options) {
     angle: 0,
     rotationAngle: 0,
     rotationBackground: [0, 0, 0, 255],
-    rotateBeforePreExtract: false,
+    rotateBefore: false,
+    orientBefore: false,
     flip: false,
     flop: false,
     extendTop: 0,

lib/resize.js

@@ -107,7 +107,7 @@ const mapFitToCanvas = {
  * @private
  */
 function isRotationExpected (options) {
-  return (options.angle % 360) !== 0 || options.input.autoOrient === true || options.rotationAngle !== 0;
+  return (options.angle % 360) !== 0 || options.rotationAngle !== 0;
 }
 
 /**
@@ -343,7 +343,7 @@ function resize (widthOrOptions, height, options) {
     }
   }
   if (isRotationExpected(this.options) && isResizeExpected(this.options)) {
-    this.options.rotateBeforePreExtract = true;
+    this.options.rotateBefore = true;
   }
   return this;
 }
@@ -490,9 +490,12 @@ function extract (options) {
   // Ensure existing rotation occurs before pre-resize extraction
   if (isRotationExpected(this.options) && !isResizeExpected(this.options)) {
     if (this.options.widthPre === -1 || this.options.widthPost === -1) {
-      this.options.rotateBeforePreExtract = true;
+      this.options.rotateBefore = true;
     }
   }
+  if (this.options.input.autoOrient) {
+    this.options.orientBefore = true;
+  }
   return this;
 }
 
@@ -566,7 +569,7 @@ function trim (options) {
     }
   }
   if (isRotationExpected(this.options)) {
-    this.options.rotateBeforePreExtract = true;
+    this.options.rotateBefore = true;
   }
   return this;
 }

src/pipeline.cc

@@ -92,30 +92,22 @@ class PipelineWorker : public Napi::AsyncWorker {
       // Calculate angle of rotation
       VipsAngle rotation = VIPS_ANGLE_D0;
       VipsAngle autoRotation = VIPS_ANGLE_D0;
-      bool autoFlip = false;
       bool autoFlop = false;
 
       if (baton->input->autoOrient) {
         // Rotate and flip image according to Exif orientation
-        std::tie(autoRotation, autoFlip, autoFlop) = CalculateExifRotationAndFlip(sharp::ExifOrientation(image));
+        std::tie(autoRotation, autoFlop) = CalculateExifRotationAndFlop(sharp::ExifOrientation(image));
       }
 
       rotation = CalculateAngleRotation(baton->angle);
 
-      // Rotate pre-extract
+      bool const shouldRotateBefore = baton->rotateBefore &&
-      bool const shouldRotateBefore = baton->rotateBeforePreExtract &&
+        (rotation != VIPS_ANGLE_D0 || baton->flip || baton->flop || baton->rotationAngle != 0.0);
-        (rotation != VIPS_ANGLE_D0 || autoRotation != VIPS_ANGLE_D0 ||
+      bool const shouldOrientBefore = (shouldRotateBefore || baton->orientBefore) &&
-          autoFlip || baton->flip || autoFlop || baton->flop ||
+        (autoRotation != VIPS_ANGLE_D0 || autoFlop);
-          baton->rotationAngle != 0.0);
-
-      if (shouldRotateBefore) {
-        image = sharp::StaySequential(image,
-          rotation != VIPS_ANGLE_D0 ||
-          autoRotation != VIPS_ANGLE_D0 ||
-          autoFlip ||
-          baton->flip ||
-          baton->rotationAngle != 0.0);
 
+      if (shouldOrientBefore) {
+        image = sharp::StaySequential(image, autoRotation != VIPS_ANGLE_D0);
         if (autoRotation != VIPS_ANGLE_D0) {
           if (autoRotation != VIPS_ANGLE_D180) {
             MultiPageUnsupported(nPages, "Rotate");
@@ -123,14 +115,20 @@ class PipelineWorker : public Napi::AsyncWorker {
           image = image.rot(autoRotation);
           autoRotation = VIPS_ANGLE_D0;
         }
-        if (autoFlip != baton->flip) {
+        if (autoFlop) {
-          image = image.flip(VIPS_DIRECTION_VERTICAL);
-          autoFlip = false;
-          baton->flip = false;
-        }
-        if (autoFlop != baton->flop) {
           image = image.flip(VIPS_DIRECTION_HORIZONTAL);
           autoFlop = false;
+        }
+      }
+
+      if (shouldRotateBefore) {
+        image = sharp::StaySequential(image, rotation != VIPS_ANGLE_D0 || baton->flip || baton->rotationAngle != 0.0);
+        if (baton->flip) {
+          image = image.flip(VIPS_DIRECTION_VERTICAL);
+          baton->flip = false;
+        }
+        if (baton->flop) {
+          image = image.flip(VIPS_DIRECTION_HORIZONTAL);
           baton->flop = false;
         }
         if (rotation != VIPS_ANGLE_D0) {
@@ -145,6 +143,7 @@ class PipelineWorker : public Napi::AsyncWorker {
           std::vector<double> background;
           std::tie(image, background) = sharp::ApplyAlpha(image, baton->rotationBackground, false);
           image = image.rotate(baton->rotationAngle, VImage::option()->set("background", background)).copy_memory();
+          baton->rotationAngle = 0.0;
         }
       }
 
@@ -183,8 +182,7 @@ class PipelineWorker : public Napi::AsyncWorker {
       // When auto-rotating by 90 or 270 degrees, swap the target width and
       // height to ensure the behavior aligns with how it would have been if
       // the rotation had taken place *before* resizing.
-      if (!baton->rotateBeforePreExtract &&
+      if (autoRotation == VIPS_ANGLE_D90 || autoRotation == VIPS_ANGLE_D270) {
-        (autoRotation == VIPS_ANGLE_D90 || autoRotation == VIPS_ANGLE_D270)) {
         std::swap(targetResizeWidth, targetResizeHeight);
       }
 
@@ -206,7 +204,7 @@ class PipelineWorker : public Napi::AsyncWorker {
       //  - input colourspace is not specified;
       bool const shouldPreShrink = (targetResizeWidth > 0 || targetResizeHeight > 0) &&
         baton->gamma == 0 && baton->topOffsetPre == -1 && baton->trimThreshold < 0.0 &&
-        baton->colourspacePipeline == VIPS_INTERPRETATION_LAST && !shouldRotateBefore;
+        baton->colourspacePipeline == VIPS_INTERPRETATION_LAST && !(shouldOrientBefore || shouldRotateBefore);
 
       if (shouldPreShrink) {
         // The common part of the shrink: the bit by which both axes must be shrunk
@@ -398,7 +396,6 @@ class PipelineWorker : public Napi::AsyncWorker {
       image = sharp::StaySequential(image,
         autoRotation != VIPS_ANGLE_D0 ||
         baton->flip ||
-        autoFlip ||
         rotation != VIPS_ANGLE_D0);
       // Auto-rotate post-extract
       if (autoRotation != VIPS_ANGLE_D0) {
@@ -408,7 +405,7 @@ class PipelineWorker : public Napi::AsyncWorker {
         image = image.rot(autoRotation);
       }
       // Mirror vertically (up-down) about the x-axis
-      if (baton->flip != autoFlip) {
+      if (baton->flip) {
         image = image.flip(VIPS_DIRECTION_VERTICAL);
       }
       // Mirror horizontally (left-right) about the y-axis
@@ -515,7 +512,7 @@ class PipelineWorker : public Napi::AsyncWorker {
       }
 
       // Rotate post-extract non-90 angle
-      if (!baton->rotateBeforePreExtract && baton->rotationAngle != 0.0) {
+      if (!baton->rotateBefore && baton->rotationAngle != 0.0) {
         MultiPageUnsupported(nPages, "Rotate");
         image = sharp::StaySequential(image);
         std::vector<double> background;
@@ -656,21 +653,16 @@ class PipelineWorker : public Napi::AsyncWorker {
           if (composite->input->autoOrient) {
             // Respect EXIF Orientation
             VipsAngle compositeAutoRotation = VIPS_ANGLE_D0;
-            bool compositeAutoFlip = false;
             bool compositeAutoFlop = false;
-            std::tie(compositeAutoRotation, compositeAutoFlip, compositeAutoFlop) =
+            std::tie(compositeAutoRotation, compositeAutoFlop) =
-              CalculateExifRotationAndFlip(sharp::ExifOrientation(compositeImage));
+              CalculateExifRotationAndFlop(sharp::ExifOrientation(compositeImage));
 
             compositeImage = sharp::RemoveExifOrientation(compositeImage);
-            compositeImage = sharp::StaySequential(compositeImage,
+            compositeImage = sharp::StaySequential(compositeImage, compositeAutoRotation != VIPS_ANGLE_D0);
-              compositeAutoRotation != VIPS_ANGLE_D0 || compositeAutoFlip);
 
             if (compositeAutoRotation != VIPS_ANGLE_D0) {
               compositeImage = compositeImage.rot(compositeAutoRotation);
             }
-            if (compositeAutoFlip) {
-              compositeImage = compositeImage.flip(VIPS_DIRECTION_VERTICAL);
-            }
             if (compositeAutoFlop) {
               compositeImage = compositeImage.flip(VIPS_DIRECTION_HORIZONTAL);
             }
@@ -1402,21 +1394,20 @@ class PipelineWorker : public Napi::AsyncWorker {
     Calculate the angle of rotation and need-to-flip for the given Exif orientation
     By default, returns zero, i.e. no rotation.
   */
-  std::tuple<VipsAngle, bool, bool>
+  std::tuple<VipsAngle, bool>
-  CalculateExifRotationAndFlip(int const exifOrientation) {
+  CalculateExifRotationAndFlop(int const exifOrientation) {
     VipsAngle rotate = VIPS_ANGLE_D0;
-    bool flip = false;
     bool flop = false;
     switch (exifOrientation) {
       case 6: rotate = VIPS_ANGLE_D90; break;
       case 3: rotate = VIPS_ANGLE_D180; break;
       case 8: rotate = VIPS_ANGLE_D270; break;
-      case 2: flop = true; break;  // flop 1
+      case 2: flop = true; break;
-      case 7: flip = true; rotate = VIPS_ANGLE_D90; break;  // flip 6
+      case 7: flop = true; rotate = VIPS_ANGLE_D270; break;
-      case 4: flop = true; rotate = VIPS_ANGLE_D180; break;  // flop 3
+      case 4: flop = true; rotate = VIPS_ANGLE_D180; break;
-      case 5: flip = true; rotate = VIPS_ANGLE_D270; break;  // flip 8
+      case 5: flop = true; rotate = VIPS_ANGLE_D90; break;
     }
-    return std::make_tuple(rotate, flip, flop);
+    return std::make_tuple(rotate, flop);
   }
 
   /*
@@ -1641,7 +1632,8 @@ Napi::Value pipeline(const Napi::CallbackInfo& info) {
   baton->angle = sharp::AttrAsInt32(options, "angle");
   baton->rotationAngle = sharp::AttrAsDouble(options, "rotationAngle");
   baton->rotationBackground = sharp::AttrAsVectorOfDouble(options, "rotationBackground");
-  baton->rotateBeforePreExtract = sharp::AttrAsBool(options, "rotateBeforePreExtract");
+  baton->rotateBefore = sharp::AttrAsBool(options, "rotateBefore");
+  baton->orientBefore = sharp::AttrAsBool(options, "orientBefore");
   baton->flip = sharp::AttrAsBool(options, "flip");
   baton->flop = sharp::AttrAsBool(options, "flop");
   baton->extendTop = sharp::AttrAsInt32(options, "extendTop");

src/pipeline.h

@@ -115,7 +115,8 @@ struct PipelineBaton {
   int angle;
   double rotationAngle;
   std::vector<double> rotationBackground;
-  bool rotateBeforePreExtract;
+  bool rotateBefore;
+  bool orientBefore;
   bool flip;
   bool flop;
   int extendTop;

test/unit/rotate.js

@@ -565,9 +565,9 @@ describe('Rotation', function () {
       .raw()
       .toBuffer();
 
-    assert.strictEqual(r, 60);
+    assert.strictEqual(r, 61);
-    assert.strictEqual(g, 73);
+    assert.strictEqual(g, 74);
-    assert.strictEqual(b, 52);
+    assert.strictEqual(b, 51);
   });
 
   it('Flip and rotate ordering', async () => {
@@ -648,6 +648,27 @@ describe('Rotation', function () {
     assert.strictEqual(orientation, undefined);
   });
 
+  it('Auto-orient and rotate 45', async () => {
+    const data = await sharp(fixtures.inputJpgWithLandscapeExif2, { autoOrient: true })
+      .rotate(45)
+      .toBuffer();
+
+    const { width, height } = await sharp(data).metadata();
+    assert.strictEqual(width, 742);
+    assert.strictEqual(height, 742);
+  });
+
+  it('Auto-orient, extract and rotate 45', async () => {
+    const data = await sharp(fixtures.inputJpgWithLandscapeExif2, { autoOrient: true })
+      .extract({ left: 20, top: 20, width: 200, height: 100 })
+      .rotate(45)
+      .toBuffer();
+
+    const { width, height } = await sharp(data).metadata();
+    assert.strictEqual(width, 212);
+    assert.strictEqual(height, 212);
+  });
+
   it('Invalid autoOrient throws', () =>
     assert.throws(
       () => sharp({ autoOrient: 'fail' }),