Ensure scaling factors are calculated independently #452

Fixes bug introduced in v0.15.0 where, if the shrink operation
rounded up along one dimension, it could then also round up the
reduce operation on the same axis, creating a small stretch effect.

docs/changelog.md

@@ -18,6 +18,10 @@ Requires libvips v8.3.1
   [#443](https://github.com/lovell/sharp/pull/443)
   [@lemnisk8](https://github.com/lemnisk8)
 
+* Ensure scaling factors are calculated independently to prevent rounding errors.
+  [#452](https://github.com/lovell/sharp/issues/452)
+  [@puzrin](https://github.com/puzrin)
+
 * Add --sharp-cxx11 flag to compile with gcc's new C++11 ABI.
   [#456](https://github.com/lovell/sharp/pull/456)
   [@kapouer](https://github.com/kapouer)

src/pipeline.cc

@@ -219,34 +219,46 @@ class PipelineWorker : public AsyncWorker {
       // Scaling calculations
       double xfactor = 1.0;
       double yfactor = 1.0;
+      int targetResizeWidth = baton->width;
+      int targetResizeHeight = baton->height;
       if (baton->width > 0 && baton->height > 0) {
         // Fixed width and height
-        xfactor = static_cast<double>(inputWidth) / (static_cast<double>(baton->width) + 0.1);
+        xfactor = static_cast<double>(inputWidth) / static_cast<double>(baton->width);
-        yfactor = static_cast<double>(inputHeight) / (static_cast<double>(baton->height) + 0.1);
+        yfactor = static_cast<double>(inputHeight) / static_cast<double>(baton->height);
         switch (baton->canvas) {
           case Canvas::CROP:
-            xfactor = std::min(xfactor, yfactor);
+            if (xfactor < yfactor) {
+              targetResizeHeight = static_cast<int>(round(static_cast<double>(inputHeight) / xfactor));
               yfactor = xfactor;
+            } else {
+              targetResizeWidth = static_cast<int>(round(static_cast<double>(inputWidth) / yfactor));
+              xfactor = yfactor;
+            }
             break;
           case Canvas::EMBED:
-            xfactor = std::max(xfactor, yfactor);
+            if (xfactor > yfactor) {
+              targetResizeHeight = static_cast<int>(round(static_cast<double>(inputHeight) / xfactor));
               yfactor = xfactor;
+            } else {
+              targetResizeWidth = static_cast<int>(round(static_cast<double>(inputWidth) / yfactor));
+              xfactor = yfactor;
+            }
             break;
           case Canvas::MAX:
             if (xfactor > yfactor) {
-              baton->height = static_cast<int>(round(static_cast<double>(inputHeight) / xfactor));
+              targetResizeHeight = baton->height = static_cast<int>(round(static_cast<double>(inputHeight) / xfactor));
               yfactor = xfactor;
             } else {
-              baton->width = static_cast<int>(round(static_cast<double>(inputWidth) / yfactor));
+              targetResizeWidth = baton->width = static_cast<int>(round(static_cast<double>(inputWidth) / yfactor));
               xfactor = yfactor;
             }
             break;
           case Canvas::MIN:
             if (xfactor < yfactor) {
-              baton->height = static_cast<int>(round(static_cast<double>(inputHeight) / xfactor));
+              targetResizeHeight = baton->height = static_cast<int>(round(static_cast<double>(inputHeight) / xfactor));
               yfactor = xfactor;
             } else {
-              baton->width = static_cast<int>(round(static_cast<double>(inputWidth) / yfactor));
+              targetResizeWidth = baton->width = static_cast<int>(round(static_cast<double>(inputWidth) / yfactor));
               xfactor = yfactor;
             }
             break;
@@ -259,23 +271,23 @@ class PipelineWorker : public AsyncWorker {
         }
       } else if (baton->width > 0) {
         // Fixed width
-        xfactor = static_cast<double>(inputWidth) / (static_cast<double>(baton->width) + 0.1);
+        xfactor = static_cast<double>(inputWidth) / static_cast<double>(baton->width);
         if (baton->canvas == Canvas::IGNORE_ASPECT) {
-          baton->height = inputHeight;
+          targetResizeHeight = baton->height = inputHeight;
         } else {
           // Auto height
           yfactor = xfactor;
-          baton->height = static_cast<int>(round(static_cast<double>(inputHeight) / yfactor));
+          targetResizeHeight = baton->height = static_cast<int>(round(static_cast<double>(inputHeight) / yfactor));
         }
       } else if (baton->height > 0) {
         // Fixed height
-        yfactor = static_cast<double>(inputHeight) / (static_cast<double>(baton->height) + 0.1);
+        yfactor = static_cast<double>(inputHeight) / static_cast<double>(baton->height);
         if (baton->canvas == Canvas::IGNORE_ASPECT) {
-          baton->width = inputWidth;
+          targetResizeWidth = baton->width = inputWidth;
         } else {
           // Auto width
           xfactor = yfactor;
-          baton->width = static_cast<int>(round(static_cast<double>(inputWidth) / xfactor));
+          targetResizeWidth = baton->width = static_cast<int>(round(static_cast<double>(inputWidth) / xfactor));
         }
       } else {
         // Identity transform
@@ -429,20 +441,14 @@ class PipelineWorker : public AsyncWorker {
           // Swap input output width and height when rotating by 90 or 270 degrees
           std::swap(shrunkWidth, shrunkHeight);
         }
-        xresidual = static_cast<double>(baton->width) / static_cast<double>(shrunkWidth);
+        xresidual = static_cast<double>(targetResizeWidth) / static_cast<double>(shrunkWidth);
-        yresidual = static_cast<double>(baton->height) / static_cast<double>(shrunkHeight);
+        yresidual = static_cast<double>(targetResizeHeight) / static_cast<double>(shrunkHeight);
-        if (baton->canvas == Canvas::EMBED) {
+        if (
-          xresidual = std::min(xresidual, yresidual);
+          !baton->rotateBeforePreExtract &&
-          yresidual = xresidual;
+          (rotation == VIPS_ANGLE_D90 || rotation == VIPS_ANGLE_D270)
-        } else if (baton->canvas == Canvas::IGNORE_ASPECT) {
+        ) {
-          if (!baton->rotateBeforePreExtract &&
-            (rotation == VIPS_ANGLE_D90 || rotation == VIPS_ANGLE_D270)) {
           std::swap(xresidual, yresidual);
         }
-        } else {
-          xresidual = std::max(xresidual, yresidual);
-          yresidual = xresidual;
-        }
       }
 
       // Ensure image has an alpha channel when there is an overlay
@@ -673,10 +679,10 @@ class PipelineWorker : public AsyncWorker {
 
           // use gravity in ovelay
           if(overlayImageWidth <= baton->width) {
-            across = static_cast<int>(ceil(static_cast<double>(baton->width) / overlayImageWidth));
+            across = static_cast<int>(ceil(static_cast<double>(image.width()) / overlayImageWidth));
           }
           if(overlayImageHeight <= baton->height) {
-            down = static_cast<int>(ceil(static_cast<double>(baton->height) / overlayImageHeight));
+            down = static_cast<int>(ceil(static_cast<double>(image.height()) / overlayImageHeight));
           }
           if(across != 0 || down != 0) {
             int left;
@@ -684,10 +690,10 @@ class PipelineWorker : public AsyncWorker {
             overlayImage = overlayImage.replicate(across, down);
             // the overlayGravity will now be used to CalculateCrop for extract_area
             std::tie(left, top) = CalculateCrop(
-              overlayImage.width(), overlayImage.height(), baton->width, baton->height, baton->overlayGravity
+              overlayImage.width(), overlayImage.height(), image.width(), image.height(), baton->overlayGravity
             );
             overlayImage = overlayImage.extract_area(
-              left, top, baton->width, baton->height
+              left, top, image.width(), image.height()
             );
           }
           // the overlayGravity was used for extract_area, therefore set it back to its default value of 0