From fd1e0bc4d3020d64ba32c97fc86f4ccbf2d243b9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 12 May 2015 11:20:24 -0700
Subject: [PATCH] Implement error-checked "preview" transforms

Summary: Ref T7707. These transforms have a single maximum dimension instead of fixed X and Y dimensions.

Test Plan: Transformed a bunch of files with different sizes/aspect ratios, got sensible results.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7707

Differential Revision: https://secure.phabricator.com/D12812
---
 .../PhabricatorFileImageTransform.php         |  7 +-
 .../PhabricatorFileThumbnailTransform.php     | 66 ++++++++++++++-----
 2 files changed, 52 insertions(+), 21 deletions(-)

diff --git a/src/applications/files/transform/PhabricatorFileImageTransform.php b/src/applications/files/transform/PhabricatorFileImageTransform.php
index ae5cbad305..4878b9e2c7 100644
--- a/src/applications/files/transform/PhabricatorFileImageTransform.php
+++ b/src/applications/files/transform/PhabricatorFileImageTransform.php
@@ -31,13 +31,14 @@ abstract class PhabricatorFileImageTransform extends PhabricatorFileTransform {
   protected function applyCropAndScale(
     $dst_w, $dst_h,
     $src_x, $src_y,
-    $src_w, $src_h) {
+    $src_w, $src_h,
+    $use_w, $use_h) {
 
     // Figure out the effective destination width, height, and offsets. We
     // never want to scale images up, so if we're copying a very small source
     // image we're just going to center it in the destination image.
-    $cpy_w = min($dst_w, $src_w);
-    $cpy_h = min($dst_h, $src_h);
+    $cpy_w = min($dst_w, $src_w, $use_w);
+    $cpy_h = min($dst_h, $src_h, $use_h);
     $off_x = ($dst_w - $cpy_w) / 2;
     $off_y = ($dst_h - $cpy_h) / 2;
 
diff --git a/src/applications/files/transform/PhabricatorFileThumbnailTransform.php b/src/applications/files/transform/PhabricatorFileThumbnailTransform.php
index 2921869c2c..45d7ad670a 100644
--- a/src/applications/files/transform/PhabricatorFileThumbnailTransform.php
+++ b/src/applications/files/transform/PhabricatorFileThumbnailTransform.php
@@ -59,32 +59,60 @@ final class PhabricatorFileThumbnailTransform
   }
 
   public function applyTransform(PhabricatorFile $file) {
-    $xformer = new PhabricatorImageTransformer();
-    if ($this->dstY === null) {
-      return $xformer->executePreviewTransform($file, $this->dstX);
-    }
-
     $this->willTransformFile($file);
 
     list($src_x, $src_y) = $this->getImageDimensions();
     $dst_x = $this->dstX;
     $dst_y = $this->dstY;
 
-    // Figure out how much we'd have to scale the image down along each
-    // dimension to get the entire thing to fit.
-    $scale_x = min(($dst_x / $src_x), 1);
-    $scale_y = min(($dst_y / $src_y), 1);
+    if ($dst_y === null) {
+      // If we only have one dimension, it represents a maximum dimension.
+      // The other dimension of the transform is scaled appropriately, except
+      // that we never generate images with crazily extreme aspect ratios.
+      if ($src_x < $src_y) {
+        // This is a tall, narrow image. Use the maximum dimension for the
+        // height and scale the width.
+        $use_y = $dst_x;
+        $dst_y = $dst_x;
 
-    if ($scale_x > $scale_y) {
-      // This image is relatively tall and narrow. We're going to crop off the
-      // top and bottom.
+        $use_x = $dst_y * ($src_x / $src_y);
+        $dst_x = max($dst_y / 4, $use_x);
+      } else {
+        // This is a short, wide image. Use the maximum dimension for the width
+        // and scale the height.
+        $use_x = $dst_x;
+
+        $use_y = $dst_x * ($src_y / $src_x);
+        $dst_y = max($dst_x / 4, $use_y);
+      }
+
+      // In this mode, we always copy the entire source image. We may generate
+      // margins in the output.
       $copy_x = $src_x;
-      $copy_y = min($src_y, $dst_y / $scale_x);
-    } else {
-      // This image is relatively short and wide. We're going to crop off the
-      // left and right.
-      $copy_x = min($src_x, $dst_x / $scale_y);
       $copy_y = $src_y;
+    } else {
+      // Otherwise, both dimensions are fixed. Figure out how much we'd have to
+      // scale the image down along each dimension to get the entire thing to
+      // fit.
+      $scale_x = min(($dst_x / $src_x), 1);
+      $scale_y = min(($dst_y / $src_y), 1);
+
+      if ($scale_x > $scale_y) {
+        // This image is relatively tall and narrow. We're going to crop off the
+        // top and bottom.
+        $copy_x = $src_x;
+        $copy_y = min($src_y, $dst_y / $scale_x);
+      } else {
+        // This image is relatively short and wide. We're going to crop off the
+        // left and right.
+        $copy_x = min($src_x, $dst_x / $scale_y);
+        $copy_y = $src_y;
+      }
+
+      // In this mode, we always use the entire destination image. We may
+      // crop the source input.
+      $use_x = $dst_x;
+      $use_y = $dst_y;
     }
 
     return $this->applyCropAndScale(
@@ -93,7 +121,9 @@ final class PhabricatorFileThumbnailTransform
       ($src_x - $copy_x) / 2,
       ($src_y - $copy_y) / 2,
       $copy_x,
-      $copy_y);
+      $copy_y,
+      $use_x,
+      $use_y);
   }
 
   public function getDefaultTransform(PhabricatorFile $file) {