Skip to content
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

ImageMagick Handler is extremely slow. #6149

Open
colethorsen opened this issue Jun 17, 2022 · 9 comments
Open

ImageMagick Handler is extremely slow. #6149

colethorsen opened this issue Jun 17, 2022 · 9 comments
Labels
enhancement PRs that improve existing functionalities

Comments

@colethorsen
Copy link
Contributor

colethorsen commented Jun 17, 2022

PHP Version

7.4

CodeIgniter4 Version

4.2

CodeIgniter4 Installation Method

Composer (as dependency to an existing project)

Which operating systems have you tested for this bug?

macOS

Which server did you use?

apache

Database

MySQL 8

What happened?

Due to running commands multiple time through multiple exec calls and through unnecessarily setting the quality to 100 the implementation is greatly slowed down

Steps to Reproduce

The following code with a 712kb image is routinely taking over 14.5 seconds to execute.

$image = \Config\Services::image();

$image->withFile($path)
	->reorient()
	->resize(2400, 16, true)
	->save(null, 90);

Expected Output

The same image processed through ImageMagick using CodeIgniter 3 takes less than 2 seconds.

There are 2 primary differences. CodeIgniter 3 executes everything in one exec command instead of 2 in CodeIgnter 4, which brings up the second difference of when CodeIgniter 4 executes a command without a quality it sets it to 100.

Simply updating line 209 of the ImageMagickHandler from:

$cmd .= $action === '-version' ? ' ' . $action : ' -quality ' . $quality . ' ' . $action;

to:

$cmd .= $action === '-version' || $quality == 100 ? ' ' . $action : ' -quality ' . $quality . ' ' . $action;

Increases performance to just over 6 seconds.

However, doing this and batching the requests together so one is made instead of 2 from:

path/to/imagemagick/convert -quality 100  -resize 2400x1600 'image.png' 'image.png'
path/to/imagemagick/convert -quality 90 'image.png' 'image.png'

to:

path/to/imagemagick/convert -quality 90  -resize 2400x1600 'image.png' 'image.png'

increases performance back to CodeIgniter 3 levels of around 2 seconds.

Anything else?

I've implemented a custom ImageMagickHandler that does this successfully for my specific use cases, however I'm not sure if there are other cases where waiting until save is called to execute a list of commands that have been compiled would be unwanted, in which case the implementation of image handlers as a whole might have to be reconsidered.

i.e at the moment calling $image->resize(2400, 16, true); actually does something, where as in the case of my custom handler it just queues the command to be run when $image->save() is called.

@colethorsen colethorsen added the bug Verified issues on the current code behavior or pull requests that will fix them label Jun 17, 2022
@kenjis kenjis removed the bug Verified issues on the current code behavior or pull requests that will fix them label Jun 17, 2022
@kenjis
Copy link
Member

kenjis commented Jun 17, 2022

Thank you for reporting.

There seems to be a lot of waste.
Could you send a PR to improve?

@colethorsen
Copy link
Contributor Author

Here is a branch with the quickly updated code, I'm not sure how ready it is. It works for the standard crop/resize/fit, but the parts that add text/watermarks etc.

develop...colethorsen:feature/imagemagick

@kenjis kenjis added the enhancement PRs that improve existing functionalities label Jun 21, 2022
@blurpy
Copy link

blurpy commented Sep 10, 2022

Anyone had the time to look at the pull request from @colethorsen?

@colethorsen
Copy link
Contributor Author

@blurpy the config files allow you to build your own handlers, you can use this Gist to implement my updated handler without having to wait for the PR to be merged into the core.

https://gist.github.com/colethorsen/9b90f8ef0443bc1f06452e203845ab02

@blurpy
Copy link

blurpy commented Sep 13, 2022

@colethorsen thanks, yeah I've already gone that way. I need to strip exif, so basing it on your changes is much better to avoid calling imagemagick 3 times to resize, strip and save.

@kenjis kenjis changed the title Bug: ImageMagick Handler is extremely slow. ImageMagick Handler is extremely slow. Jan 31, 2024
@paulbalandan
Copy link
Member

I'm tackling on this now and there are some things I like to tidy up before submitting a PR.

I'm targetting the 4.5 branch as always requiring ->save() for the image manipulation methods is a breaking change in behavior. However, this also brings another set of inconsistencies:

  1. Always calling save() will now bring down the quality to 90. Before, calling e.g. resize() will have quality set as 100 because it is passed to process() with default 100 quality. Now, quality will always be 90 when saving. Is there a reason setting save()'s default quality to 90 and not 100? Can we make it 100 though?

@kenjis
Copy link
Member

kenjis commented Mar 30, 2024

I think basically we should provide the same behavior as CI3.
If so, CI3 users can upgrade to CI4 easily.

But the APIs are completely different already.

@kenjis
Copy link
Member

kenjis commented Mar 30, 2024

Is there a reason setting save()'s default quality to 90 and not 100? Can we make it 100 though?

The default values in PHP's GD functions are not 100.
So I think we don't need 100 as the default.

GDHandler's default is also 90. We should use the value.

public function save(?string $target = null, int $quality = 90): bool

@blurpy
Copy link

blurpy commented Mar 31, 2024

Really great news that you have started to look at this!

Would it be possible to add support for stripping exif while you are at it?

I added this to my ImageMagickHandlerPatched.php:

    /**
     * Strips exif from the image.
     **
     * @return ImageMagickHandler
     */
    public function stripExif()
    {
        $this->addCommand(" -strip");

        return $this;
    }

So now my code for resize looks like this:

    $resizeSuccess = $this->image->withFile($fullImagePath)
        ->resize($imageWidth, $imageHeight, true)
        ->stripExif()
        ->save($scaledImagePath, 70);

Would be pretty nice if I could use the default handler in the future 🙂
I had to patch the handler in CI3 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that improve existing functionalities
Projects
None yet
Development

No branches or pull requests

4 participants