Skip to content

Commit

Permalink
[BUGFIX] Process cropped images only once
Browse files Browse the repository at this point in the history 
To avoid loss of quality and spawning unnecessary imagemagick
processes, cropping and scaling of images is now done
with a single imagemagick process.

By doing so, the code for SVG processing is streamlined.
SVG processing code can further be improved later,
by putting it into a dedicated file processing task processor.

Releases: master, 10.4
Resolves: #91855
Change-Id: I3bf735e74dd46dec73431405f37616506747ccdf
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/65187
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Markus Klein <markus.klein@typo3.org>
Tested-by: Benni Mack <benni@typo3.org>
Reviewed-by: Markus Klein <markus.klein@typo3.org>
Reviewed-by: Benni Mack <benni@typo3.org>
  • Loading branch information
@helhum @bmack
helhum authored and bmack committed Nov 17, 2020
1 parent c577de8 commit 844f024
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 86 deletions.
65 changes: 40 additions & 25 deletions typo3/sysext/core/Classes/Imaging/GraphicalFunctions.php
View file
Expand Up @@ -18,6 +18,7 @@
use TYPO3\CMS\Core\Cache\CacheManager;
use TYPO3\CMS\Core\Charset\CharsetConverter;
use TYPO3\CMS\Core\Core\Environment;
use TYPO3\CMS\Core\Imaging\ImageManipulation\Area;
use TYPO3\CMS\Core\Type\File\ImageInfo;
use TYPO3\CMS\Core\Utility\ArrayUtility;
use TYPO3\CMS\Core\Utility\CommandUtility;
Expand Down Expand Up @@ -339,8 +340,6 @@ public function __construct()
// ... but if 'processor_effects' is set, enable effects
if ($gfxConf['processor_effects']) {
$this->processorEffectsEnabled = true;
$this->cmds['jpg'] .= $this->v5_sharpen(10);
$this->cmds['jpeg'] .= $this->v5_sharpen(10);
}
// Secures that images are not scaled up.
$this->mayScaleUp = (bool)$gfxConf['processor_allowUpscaling'];
Expand Down Expand Up @@ -2097,9 +2096,6 @@ public function imageMagickConvert($imagefile, $newExt = '', $w = '', $h = '', $
$newExt = $info[2];
} else {
$newExt = $this->gif_or_jpg($info[2], $info[0], $info[1]);
if (!$params) {
$params = $this->cmds[$newExt];
}
}
}
if (!in_array($newExt, $this->imageFileExt, true)) {
Expand All @@ -2115,7 +2111,7 @@ public function imageMagickConvert($imagefile, $newExt = '', $w = '', $h = '', $
// given or if the destination w/h matches the original image
// dimensions or if the option to not scale the image is set)
$noScale = !$w && !$h || $data[0] == $info[0] && $data[1] == $info[1] || !empty($options['noScale']);
if ($noScale && !$data['crs'] && !$params && !$frame && $newExt == $info[2] && !$mustCreate) {
if ($noScale && !$data['crs'] && !$params && !$frame && $newExt === $info[2] && !$mustCreate) {
// Set the new width and height before returning,
// if the noScale option is set
if (!empty($options['noScale'])) {
Expand All @@ -2125,13 +2121,33 @@ public function imageMagickConvert($imagefile, $newExt = '', $w = '', $h = '', $
$info[3] = $imagefile;
return $info;
}
$info[0] = $data[0];
$info[1] = $data[1];
$frame = $this->addFrameSelection ? (int)$frame : 0;
if (!$params) {
$params = $this->cmds[$newExt];
$params = ($this->cmds[$newExt] ?? '') . ($params ? ' ' . $params : '');
$command = '';
// Cropping
if (($options['crop'] ?? null) instanceof Area) {
/** @var Area $cropArea */
$cropArea = $options['crop'];
$command .= sprintf(
$this->scalecmd . ' %dx%d! -crop %dx%d+%d+%d +repage ' . $params . ' ',
$info[0],
$info[1],
$cropArea->getWidth(),
$cropArea->getHeight(),
$cropArea->getOffsetLeft(),
$cropArea->getOffsetTop()
);
}
// Scaling when required
if ($info[0] !== $data[0] || $info[1] !== $data[1]) {
$effectsParams = ' ';
if ($this->processorEffectsEnabled && in_array($newExt, ['jpg', 'jpeg'], true)) {
$effectsParams .= $this->v5_sharpen(10) . ' ';
}
// Don't scale if original size equals target size
$command .= $this->scalecmd . ' ' . $data[0] . 'x' . $data[1] . '! ' . $params . $effectsParams;
}
// Cropscaling:
// Crop scaling:
if ($data['crs']) {
if (!$data['origW']) {
$data['origW'] = $data[0];
Expand All @@ -2141,11 +2157,8 @@ public function imageMagickConvert($imagefile, $newExt = '', $w = '', $h = '', $
}
$offsetX = (int)(($data[0] - $data['origW']) * ($data['cropH'] + 100) / 200);
$offsetY = (int)(($data[1] - $data['origH']) * ($data['cropV'] + 100) / 200);
$params .= ' -crop ' . $data['origW'] . 'x' . $data['origH'] . '+' . $offsetX . '+' . $offsetY . '! +repage';
$command .= ' -crop ' . $data['origW'] . 'x' . $data['origH'] . '+' . $offsetX . '+' . $offsetY . '! +repage ' . $params . ' ';
}
$command = $this->scalecmd . ' ' . $info[0] . 'x' . $info[1] . '! ' . $params . ' ';
// re-apply colorspace-setting for the resulting image so colors don't appear to dark (sRGB instead of RGB)
$command .= ' -colorspace ' . $this->colorspace;
$cropscale = $data['crs'] ? 'crs-V' . $data['cropV'] . 'H' . $data['cropH'] : '';
if ($this->alternativeOutputKey) {
$theOutputName = GeneralUtility::shortMD5($command . $cropscale . PathUtility::basename($imagefile) . $this->alternativeOutputKey . '[' . $frame . ']');
Expand All @@ -2163,13 +2176,15 @@ public function imageMagickConvert($imagefile, $newExt = '', $w = '', $h = '', $
$this->imageMagickExec($imagefile, $output, $command, $frame);
}
if (file_exists($output)) {
$info[3] = $output;
$info[0] = $data[0];
$info[1] = $data[1];
$info[2] = $newExt;
$info[3] = $output;
// params might change some image data!
if ($params) {
$info = $this->getImageDimensions($info[3]);
}
if ($info[2] == $this->gifExtension && !$this->dontCompress) {
if (!$this->dontCompress && $info[2] === $this->gifExtension) {
// Compress with IM (lzw) or GD (rle) (Workaround for the absence of lzw-compression in GD)
self::gifCompress($info[3], '');
}
Expand Down Expand Up @@ -2370,28 +2385,28 @@ public function getImageScale($info, $w, $h, $options)
// If scaling should be performed. Check that input "info" array will not cause division-by-zero
if (($w || $h) && $info[0] && $info[1]) {
if ($w && !$h) {
$info[1] = ceil($info[1] * ($w / $info[0]));
$info[1] = (int)ceil($info[1] * ($w / $info[0]));
$info[0] = $w;
}
if (!$w && $h) {
$info[0] = ceil($info[0] * ($h / $info[1]));
$info[0] = (int)ceil($info[0] * ($h / $info[1]));
$info[1] = $h;
}
if ($w && $h) {
if ($max) {
$ratio = $info[0] / $info[1];
if ($h * $ratio > $w) {
$h = round($w / $ratio);
$h = (int)round($w / $ratio);
} else {
$w = round($h * $ratio);
$w = (int)round($h * $ratio);
}
}
if ($crs) {
$ratio = $info[0] / $info[1];
if ($h * $ratio < $w) {
$h = round($w / $ratio);
$h = (int)round($w / $ratio);
} else {
$w = round($h * $ratio);
$w = (int)round($h * $ratio);
}
}
$info[0] = $w;
Expand All @@ -2403,13 +2418,13 @@ public function getImageScale($info, $w, $h, $options)
// Set minimum-measures!
if (isset($options['minW']) && $out[0] < $options['minW']) {
if (($max || $crs) && $out[0]) {
$out[1] = round($out[1] * $options['minW'] / $out[0]);
$out[1] = (int)round($out[1] * $options['minW'] / $out[0]);
}
$out[0] = $options['minW'];
}
if (isset($options['minH']) && $out[1] < $options['minH']) {
if (($max || $crs) && $out[1]) {
$out[0] = round($out[0] * $options['minH'] / $out[1]);
$out[0] = (int)round($out[0] * $options['minH'] / $out[1]);
}
$out[1] = $options['minH'];
}
Expand Down
100 changes: 39 additions & 61 deletions typo3/sysext/core/Classes/Resource/Processing/LocalCropScaleMaskHelper.php
View file
Expand Up @@ -16,11 +16,11 @@
namespace TYPO3\CMS\Core\Resource\Processing;

use TYPO3\CMS\Core\Core\Environment;
use TYPO3\CMS\Core\Imaging\ImageManipulation\Area;
use TYPO3\CMS\Core\Resource;
use TYPO3\CMS\Core\Resource\FileInterface;
use TYPO3\CMS\Core\Resource\ProcessedFile;
use TYPO3\CMS\Core\Utility\GeneralUtility;
use TYPO3\CMS\Core\Utility\MathUtility;
use TYPO3\CMS\Frontend\Imaging\GifBuilder;

/**
Expand Down Expand Up @@ -62,6 +62,10 @@ public function process(TaskInterface $task)
*/
public function processWithLocalFile(TaskInterface $task, string $originalFileName): ?array
{
if (empty($GLOBALS['TYPO3_CONF_VARS']['GFX']['processor_enabled'])) {
return null;
}

$result = null;
$targetFile = $task->getTargetFile();

Expand All @@ -75,53 +79,16 @@ public function processWithLocalFile(TaskInterface $task, string $originalFileNa
}

$options = $this->getConfigurationForImageCropScaleMask($targetFile, $gifBuilder);

$croppedImage = null;
if (!empty($configuration['crop'])) {

// check if it is a json object
$cropData = json_decode($configuration['crop']);
if ($cropData) {
$crop = implode(',', [(int)$cropData->x, (int)$cropData->y, (int)$cropData->width, (int)$cropData->height]);
} else {
$crop = $configuration['crop'];
}

[$offsetLeft, $offsetTop, $newWidth, $newHeight] = explode(',', $crop, 4);

$backupPrefix = $gifBuilder->filenamePrefix;
$gifBuilder->filenamePrefix = 'crop_';

$jpegQuality = MathUtility::forceIntegerInRange($GLOBALS['TYPO3_CONF_VARS']['GFX']['jpg_quality'], 10, 100, 85);

// the result info is an array with 0=width,1=height,2=extension,3=filename
$result = $gifBuilder->imageMagickConvert(
$originalFileName,
$configuration['fileExtension'],
'',
'',
sprintf('-crop %dx%d+%d+%d +repage -quality %d', $newWidth, $newHeight, $offsetLeft, $offsetTop, $jpegQuality),
'',
['noScale' => true],
true
);
$gifBuilder->filenamePrefix = $backupPrefix;

if ($result !== null) {
$originalFileName = $croppedImage = $result[3];
}
}

// Normal situation (no masking)
if (!(is_array($configuration['maskImages']) && $GLOBALS['TYPO3_CONF_VARS']['GFX']['processor_enabled'])) {
if (empty($configuration['maskImages'])) {
// the result info is an array with 0=width,1=height,2=extension,3=filename
$result = $gifBuilder->imageMagickConvert(
$originalFileName,
$configuration['fileExtension'],
$configuration['width'],
$configuration['height'],
$configuration['additionalParameters'],
$configuration['frame'],
$configuration['fileExtension'] ?? null,
$configuration['width'] ?? null,
$configuration['height'] ?? null,
$configuration['additionalParameters'] ?? null,
$configuration['frame'] ?? null,
$options
);
} else {
Expand All @@ -138,16 +105,16 @@ public function processWithLocalFile(TaskInterface $task, string $originalFileNa
$tempFileInfo = $gifBuilder->imageMagickConvert(
$originalFileName,
$temporaryExtension,
$configuration['width'],
$configuration['height'],
$configuration['additionalParameters'],
$configuration['frame'],
$configuration['width'] ?? null,
$configuration['height'] ?? null,
$configuration['additionalParameters'] ?? null,
$configuration['frame'] ?? null,
$options
);
if (is_array($tempFileInfo)) {
$maskBottomImage = $configuration['maskImages']['maskBottomImage'];
$maskBottomImage = $configuration['maskImages']['maskBottomImage'] ?? null;
if ($maskBottomImage instanceof FileInterface) {
$maskBottomImageMask = $configuration['maskImages']['maskBottomImageMask'];
$maskBottomImageMask = $configuration['maskImages']['maskBottomImageMask'] ?? null;
} else {
$maskBottomImageMask = null;
}
Expand Down Expand Up @@ -189,7 +156,7 @@ public function processWithLocalFile(TaskInterface $task, string $originalFileNa

// check if the processing really generated a new file (scaled and/or cropped)
if ($result !== null) {
if ($result[3] !== $originalFileName || $originalFileName === $croppedImage) {
if ($result[3] !== $originalFileName) {
$result = [
'width' => $result[0],
'height' => $result[1],
Expand All @@ -201,11 +168,6 @@ public function processWithLocalFile(TaskInterface $task, string $originalFileNa
}
}

// Cleanup temp file if it isn't used as result
if ($croppedImage && ($result === null || $croppedImage !== $result['filePath'])) {
GeneralUtility::unlink_tempfile($croppedImage);
}

return $result;
}

Expand All @@ -219,22 +181,38 @@ protected function getConfigurationForImageCropScaleMask(ProcessedFile $processe
{
$configuration = $processedFile->getProcessingConfiguration();

if ($configuration['useSample']) {
if ($configuration['useSample'] ?? null) {
$gifBuilder->scalecmd = '-sample';
}
$options = [];
if ($configuration['maxWidth']) {
if ($configuration['maxWidth'] ?? null) {
$options['maxW'] = $configuration['maxWidth'];
}
if ($configuration['maxHeight']) {
if ($configuration['maxHeight'] ?? null) {
$options['maxH'] = $configuration['maxHeight'];
}
if ($configuration['minWidth']) {
if ($configuration['minWidth'] ?? null) {
$options['minW'] = $configuration['minWidth'];
}
if ($configuration['minHeight']) {
if ($configuration['minHeight'] ?? null) {
$options['minH'] = $configuration['minHeight'];
}
if ($configuration['crop'] ?? null) {
$options['crop'] = $configuration['crop'];
// This normalisation is required to preserve backwards compatibility
// In the future the whole processing configuration should be a plain array or json serializable object for straightforward serialisation
// The crop configuration currently can be a json string, string with comma separated values or an Area object
if (is_string($configuration['crop'])) {
// check if it is a json object
$cropData = json_decode($configuration['crop']);
if ($cropData) {
$options['crop'] = new Area($cropData->x, $cropData->y, $cropData->width, $cropData->height);
} else {
[$offsetLeft, $offsetTop, $newWidth, $newHeight] = explode(',', $configuration['crop'], 4);
$options['crop'] = new Area($offsetLeft, $offsetTop, $newWidth, $newHeight);
}
}
}

$options['noScale'] = $configuration['noScale'];

Expand Down

0 comments on commit 844f024

Please sign in to comment.