Prevent rounding err with shrink-on-load and 90/270 rot #1241

lovell · Jun 19, 2018 · da0b034 · da0b034
1 parent 0926345
commit da0b034
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 26 deletions.
diff --git a/docs/changelog.md b/docs/changelog.md
@@ -4,6 +4,12 @@
 
 Requires libvips v8.6.1.
 
+#### v0.20.3 - TBD
+
+* Prevent possible rounding error when using shrink-on-load and 90/270 degree rotation.
+  [#1241](https://github.com/lovell/sharp/issues/1241)
+  [@anahit42](https://github.com/anahit42)
+
 #### v0.20.3 - 29<sup>th</sup> May 2018
 
 * Fix tint operation by ensuring LAB interpretation and allowing negative values.

diff --git a/src/pipeline.cc b/src/pipeline.cc
@@ -281,15 +281,17 @@ class PipelineWorker : public Nan::AsyncWorker {
           }
         }
         // Recalculate integral shrink and double residual
-        int shrunkOnLoadWidth = image.width();
-        int shrunkOnLoadHeight = image.height();
+        int const shrunkOnLoadWidth = image.width();
+        int const shrunkOnLoadHeight = image.height();
         if (!baton->rotateBeforePreExtract &&
           (rotation == VIPS_ANGLE_D90 || rotation == VIPS_ANGLE_D270)) {
-          // Swap input output width and height when rotating by 90 or 270 degrees
-          std::swap(shrunkOnLoadWidth, shrunkOnLoadHeight);
+          // Swap when rotating by 90 or 270 degrees
+          xfactor = static_cast<double>(shrunkOnLoadWidth) / static_cast<double>(targetResizeHeight);
+          yfactor = static_cast<double>(shrunkOnLoadHeight) / static_cast<double>(targetResizeWidth);
+        } else {
+          xfactor = static_cast<double>(shrunkOnLoadWidth) / static_cast<double>(targetResizeWidth);
+          yfactor = static_cast<double>(shrunkOnLoadHeight) / static_cast<double>(targetResizeHeight);
         }
-        xfactor = static_cast<double>(shrunkOnLoadWidth) / static_cast<double>(targetResizeWidth);
-        yfactor = static_cast<double>(shrunkOnLoadHeight) / static_cast<double>(targetResizeHeight);
       }
 
       // Ensure we're using a device-independent colour space
@@ -381,7 +383,6 @@ class PipelineWorker : public Nan::AsyncWorker {
         ) {
           throw vips::VError("Unknown kernel");
         }
-
         image = image.resize(1.0 / xfactor, VImage::option()
           ->set("vscale", 1.0 / yfactor)
           ->set("kernel", kernel));

diff --git a/test/unit/crop.js b/test/unit/crop.js
@@ -177,25 +177,6 @@ describe('Crop', function () {
       });
   });
 
-  it('Clamp before crop when one post-resize dimension is below target', function () {
-    return sharp(fixtures.inputJpg)
-      .resize(1024, 1034)
-      .toBuffer()
-      .then(function (input) {
-        return sharp(input)
-          .rotate(270)
-          .resize(256)
-          .crop(sharp.strategy.entropy)
-          .toBuffer({ resolveWithObject: true })
-          .then(function (result) {
-            assert.strictEqual(256, result.info.width);
-            assert.strictEqual(253, result.info.height);
-            assert.strictEqual(0, result.info.cropOffsetLeft);
-            assert.strictEqual(0, result.info.cropOffsetTop);
-          });
-      });
-  });
-
   describe('Entropy-based strategy', function () {
     it('JPEG', function (done) {
       sharp(fixtures.inputJpg)

diff --git a/test/unit/resize.js b/test/unit/resize.js
@@ -130,6 +130,25 @@ describe('Resize dimensions', function () {
       });
   });
 
+  it('JPEG shrink-on-load with 90 degree rotation, ensure recalculation is correct', function (done) {
+    sharp(fixtures.inputJpg)
+      .resize(1920, 1280)
+      .toBuffer(function (err, data, info) {
+        if (err) throw err;
+        assert.strictEqual(1920, info.width);
+        assert.strictEqual(1280, info.height);
+        sharp(data)
+          .rotate(90)
+          .resize(533, 800)
+          .toBuffer(function (err, data, info) {
+            if (err) throw err;
+            assert.strictEqual(533, info.width);
+            assert.strictEqual(800, info.height);
+            done();
+          });
+      });
+  });
+
   it('TIFF embed known to cause rounding errors', function (done) {
     sharp(fixtures.inputTiff)
       .resize(240, 320)