From 7e1152603f1a083566241527ef2a615104c91395 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 1 Feb 2023 13:23:35 +0100 Subject: feat(theming): Only convert a background image if it benefits from it * WebP images are generally quite small, converting to pngs would increase the filesize a lot. * Small JPEG and PNG images do not benefit from any conversion, so skip it. * JPEG images will get quite bigger when converted to PNG so instead convert to progressive JPEG Signed-off-by: Ferdinand Thiessen --- apps/theming/lib/ImageManager.php | 33 +++++++++++++++++++++++++++++++-- apps/theming/tests/ImageManagerTest.php | 9 ++++++--- 2 files changed, 37 insertions(+), 5 deletions(-) (limited to 'apps/theming') diff --git a/apps/theming/lib/ImageManager.php b/apps/theming/lib/ImageManager.php index f7b0c12844a..fe9c3332f7e 100644 --- a/apps/theming/lib/ImageManager.php +++ b/apps/theming/lib/ImageManager.php @@ -243,7 +243,7 @@ class ImageManager { throw new \Exception('Unsupported image type'); } - if ($key === 'background' && strpos($detectedMimeType, 'image/svg') === false && strpos($detectedMimeType, 'image/gif') === false) { + if ($key === 'background' && $this->shouldOptimizeBackgroundImage($detectedMimeType, filesize($tmpFile))) { // Optimize the image since some people may upload images that will be // either to big or are not progressive rendering. $newImage = @imagecreatefromstring(file_get_contents($tmpFile)); @@ -258,7 +258,11 @@ class ImageManager { $outputImage = imagescale($newImage, $newWidth, $newHeight); imageinterlace($outputImage, 1); - imagepng($outputImage, $tmpFile, 8); + if (strpos($detectedMimeType, 'image/jpeg') !== false) { + imagejpeg($outputImage, $tmpFile, 90); + } else { + imagepng($outputImage, $tmpFile, 8); + } imagedestroy($outputImage); $target->putContent(file_get_contents($tmpFile)); @@ -269,6 +273,31 @@ class ImageManager { return $detectedMimeType; } + /** + * Decide whether an image benefits from shrinking and reconverting + * + * @param string $mimeType the mime type of the image + * @param int $contentSize size of the image file + * @return bool + */ + private function shouldOptimizeBackgroundImage(string $mimeType, int $contentSize): bool { + // Do not touch SVGs + if (strpos($mimeType, 'image/svg') !== false) { + return false; + } + // GIF does not benefit from converting + if (strpos($mimeType, 'image/gif') !== false) { + return false; + } + // WebP also does not benefit from converting + // We could possibly try to convert to progressive image, but normally webP images are quite small + if (strpos($mimeType, 'image/webp') !== false) { + return false; + } + // As a rule of thumb background images should be max. 150-300 KiB, small images do not benefit from converting + return $contentSize > 150000; + } + /** * Returns a list of supported mime types for image uploads. * "favicon" images are only allowed to be SVG when imagemagick with SVG support is available. diff --git a/apps/theming/tests/ImageManagerTest.php b/apps/theming/tests/ImageManagerTest.php index ffb023c970f..22432a00103 100644 --- a/apps/theming/tests/ImageManagerTest.php +++ b/apps/theming/tests/ImageManagerTest.php @@ -337,9 +337,12 @@ class ImageManagerTest extends TestCase { public function dataUpdateImage() { return [ - ['background', __DIR__ . '/../../../tests/data/testimage.png', true, true], - ['background', __DIR__ . '/../../../tests/data/testimage.png', false, true], - ['background', __DIR__ . '/../../../tests/data/testimage.jpg', true, true], + ['background', __DIR__ . '/../../../tests/data/testimage.png', true, false], + ['background', __DIR__ . '/../../../tests/data/testimage.png', false, false], + ['background', __DIR__ . '/../../../tests/data/testimage.jpg', true, false], + ['background', __DIR__ . '/../../../tests/data/testimage.webp', true, false], + ['background', __DIR__ . '/../../../tests/data/testimage-large.jpg', true, true], + ['background', __DIR__ . '/../../../tests/data/testimage-wide.png', true, true], ['logo', __DIR__ . '/../../../tests/data/testimagelarge.svg', true, false], ]; } -- cgit v1.2.3 From 2ca1153ff6667d7d7d98189aec95f246854ffcfa Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Mon, 20 Feb 2023 13:10:49 +0100 Subject: fix(themeing): Add error handling to ImageManager Signed-off-by: Ferdinand Thiessen --- apps/theming/lib/ImageManager.php | 64 +++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 23 deletions(-) (limited to 'apps/theming') diff --git a/apps/theming/lib/ImageManager.php b/apps/theming/lib/ImageManager.php index fe9c3332f7e..d088699fd3c 100644 --- a/apps/theming/lib/ImageManager.php +++ b/apps/theming/lib/ImageManager.php @@ -244,32 +244,50 @@ class ImageManager { } if ($key === 'background' && $this->shouldOptimizeBackgroundImage($detectedMimeType, filesize($tmpFile))) { - // Optimize the image since some people may upload images that will be - // either to big or are not progressive rendering. - $newImage = @imagecreatefromstring(file_get_contents($tmpFile)); - - // Preserve transparency - imagesavealpha($newImage, true); - imagealphablending($newImage, true); - - $tmpFile = $this->tempManager->getTemporaryFile(); - $newWidth = (int)(imagesx($newImage) < 4096 ? imagesx($newImage) : 4096); - $newHeight = (int)(imagesy($newImage) / (imagesx($newImage) / $newWidth)); - $outputImage = imagescale($newImage, $newWidth, $newHeight); - - imageinterlace($outputImage, 1); - if (strpos($detectedMimeType, 'image/jpeg') !== false) { - imagejpeg($outputImage, $tmpFile, 90); - } else { - imagepng($outputImage, $tmpFile, 8); - } - imagedestroy($outputImage); + try { + // Optimize the image since some people may upload images that will be + // either to big or are not progressive rendering. + $newImage = @imagecreatefromstring(file_get_contents($tmpFile)); + if ($newImage === false) { + throw new \Exception('Could not read background image, possibly corrupted.'); + } - $target->putContent(file_get_contents($tmpFile)); - } else { - $target->putContent(file_get_contents($tmpFile)); + // Preserve transparency + imagesavealpha($newImage, true); + imagealphablending($newImage, true); + + $newWidth = (int)(imagesx($newImage) < 4096 ? imagesx($newImage) : 4096); + $newHeight = (int)(imagesy($newImage) / (imagesx($newImage) / $newWidth)); + $outputImage = imagescale($newImage, $newWidth, $newHeight); + if ($outputImage === false) { + throw new \Exception('Could not scale uploaded background image.'); + } + + $newTmpFile = $this->tempManager->getTemporaryFile(); + imageinterlace($outputImage, 1); + // Keep jpeg images encoded as jpeg + if (strpos($detectedMimeType, 'image/jpeg') !== false) { + if (!imagejpeg($outputImage, $newTmpFile, 90)) { + throw new \Exception('Could not recompress background image as JPEG'); + } + } else { + if (!imagepng($outputImage, $newTmpFile, 8)) { + throw new \Exception('Could not recompress background image as PNG'); + } + } + $tmpFile = $newTmpFile; + imagedestroy($outputImage); + } catch (\Exception $e) { + if (is_resource($outputImage) || $outputImage instanceof \GdImage) { + imagedestroy($outputImage); + } + + $this->logger->debug($e->getMessage()); + } } + $target->putContent(file_get_contents($tmpFile)); + return $detectedMimeType; } -- cgit v1.2.3