Ensure op ordering is respected where possible #3319

Emit warnings when previous ops might be ignored
Flip and flop now occur before rotate, if any

docs/api-operation.md

@@ -16,8 +16,11 @@ Mirroring is supported and may infer the use of a flip operation.
 
 The use of `rotate` implies the removal of the EXIF `Orientation` tag, if any.
 
-Method order is important when both rotating and extracting regions,
-for example `rotate(x).extract(y)` will produce a different result to `extract(y).rotate(x)`.
+Only one rotation can occur per pipeline.
+Previous calls to `rotate` in the same pipeline will be ignored.
+
+Method order is important when rotating, resizing and/or extracting regions,
+for example `.rotate(x).extract(y)` will produce a different result to `.extract(y).rotate(x)`.
 
 ### Parameters
 
@@ -40,13 +43,24 @@ const pipeline = sharp()
 readableStream.pipe(pipeline);
 ```
 
+```javascript
+const rotateThenResize = await sharp(input)
+  .rotate(90)
+  .resize({ width: 16, height: 8, fit: 'fill' })
+  .toBuffer();
+const resizeThenRotate = await sharp(input)
+  .resize({ width: 16, height: 8, fit: 'fill' })
+  .rotate(90)
+  .toBuffer();
+```
+
 *   Throws **[Error][5]** Invalid parameters
 
 Returns **Sharp** 
 
 ## flip
 
-Flip the image about the vertical Y axis. This always occurs after rotation, if any.
+Flip the image about the vertical Y axis. This always occurs before rotation, if any.
 The use of `flip` implies the removal of the EXIF `Orientation` tag, if any.
 
 ### Parameters
@@ -63,7 +77,7 @@ Returns **Sharp**
 
 ## flop
 
-Flop the image about the horizontal X axis. This always occurs after rotation, if any.
+Flop the image about the horizontal X axis. This always occurs before rotation, if any.
 The use of `flop` implies the removal of the EXIF `Orientation` tag, if any.
 
 ### Parameters

docs/api-resize.md

@@ -36,6 +36,9 @@ Possible interpolation kernels are:
 *   `lanczos2`: Use a [Lanczos kernel][7] with `a=2`.
 *   `lanczos3`: Use a Lanczos kernel with `a=3` (the default).
 
+Only one resize can occur per pipeline.
+Previous calls to `resize` in the same pipeline will be ignored.
+
 ### Parameters
 
 *   `width` **[number][8]?** pixels wide the resultant image should be. Use `null` or `undefined` to auto-scale the width to match the height.

docs/changelog.md

@@ -14,6 +14,8 @@ Requires libvips v8.13.0
 
 * Remove previously-deprecated WebP `reductionEffort` and HEIF `speed` options. Use `effort` to control these.
 
+* The `flip` and `flop` operations will now occur before the `rotate` operation.
+
 * Use combined bounding box of alpha and non-alpha channels for `trim` operation.
   [#2166](https://github.com/lovell/sharp/issues/2166)
 
@@ -42,6 +44,10 @@ Requires libvips v8.13.0
 * Ensure only properties owned by the `withMetadata` EXIF Object are parsed.
   [#3292](https://github.com/lovell/sharp/issues/3292)
 
+* Ensure the order of `rotate`, `resize` and `extend` operations is respected where possible.
+  Emit warnings when previous calls in the same pipeline will be ignored.
+  [#3319](https://github.com/lovell/sharp/issues/3319)
+
 ## v0.30 - *dresser*
 
 Requires libvips v8.12.2

lib/operation.js

@@ -18,8 +18,11 @@ const is = require('./is');
  *
  * The use of `rotate` implies the removal of the EXIF `Orientation` tag, if any.
  *
- * Method order is important when both rotating and extracting regions,
- * for example `rotate(x).extract(y)` will produce a different result to `extract(y).rotate(x)`.
+ * Only one rotation can occur per pipeline.
+ * Previous calls to `rotate` in the same pipeline will be ignored.
+ *
+ * Method order is important when rotating, resizing and/or extracting regions,
+ * for example `.rotate(x).extract(y)` will produce a different result to `.extract(y).rotate(x)`.
  *
  * @example
  * const pipeline = sharp()
@@ -32,13 +35,26 @@ const is = require('./is');
  *   });
  * readableStream.pipe(pipeline);
  *
+ * @example
+ * const rotateThenResize = await sharp(input)
+ *   .rotate(90)
+ *   .resize({ width: 16, height: 8, fit: 'fill' })
+ *   .toBuffer();
+ * const resizeThenRotate = await sharp(input)
+ *   .resize({ width: 16, height: 8, fit: 'fill' })
+ *   .rotate(90)
+ *   .toBuffer();
+ *
  * @param {number} [angle=auto] angle of rotation.
  * @param {Object} [options] - if present, is an Object with optional attributes.
  * @param {string|Object} [options.background="#000000"] parsed by the [color](https://www.npmjs.org/package/color) module to extract values for red, green, blue and alpha.
  * @returns {Sharp}
  * @throws {Error} Invalid parameters
  */
 function rotate (angle, options) {
+  if (this.options.useExifOrientation || this.options.angle || this.options.rotationAngle) {
+    this.options.debuglog('ignoring previous rotate options');
+  }
   if (!is.defined(angle)) {
     this.options.useExifOrientation = true;
   } else if (is.integer(angle) && !(angle % 90)) {
@@ -61,7 +77,7 @@ function rotate (angle, options) {
 }
 
 /**
- * Flip the image about the vertical Y axis. This always occurs after rotation, if any.
+ * Flip the image about the vertical Y axis. This always occurs before rotation, if any.
  * The use of `flip` implies the removal of the EXIF `Orientation` tag, if any.
  *
  * @example
@@ -76,7 +92,7 @@ function flip (flip) {
 }
 
 /**
- * Flop the image about the horizontal X axis. This always occurs after rotation, if any.
+ * Flop the image about the horizontal X axis. This always occurs before rotation, if any.
  * The use of `flop` implies the removal of the EXIF `Orientation` tag, if any.
  *
  * @example

lib/resize.js

@@ -92,6 +92,13 @@ function isRotationExpected (options) {
   return (options.angle % 360) !== 0 || options.useExifOrientation === true || options.rotationAngle !== 0;
 }
 
+/**
+ * @private
+ */
+function isResizeExpected (options) {
+  return options.width !== -1 || options.height !== -1;
+}
+
 /**
  * Resize image to `width`, `height` or `width x height`.
  *
@@ -123,6 +130,9 @@ function isRotationExpected (options) {
  * - `lanczos2`: Use a [Lanczos kernel](https://en.wikipedia.org/wiki/Lanczos_resampling#Lanczos_kernel) with `a=2`.
  * - `lanczos3`: Use a Lanczos kernel with `a=3` (the default).
  *
+ * Only one resize can occur per pipeline.
+ * Previous calls to `resize` in the same pipeline will be ignored.
+ *
  * @example
  * sharp(input)
  *   .resize({ width: 100 })
@@ -220,6 +230,9 @@ function isRotationExpected (options) {
  * @throws {Error} Invalid parameters
  */
 function resize (width, height, options) {
+  if (isResizeExpected(this.options)) {
+    this.options.debuglog('ignoring previous resize options');
+  }
   if (is.defined(width)) {
     if (is.object(width) && !is.defined(options)) {
       options = width;
@@ -300,6 +313,9 @@ function resize (width, height, options) {
       this._setBooleanOption('fastShrinkOnLoad', options.fastShrinkOnLoad);
     }
   }
+  if (isRotationExpected(this.options) && isResizeExpected(this.options)) {
+    this.options.rotateBeforePreExtract = true;
+  }
   return this;
 }
 
@@ -412,7 +428,10 @@ function extend (extend) {
  * @throws {Error} Invalid parameters
  */
 function extract (options) {
-  const suffix = this.options.width === -1 && this.options.height === -1 ? 'Pre' : 'Post';
+  const suffix = isResizeExpected(this.options) || isRotationExpected(this.options) ? 'Post' : 'Pre';
+  if (this.options[`width${suffix}`] !== -1) {
+    this.options.debuglog('ignoring previous extract options');
+  }
   ['left', 'top', 'width', 'height'].forEach(function (name) {
     const value = options[name];
     if (is.integer(value) && value >= 0) {
@@ -422,8 +441,10 @@ function extract (options) {
     }
   }, this);
   // Ensure existing rotation occurs before pre-resize extraction
-  if (suffix === 'Pre' && isRotationExpected(this.options)) {
-    this.options.rotateBeforePreExtract = true;
+  if (isRotationExpected(this.options) && !isResizeExpected(this.options)) {
+    if (this.options.widthPre === -1 || this.options.widthPost === -1) {
+      this.options.rotateBeforePreExtract = true;
+    }
   }
   return this;
 }

src/common.cc

@@ -951,7 +951,7 @@ namespace sharp {
 
   std::pair<double, double> ResolveShrink(int width, int height, int targetWidth, int targetHeight,
     Canvas canvas, bool swap, bool withoutEnlargement, bool withoutReduction) {
-    if (swap) {
+    if (swap && canvas != Canvas::IGNORE_ASPECT) {
       // Swap input width and height when requested.
       std::swap(width, height);
     }
@@ -982,9 +982,6 @@ namespace sharp {
           }
           break;
         case Canvas::IGNORE_ASPECT:
-          if (swap) {
-            std::swap(hshrink, vshrink);
-          }
           break;
       }
     } else if (targetWidth > 0) {

src/pipeline.cc

@@ -387,6 +387,18 @@ class PipelineWorker : public Napi::AsyncWorker {
           ->set("kernel", kernel));
       }
 
+      // Flip (mirror about Y axis)
+      if (baton->flip || flip) {
+        image = image.flip(VIPS_DIRECTION_VERTICAL);
+        image = sharp::RemoveExifOrientation(image);
+      }
+
+      // Flop (mirror about X axis)
+      if (baton->flop || flop) {
+        image = image.flip(VIPS_DIRECTION_HORIZONTAL);
+        image = sharp::RemoveExifOrientation(image);
+      }
+
       // Rotate post-extract 90-angle
       if (!baton->rotateBeforePreExtract && rotation != VIPS_ANGLE_D0) {
         image = image.rot(rotation);
@@ -401,18 +413,6 @@ class PipelineWorker : public Napi::AsyncWorker {
         image = sharp::RemoveExifOrientation(image);
       }
 
-      // Flip (mirror about Y axis)
-      if (baton->flip || flip) {
-        image = image.flip(VIPS_DIRECTION_VERTICAL);
-        image = sharp::RemoveExifOrientation(image);
-      }
-
-      // Flop (mirror about X axis)
-      if (baton->flop || flop) {
-        image = image.flip(VIPS_DIRECTION_HORIZONTAL);
-        image = sharp::RemoveExifOrientation(image);
-      }
-
       // Join additional color channels to the image
       if (baton->joinChannelIn.size() > 0) {
         VImage joinImage;

test/unit/extract.js

@@ -138,7 +138,20 @@ describe('Partial image extraction', function () {
         if (err) throw err;
         assert.strictEqual(280, info.width);
         assert.strictEqual(380, info.height);
-        fixtures.assertSimilar(fixtures.expected('rotate-extract.jpg'), data, { threshold: 7 }, done);
+        fixtures.assertSimilar(fixtures.expected('rotate-extract.jpg'), data, done);
+      });
+  });
+
+  it('Extract then rotate then extract', function (done) {
+    sharp(fixtures.inputPngWithGreyAlpha)
+      .extract({ left: 20, top: 10, width: 180, height: 280 })
+      .rotate(90)
+      .extract({ left: 20, top: 10, width: 200, height: 100 })
+      .toBuffer(function (err, data, info) {
+        if (err) throw err;
+        assert.strictEqual(200, info.width);
+        assert.strictEqual(100, info.height);
+        fixtures.assertSimilar(fixtures.expected('extract-rotate-extract.jpg'), data, done);
       });
   });
 
@@ -164,7 +177,7 @@ describe('Partial image extraction', function () {
         if (err) throw err;
         assert.strictEqual(380, info.width);
         assert.strictEqual(280, info.height);
-        fixtures.assertSimilar(fixtures.expected('rotate-extract-45.jpg'), data, { threshold: 7 }, done);
+        fixtures.assertSimilar(fixtures.expected('rotate-extract-45.jpg'), data, done);
       });
   });
 
@@ -281,5 +294,27 @@ describe('Partial image extraction', function () {
           done();
         });
     });
+
+    it('Multiple extract emits warning', () => {
+      let warningMessage = '';
+      const s = sharp();
+      s.on('warning', function (msg) { warningMessage = msg; });
+      const options = { top: 0, left: 0, width: 1, height: 1 };
+      s.extract(options);
+      assert.strictEqual(warningMessage, '');
+      s.extract(options);
+      assert.strictEqual(warningMessage, 'ignoring previous extract options');
+    });
+
+    it('Multiple rotate+extract emits warning', () => {
+      let warningMessage = '';
+      const s = sharp().rotate();
+      s.on('warning', function (msg) { warningMessage = msg; });
+      const options = { top: 0, left: 0, width: 1, height: 1 };
+      s.extract(options);
+      assert.strictEqual(warningMessage, '');
+      s.extract(options);
+      assert.strictEqual(warningMessage, 'ignoring previous extract options');
+    });
   });
 });

test/unit/resize.js

@@ -791,4 +791,14 @@ describe('Resize dimensions', function () {
       sharp().resize(null, null, { position: 'unknown' });
     });
   });
+
+  it('Multiple resize emits warning', () => {
+    let warningMessage = '';
+    const s = sharp();
+    s.on('warning', function (msg) { warningMessage = msg; });
+    s.resize(1);
+    assert.strictEqual(warningMessage, '');
+    s.resize(2);
+    assert.strictEqual(warningMessage, 'ignoring previous resize options');
+  });
 });