Made display thumbnail generation use original data if smaller

Thumbnail generation would sometimes create a file larger than the
original, if the original was already well optimized, therefore making
the thumbnail counter-productive. This change compares the sizes of the
original and the generated thumbnail, and uses the smaller of the two if
the thumbnail does not change the aspect ratio of the image.

Fixes #1751

diff --git a/app/Uploads/ImageRepo.php b/app/Uploads/ImageRepo.php
index da0b7d379eddd2fb4a227dd071af248c2ba44d07..01b65f882698933b54f66ba51dcee3f885aaf5cc 100644 (file)
--- a/app/Uploads/ImageRepo.php
+++ b/app/Uploads/ImageRepo.php
@@ -2,6 +2,8 @@
 
 use BookStack\Auth\Permissions\PermissionService;
 use BookStack\Entities\Page;
+use BookStack\Exceptions\ImageUploadException;
+use Exception;
 use Illuminate\Database\Eloquent\Builder;
 use Symfony\Component\HttpFoundation\File\UploadedFile;
 
@@ -15,10 +17,6 @@ class ImageRepo
 
     /**
      * ImageRepo constructor.
-     * @param Image $image
-     * @param ImageService $imageService
-     * @param \BookStack\Auth\Permissions\PermissionService $permissionService
-     * @param \BookStack\Entities\Page $page
      */
     public function __construct(
         Image $image,
@@ -35,10 +33,8 @@ class ImageRepo
 
     /**
      * Get an image with the given id.
-     * @param $id
-     * @return Image
      */
-    public function getById($id)
+    public function getById($id): Image
     {
         return $this->image->findOrFail($id);
     }
@@ -46,13 +42,8 @@ class ImageRepo
     /**
      * Execute a paginated query, returning in a standard format.
      * Also runs the query through the restriction system.
-     * @param $query
-     * @param int $page
-     * @param int $pageSize
-     * @param bool $filterOnPage
-     * @return array
      */
-    private function returnPaginated($query, $page = 1, $pageSize = 24)
+    private function returnPaginated($query, $page = 1, $pageSize = 24): array
     {
         $images = $query->orderBy('created_at', 'desc')->skip($pageSize * ($page - 1))->take($pageSize + 1)->get();
         $hasMore = count($images) > $pageSize;
@@ -71,13 +62,6 @@ class ImageRepo
     /**
      * Fetch a list of images in a paginated format, filtered by image type.
      * Can be filtered by uploaded to and also by name.
-     * @param string $type
-     * @param int $page
-     * @param int $pageSize
-     * @param int $uploadedTo
-     * @param string|null $search
-     * @param callable|null $whereClause
-     * @return array
      */
     public function getPaginatedByType(
         string $type,
@@ -86,7 +70,8 @@ class ImageRepo
         int $uploadedTo = null,
         string $search = null,
         callable $whereClause = null
-    ) {
+    ): array
+    {
         $imageQuery = $this->image->newQuery()->where('type', '=', strtolower($type));
 
         if ($uploadedTo !== null) {
@@ -109,13 +94,6 @@ class ImageRepo
 
     /**
      * Get paginated gallery images within a specific page or book.
-     * @param string $type
-     * @param string $filterType
-     * @param int $page
-     * @param int $pageSize
-     * @param int|null $uploadedTo
-     * @param string|null $search
-     * @return array
      */
     public function getEntityFiltered(
         string $type,
@@ -124,7 +102,8 @@ class ImageRepo
         int $pageSize = 24,
         int $uploadedTo = null,
         string $search = null
-    ) {
+    ): array
+    {
         $contextPage = $this->page->findOrFail($uploadedTo);
         $parentFilter = null;
 
@@ -144,16 +123,9 @@ class ImageRepo
 
     /**
      * Save a new image into storage and return the new image.
-     * @param UploadedFile $uploadFile
-     * @param string $type
-     * @param int $uploadedTo
-     * @param int|null $resizeWidth
-     * @param int|null $resizeHeight
-     * @param bool $keepRatio
-     * @return Image
-     * @throws \BookStack\Exceptions\ImageUploadException
+     * @throws ImageUploadException
      */
-    public function saveNew(UploadedFile $uploadFile, $type, $uploadedTo = 0, int $resizeWidth = null, int $resizeHeight = null, bool $keepRatio = true)
+    public function saveNew(UploadedFile $uploadFile, string $type, int $uploadedTo = 0, int $resizeWidth = null, int $resizeHeight = null, bool $keepRatio = true): Image
     {
         $image = $this->imageService->saveNewFromUpload($uploadFile, $type, $uploadedTo, $resizeWidth, $resizeHeight, $keepRatio);
         $this->loadThumbs($image);
@@ -161,29 +133,22 @@ class ImageRepo
     }
 
     /**
-     * Save a drawing the the database;
-     * @param string $base64Uri
-     * @param int $uploadedTo
-     * @return Image
-     * @throws \BookStack\Exceptions\ImageUploadException
+     * Save a drawing the the database.
+     * @throws ImageUploadException
      */
-    public function saveDrawing(string $base64Uri, int $uploadedTo)
+    public function saveDrawing(string $base64Uri, int $uploadedTo): Image
     {
         $name = 'Drawing-' . user()->getShortName(40) . '-' . strval(time()) . '.png';
-        $image = $this->imageService->saveNewFromBase64Uri($base64Uri, $name, 'drawio', $uploadedTo);
-        return $image;
+        return $this->imageService->saveNewFromBase64Uri($base64Uri, $name, 'drawio', $uploadedTo);
     }
 
 
     /**
      * Update the details of an image via an array of properties.
-     * @param Image $image
-     * @param array $updateDetails
-     * @return Image
-     * @throws \BookStack\Exceptions\ImageUploadException
-     * @throws \Exception
+     * @throws ImageUploadException
+     * @throws Exception
      */
-    public function updateImageDetails(Image $image, $updateDetails)
+    public function updateImageDetails(Image $image, $updateDetails): Image
     {
         $image->fill($updateDetails);
         $image->save();
@@ -191,14 +156,11 @@ class ImageRepo
         return $image;
     }
 
-
     /**
      * Destroys an Image object along with its revisions, files and thumbnails.
-     * @param Image $image
-     * @return bool
-     * @throws \Exception
+     * @throws Exception
      */
-    public function destroyImage(Image $image = null)
+    public function destroyImage(Image $image = null): bool
     {
         if ($image) {
             $this->imageService->destroy($image);
@@ -208,8 +170,7 @@ class ImageRepo
 
     /**
      * Destroy all images of a certain type.
-     * @param string $imageType
-     * @throws \Exception
+     * @throws Exception
      */
     public function destroyByType(string $imageType)
     {
@@ -222,9 +183,7 @@ class ImageRepo
 
     /**
      * Load thumbnails onto an image object.
-     * @param Image $image
-     * @throws \BookStack\Exceptions\ImageUploadException
-     * @throws \Exception
+     * @throws Exception
      */
     protected function loadThumbs(Image $image)
     {
@@ -238,42 +197,33 @@ class ImageRepo
      * Get the thumbnail for an image.
      * If $keepRatio is true only the width will be used.
      * Checks the cache then storage to avoid creating / accessing the filesystem on every check.
-     * @param Image $image
-     * @param int $width
-     * @param int $height
-     * @param bool $keepRatio
-     * @return string
-     * @throws \BookStack\Exceptions\ImageUploadException
-     * @throws \Exception
+     * @throws Exception
      */
-    protected function getThumbnail(Image $image, $width = 220, $height = 220, $keepRatio = false)
+    protected function getThumbnail(Image $image, ?int $width = 220, ?int $height = 220, bool $keepRatio = false): ?string
     {
         try {
             return $this->imageService->getThumbnail($image, $width, $height, $keepRatio);
-        } catch (\Exception $exception) {
+        } catch (Exception $exception) {
             return null;
         }
     }
 
     /**
      * Get the raw image data from an Image.
-     * @param Image $image
-     * @return null|string
      */
-    public function getImageData(Image $image)
+    public function getImageData(Image $image): ?string
     {
         try {
             return $this->imageService->getImageData($image);
-        } catch (\Exception $exception) {
+        } catch (Exception $exception) {
             return null;
         }
     }
 
     /**
      * Get the validation rules for image files.
-     * @return string
      */
-    public function getImageValidationRules()
+    public function getImageValidationRules(): string
     {
         return 'image_extension|no_double_extension|mimes:jpeg,png,gif,bmp,webp,tiff';
     }

diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php
index e7668471bd64c174c858eef0ac6c9c36a16e8880..756149fe7a1bacdc2f11a2f4ee3672ceb50c8676 100644 (file)
--- a/app/Uploads/ImageService.php
+++ b/app/Uploads/ImageService.php
@@ -254,7 +254,16 @@ class ImageService extends UploadService
         } else {
             $thumb->fit($width, $height);
         }
-        return (string)$thumb->encode();
+
+        $thumbData = (string)$thumb->encode();
+
+        // Use original image data if we're keeping the ratio
+        // and the resizing does not save any space.
+        if ($keepRatio && strlen($thumbData) > strlen($imageData)) {
+            return $imageData;
+        }
+
+        return $thumbData;
     }
 
     /**

diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php
index 0615a95ce52641cff442ce913d59bb65f812d2b3..3f6c021a7f0f86c74ad972c4166b0e64b485518a 100644 (file)
--- a/tests/Uploads/ImageTest.php
+++ b/tests/Uploads/ImageTest.php
@@ -36,6 +36,30 @@ class ImageTest extends TestCase
         ]);
     }
 
+    public function test_image_display_thumbnail_generation_does_not_increase_image_size()
+    {
+        $page = Page::first();
+        $admin = $this->getAdmin();
+        $this->actingAs($admin);
+
+        $originalFile = $this->getTestImageFilePath('compressed.png');
+        $originalFileSize = filesize($originalFile);
+        $imgDetails = $this->uploadGalleryImage($page, 'compressed.png');
+        $relPath = $imgDetails['path'];
+
+        $this->assertTrue(file_exists(public_path($relPath)), 'Uploaded image found at path: '. public_path($relPath));
+        $displayImage = $imgDetails['response']->thumbs->display;
+
+        $displayImageRelPath = implode('/', array_slice(explode('/', $displayImage), 3));
+        $displayImagePath = public_path($displayImageRelPath);
+        $displayFileSize = filesize($displayImagePath);
+
+        $this->deleteImage($relPath);
+        $this->deleteImage($displayImageRelPath);
+
+        $this->assertEquals($originalFileSize, $displayFileSize, 'Display thumbnail generation should not increase image size');
+    }
+
     public function test_image_edit()
     {
         $editor = $this->getEditor();

diff --git a/tests/Uploads/UsesImages.php b/tests/Uploads/UsesImages.php
index aa5ffe4c78250fb41f3a96c1319970a06eae3344..9ce559acdb167e595482632b92961bf676dfd39f 100644 (file)
--- a/tests/Uploads/UsesImages.php
+++ b/tests/Uploads/UsesImages.php
@@ -10,9 +10,13 @@ trait UsesImages
      * Get the path to our basic test image.
      * @return string
      */
-    protected function getTestImageFilePath()
+    protected function getTestImageFilePath(?string $fileName = null)
     {
-        return base_path('tests/test-data/test-image.png');
+        if (is_null($fileName)) {
+            $fileName = 'test-image.png';
+        }
+        
+        return base_path('tests/test-data/' . $fileName);
     }
 
     /**
@@ -20,9 +24,9 @@ trait UsesImages
      * @param $fileName
      * @return UploadedFile
      */
-    protected function getTestImage($fileName)
+    protected function getTestImage($fileName, ?string $testDataFileName = null)
     {
-        return new UploadedFile($this->getTestImageFilePath(), $fileName, 'image/png', 5238, null, true);
+        return new UploadedFile($this->getTestImageFilePath($testDataFileName), $fileName, 'image/png', 5238, null, true);
     }
 
     /**
@@ -52,9 +56,9 @@ trait UsesImages
      * @param string $contentType
      * @return \Illuminate\Foundation\Testing\TestResponse
      */
-    protected function uploadImage($name, $uploadedTo = 0, $contentType = 'image/png')
+    protected function uploadImage($name, $uploadedTo = 0, $contentType = 'image/png', ?string $testDataFileName = null)
     {
-        $file = $this->getTestImage($name);
+        $file = $this->getTestImage($name, $testDataFileName);
         return $this->withHeader('Content-Type', $contentType)
             ->call('POST', '/images/gallery', ['uploaded_to' => $uploadedTo], [], ['file' => $file], []);
     }
@@ -66,22 +70,23 @@ trait UsesImages
      * @param Page|null $page
      * @return array
      */
-    protected function uploadGalleryImage(Page $page = null)
+    protected function uploadGalleryImage(Page $page = null, ?string $testDataFileName = null)
     {
         if ($page === null) {
             $page = Page::query()->first();
         }
 
-        $imageName = 'first-image.png';
+        $imageName = $testDataFileName ?? 'first-image.png';
         $relPath = $this->getTestImagePath('gallery', $imageName);
         $this->deleteImage($relPath);
 
-        $upload = $this->uploadImage($imageName, $page->id);
+        $upload = $this->uploadImage($imageName, $page->id, 'image/png', $testDataFileName);
         $upload->assertStatus(200);
         return [
             'name' => $imageName,
             'path' => $relPath,
-            'page' => $page
+            'page' => $page,
+            'response' => json_decode($upload->getContent()),
         ];
     }
 

diff --git a/tests/test-data/compressed.png b/tests/test-data/compressed.png
new file mode 100644 (file)
index 0000000..e42ef58
Binary files /dev/null and b/tests/test-data/compressed.png differ