From: Dan Brown Date: Sun, 22 Dec 2019 12:44:49 +0000 (+0000) Subject: Made display thumbnail generation use original data if smaller X-Git-Tag: v0.28.0~1^2~28 X-Git-Url: http://source.bookstackapp.com/bookstack/commitdiff_plain/32e7f0a2e672226384f58a0f7ba96e0b17535d4a 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 da0b7d379..01b65f882 100644 --- 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 e7668471b..756149fe7 100644 --- 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 0615a95ce..3f6c021a7 100644 --- 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 aa5ffe4c7..9ce559acd 100644 --- 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 index 000000000..e42ef58f0 Binary files /dev/null and b/tests/test-data/compressed.png differ