New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image transformation chain reimplementation #417

Merged
merged 43 commits into from May 30, 2017

Conversation

Projects
None yet
5 participants
@rexxars
Member

rexxars commented Dec 6, 2015

TL;DR: I found a way to optimize resizing of JPEG-images significantly. Many changes had to be done in order to get this to work.

Background

I was benchmarking Imagick against a different image backend and turned to Imbo to see how we had implemented the jpeg:size-flag, and was surprised to find that it didn't actually give us any speed increase. After a few minutes, I quickly realized that this is caused by us not setting the flag prior to loading the image blob into Imagick.

Obviously I wanted to fix this in Imbo, but it turned out to be quite a lot of work:

  • I needed a way to get the size of the smallest input image the transformation chain could accept. This is already done in the image variations listener, but I never liked how it has transformation names hardcoded into it, making it impossible for third-party resize transformations to hook onto this.
  • There was no simple way to get the class associated with a given transformation name. You had to simply trigger an event and see if anything picked it up. To make things worse, if multiple transformations of the same name was in the same chain, it was impossible to see which one returned the smallest size.
  • Some transformations require the full image, and some transformations alter the image in a way that makes it hard or impossible for subsequent transformations to return the correct minimum input size. For instance, rotating an image 37 degrees and applying a maxSize-transformation after this is not resizing just the actual image, but instead a rather weird canvas.

Changes

To try and summarize the changes done in this branch:

  • Imbo now has a TransformationManager, which keeps track of all the available transformations. Transformations are no longer configured in the eventListeners configuration section, instead they are now in a separate transformations section.
  • The manager is what triggers the transformations, and should in theory be able to pass on more information to each transformation if we want it to. For instance, some transformations might want to know if the image has been rotated or resized in order to correctly calculate where to draw objects (drawPois being one of them)
  • Two new interfaces have been added that each transformation can implement:
    • InputSizeConstraint - will signal to the manager that this transformation needs a certain input size. Typically this is for transformations that do resizing - maxSize, thumbnail and resize all implement it. It can also be used to indicate that it needs the full size image, that the result of the transformation is unpredictable and should stop the minimum size resolving and can also tell Imbo if any rotation was performed. It also contains a method (adjustParameters()) that transformations can implement to adjust any parameters that they use, in case the input size was changed. Currently this is used for canvas, border, watermark and crop, which all use coordinates or width/height parameters.
    • RegionExtractor - will signal to the manager that the transformation extracts a portion of the image. This is used in combination with the minimum input size resolving in order to find the smallest possible size. crop currently implements this.
  • Imagick asks the transformation manager what the minimum input size is and sets the jpeg:size hint before loading the image blob. This allows libjpeg to use a "shrink-on-load" implementation, which significantly increases the speed of the resizing.
  • The image variations listener uses the transformation manager in order to resolve minimum variation size and to adjust transformation parameters. This decouples it from core and enables third-party transformations to benefit from it.

Result

To benchmark the old implementation against the new, I wrote a script that:

  • Queries for 300 images (all 2000+ pixels in width)
  • Loops through each image and generates an image url with a maxSize transformation set to 523x315
    • Image variations are generated for all the images, closest size to 523px being 1000px
  • Downloads the image URLs
  • Iterates over the same image set 3 times to remove any outliers

This is all done against a server running locally, so network conditions shouldn't have any say. I've reproduced the results multiple times with just small deviations. The result of resizing 900 images (300 * 3 iterations):

Implementation Variations enabled Speed
Old No 123.01s
New No 45.57s
Old Yes 22.30s
New Yes 16.73s

Conclusions

I think the results speak for themselves. Most importantly, this is a "free" optimization - it doesn't require any more disk space or configuration to set up. Obviously, this only applies to JPEG-images that is able to use the "shrink-on-load" trick, but I'm guessing most peoples image collection consists primarily of JPEGs anyway.

Further work

First and foremost, the smartSize transformation has not been optimized. I think @kbrabrand should do this, being the author of it, and it might be that the transformation manager will require some small changes for it to work with this transformation.

We should also give this some more testing in a production-like environment. Many combined transformations and order of these might provoke bugs, so we should also try quite a bit of transformation combinations before merging.

@kbrabrand

This comment has been minimized.

Show comment
Hide comment
@kbrabrand

kbrabrand Dec 7, 2015

Member

This is awesome! 👏 I've added a comment with a link to a gist in #393. Will try to find time to look at getting this optimization to work with smartSize soon.

Member

kbrabrand commented Dec 7, 2015

This is awesome! 👏 I've added a comment with a link to a gist in #393. Will try to find time to look at getting this optimization to work with smartSize soon.

@christeredvartsen

This comment has been minimized.

Show comment
Hide comment
@christeredvartsen

christeredvartsen Dec 7, 2015

Member

Really nice work @rexxars! The way the transformations are handled here enables more interaction between the transformations which is a nice addition.

Member

christeredvartsen commented Dec 7, 2015

