codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.36k stars 1.9k forks source link

ImageMagick Handler is extremely slow. #6149

Open colethorsen opened 2 years ago

colethorsen commented 2 years ago

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.

kenjis commented 2 years ago

Thank you for reporting.

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

colethorsen commented 2 years ago

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.

https://github.com/codeigniter4/CodeIgniter4/compare/develop...colethorsen:feature/imagemagick

blurpy commented 2 years ago

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

colethorsen commented 2 years ago

@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 commented 2 years ago

@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.

paulbalandan commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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. https://github.com/codeigniter4/CodeIgniter4/blob/7ca24b2380c4326698edf847f4320fe69afa7e29/system/Images/Handlers/GDHandler.php#L210

blurpy commented 7 months ago

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 :slightly_smiling_face: I had to patch the handler in CI3 as well.