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

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';
     }

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;
     }
 
     /**

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();

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()),
         ];
     }