Really nice work @rexxars! The way the transformations are handled here enables more interaction between the transformations which is a nice addition.

@rexxars rexxars added the Improvement label Feb 26, 2016

@rexxars rexxars added this to the Imbo 2.1 milestone Feb 28, 2016

@christeredvartsen christeredvartsen modified the milestones: Imbo 2.2, Imbo 2.1 Mar 11, 2016

@christeredvartsen

This comment has been minimized.

Show comment
Hide comment
@christeredvartsen

christeredvartsen Mar 16, 2016

Member

@rexxars Care to do a rebase against develop?

@kbrabrand Could you see if you might find some time to implement support for the smartSize transformation as well?

Member

christeredvartsen commented Mar 16, 2016

@rexxars Care to do a rebase against develop?

@kbrabrand Could you see if you might find some time to implement support for the smartSize transformation as well?

@rexxars

This comment has been minimized.

Show comment
Hide comment
@rexxars

rexxars Mar 17, 2016

Member

Rebased.

Member

rexxars commented Mar 17, 2016

Rebased.

@kbrabrand

This comment has been minimized.

Show comment
Hide comment
@kbrabrand

kbrabrand Mar 17, 2016

Member

I'll try, but it's been really busy lately with courses at university and the new job :/

Member

kbrabrand commented Mar 17, 2016

I'll try, but it's been really busy lately with courses at university and the new job :/

@christeredvartsen christeredvartsen self-assigned this May 4, 2017

@christeredvartsen

This PR probably needs some more eyes. Up for the task @matslindh?

I general I'm all for merging this, and at first glance it looks good to go. Nicely done @rexxars!

*
* @var boolean
*/
'jpegSizeHint' => true,

This comment has been minimized.

@christeredvartsen

christeredvartsen May 7, 2017

Member

@matslindh How do you feel about this being enabled by default? I'm somewhat worried about the "one pixel off"-tradeoff. Not sure if that will ever be an issue for anyone though...

@christeredvartsen

christeredvartsen May 7, 2017

Member

@matslindh How do you feel about this being enabled by default? I'm somewhat worried about the "one pixel off"-tradeoff. Not sure if that will ever be an issue for anyone though...

This comment has been minimized.

@matslindh

matslindh May 28, 2017

Contributor

I feel that true by default is A-OK. This won't be an issue for most users, and the constraint on image delivery (meaning that the delivered image isn't suddenly one pixel larger or smaller than what the request asks for) is the most important size limitation.

@matslindh

matslindh May 28, 2017

Contributor

I feel that true by default is A-OK. This won't be an issue for most users, and the constraint on image delivery (meaning that the delivered image isn't suddenly one pixel larger or smaller than what the request asks for) is the most important size limitation.

@@ -346,13 +223,12 @@ public function generateVariations(EventInterface $event) {
$variationWidth = round($variationWidth * $scaleFactor);
if ($variationWidth > $maxWidth) {
// Width too big, try again (twss)

This comment has been minimized.

@christeredvartsen
@christeredvartsen
Show outdated Hide outdated src/Image/Transformation/AutoRotate.php
Show outdated Hide outdated src/Image/Transformation/MaxSize.php
protected function getImagickVersion() {
// Newer versions of Imagick expose getVersion as a static method,
// and won't allow phpunit to mock it even when called on an instance
if (method_exists('Imagick', 'getVersion')) {

This comment has been minimized.

@christeredvartsen

christeredvartsen May 7, 2017

Member

Somewhat icky. Perhaps it's time to go over the tests and fix them instead of doing things like this?

@christeredvartsen

christeredvartsen May 7, 2017

Member

Somewhat icky. Perhaps it's time to go over the tests and fix them instead of doing things like this?

This comment has been minimized.

@matslindh

matslindh May 28, 2017

Contributor

👍 But let's create a separate issue for that and allow this if it works for now. We have a least one other code path that has to check the ImageMagick version.

@matslindh

matslindh May 28, 2017

Contributor

👍 But let's create a separate issue for that and allow this if it works for now. We have a least one other code path that has to check the ImageMagick version.

Show outdated Hide outdated src/Image/TransformationManager.php
Show outdated Hide outdated src/Image/TransformationManager.php
@matslindh

Almost 💯 files changed. Cursory review.

*
* @var boolean
*/
'jpegSizeHint' => true,

This comment has been minimized.

@matslindh

matslindh May 28, 2017

Contributor

I feel that true by default is A-OK. This won't be an issue for most users, and the constraint on image delivery (meaning that the delivered image isn't suddenly one pixel larger or smaller than what the request asks for) is the most important size limitation.

@matslindh

matslindh May 28, 2017

Contributor

I feel that true by default is A-OK. This won't be an issue for most users, and the constraint on image delivery (meaning that the delivered image isn't suddenly one pixel larger or smaller than what the request asks for) is the most important size limitation.

Show outdated Hide outdated src/Application.php
}
try {
$this->imagick->thumbnailImage($size['width'], $size['height']);

This comment has been minimized.

@matslindh

matslindh May 28, 2017

Contributor

Overextending try/catch

@matslindh

matslindh May 28, 2017

Contributor

Overextending try/catch

This comment has been minimized.

@matslindh

matslindh May 28, 2017

Contributor

These happen in multiple locations in the transformations - we should probably use everything not relevant for the exception out of the try/catch, but it'd require changes in multiple locations.

@matslindh

matslindh May 28, 2017

Contributor

These happen in multiple locations in the transformations - we should probably use everything not relevant for the exception out of the try/catch, but it'd require changes in multiple locations.

// If the angle of the rotation is dividable by 90, we can calculate the input
// size for the transformation that follow. Otherwise, this will be hard, so we
// return false to signal that we can't make any assumptions from this point on
if ($params['angle'] % 90 === 0) {

This comment has been minimized.

@matslindh

matslindh May 28, 2017

Contributor

Rotating by a value not divisble by 90 is probably not used much in the wild anyway, but we should note this as a future enhancement (it wouldn't be too hard to calculate the bounding box of a rotation, but it's not worth delaying the patch further for it.

@matslindh

matslindh May 28, 2017

Contributor

Rotating by a value not divisble by 90 is probably not used much in the wild anyway, but we should note this as a future enhancement (it wouldn't be too hard to calculate the bounding box of a rotation, but it's not worth delaying the patch further for it.

protected function getImagickVersion() {
// Newer versions of Imagick expose getVersion as a static method,
// and won't allow phpunit to mock it even when called on an instance
if (method_exists('Imagick', 'getVersion')) {

This comment has been minimized.

@matslindh

matslindh May 28, 2017

Contributor

👍 But let's create a separate issue for that and allow this if it works for now. We have a least one other code path that has to check the ImageMagick version.

@matslindh

matslindh May 28, 2017

Contributor

👍 But let's create a separate issue for that and allow this if it works for now. We have a least one other code path that has to check the ImageMagick version.

->with('convert')
->will($this->returnValue($convert));
$this->event->expects($this->once())->method('getTransformationManager')->will($this->returnValue($transformationManager));

This comment has been minimized.

@matslindh

matslindh May 28, 2017

Contributor

Multiple uses of ->once() in block

@matslindh

matslindh May 28, 2017

Contributor

Multiple uses of ->once() in block

Imagick::ORIENTATION_TOPRIGHT
));
$imagick->expects($this->once())->method('flopImage');
$imagick->expects($this->once())->method('setImageOrientation')->with(Imagick::ORIENTATION_TOPLEFT);

This comment has been minimized.

@matslindh

matslindh May 28, 2017

Contributor

At least a few of these should be changed to ->any() (the flopImage check might be allowed a once(), the other two should be any().

@matslindh

matslindh May 28, 2017

Contributor

At least a few of these should be changed to ->any() (the flopImage check might be allowed a once(), the other two should be any().

$imagick->expects($this->once())->method('readImageBlob')->with('foo');
$transformation = new Strip();
$transformation->setImagick($imagick)->setImage($image)->transform([]);

This comment has been minimized.

@matslindh

matslindh May 28, 2017

Contributor

Multiple uses of once()

@matslindh

matslindh May 28, 2017

Contributor

Multiple uses of once()

$event->expects($this->at(0))->method('getArgument')->with('image')->will($this->returnValue($image));
$event->expects($this->at(1))->method('getArgument')->with('params')->will($this->returnValue($params));
$event->expects($this->at(2))->method('getResponse')->will($this->returnValue($response));
$event->expects($this->once())->method('getResponse')->will($this->returnValue($response));

This comment has been minimized.

@matslindh

matslindh May 28, 2017

Contributor

Multiple uses of once

@matslindh

matslindh May 28, 2017

Contributor

Multiple uses of once

$event->expects($this->at(2))->method('getStorage')->will($this->returnValue($storage));
$event->expects($this->at(3))->method('getRequest')->will($this->returnValue($request));
$event->expects($this->once())->method('getStorage')->will($this->returnValue($storage));
$event->expects($this->once())->method('getRequest')->will($this->returnValue($request));

This comment has been minimized.

@matslindh

matslindh May 28, 2017

Contributor

Multiple uses of once()

@matslindh

matslindh May 28, 2017

Contributor

Multiple uses of once()

rexxars and others added some commits Nov 27, 2015

Update and improve Behat tests
* Bump Behat-version and add imbo/behat-api-extension
* Move behat.yml to the default location
* Add composer script for starting PHPs built in httpd for the Behat tests
* Rename Behat configuration file to .dist so developers can have local configuration files
* Install micheh/psr7-cache to easily check if a response is cacheable or not. This functionality was earlier present in Guzzle, but has been removed.
* Move the PHPUnit configuration files to root. Resolves #507
Use constants instead of null and false to improve code readability, …
…and to allow for other return values later on

@christeredvartsen christeredvartsen merged commit 8de7ffd into imbo:develop May 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

christeredvartsen added a commit to matslindh/imbo that referenced this pull request Jun 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment