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

Enable timeout setting for the optimize method #187

Closed
wants to merge 6 commits into from

Conversation

golubev
Copy link
Contributor

@golubev golubev commented Mar 20, 2023

This PR introduces an ability to set timeout for the OptimizerChain other than the default 60s when calling the optimize method.

The optimize manipulation configuration under the hood is being stored as a JSON string at runtime. And it is being json_encode/json_decode'd each time when we are configuring the optimizer or running the optimizations. This design decision was a limiting factor to find a proper solution. So, IMO, the only way to achieve the goal of this PR was to play around with the configuration array structure as no PHP OOP can be used to pass the optimizers' configuration.

Rationale

I've ran into an issue while running optimizer for a 2600px retina (2x) WebP images. From time to time such images are hitting the default 60s timeout.

Here is a sample of an error message thrown via the Symfony\Component\Process\Exception\ProcessTimedOutException:

The process \"\"cwebp\" -q 85 -m 6 -pass 10 -mt '/var/www/storage/media-library/temp/JWrithAGLF2WKk74bEtaNtPXtlh9sSVg/wxsCJGDr2AQeM7WnSoGlugLiVa7hSHB02600px-2x-webp.JPG' -o '/var/www/storage/media-library/temp/JWrithAGLF2WKk74bEtaNtPXtlh9sSVg/wxsCJGDr2AQeM7WnSoGlugLiVa7hSHB02600px-2x-webp.JPG'\" exceeded the timeout of 60 seconds.

Under the hood in the Spatie\ImageOptimizer\OptimizerChain there’s a property protected $timeout = 60; with a public setter, see:

But higher in the call stack the property is not being used at all and OptimizerChain instances are always running with the default timeout value of 60 s, see

image/src/Image.php

Lines 138 to 159 in cb1bb1a

protected function performOptimization($path, array $optimizerChainConfiguration): void
{
$optimizerChain = $this->optimizerChain ?? OptimizerChainFactory::create();
if (count($optimizerChainConfiguration)) {
$existingOptimizers = $optimizerChain->getOptimizers();
$optimizers = array_map(function (array $optimizerOptions, string $optimizerClassName) use ($existingOptimizers) {
$optimizer = array_values(array_filter($existingOptimizers, function ($optimizer) use ($optimizerClassName) {
return $optimizer::class === $optimizerClassName;
}));
$optimizer = isset($optimizer[0]) && $optimizer[0] instanceof BaseOptimizer ? $optimizer[0] : new $optimizerClassName();
return $optimizer->setOptions($optimizerOptions)->setBinaryPath($optimizer->binaryPath);
}, $optimizerChainConfiguration, array_keys($optimizerChainConfiguration));
$optimizerChain->setOptimizers($optimizers);
}
$optimizerChain->optimize($path);
}
.

So it was required to introduce an ability to set the timeout.

@chrisdicarlo
Copy link

Definitely interested in this PR! I just hit the same issue when processing PNGs...

Any timeline if this will be merged?

@freekmurze
Copy link
Member

@golubev Thanks! Could you also document this new option?

@golubev
Copy link
Contributor Author

golubev commented Jun 20, 2023

@freekmurze Sorry, have lots of work right now, but I'd like to add the docs to finalize the PR. Perhaps, I'd be able to do that in a month or something like that when it gets easier.

@golubev golubev changed the title Enable timeout adjustment for the OptimizerChain Enable timeout setting for the optimize method Jul 18, 2023
@golubev
Copy link
Contributor Author

golubev commented Jul 18, 2023

@freekmurze I've updated the docs. Can you have a look, please?

'--all-progressive',
]])
->optimize([
'timeout' => 120,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of adding a key in the array, let's add a dedicated parameter to specify timeout.

So this would use default timeout.

php
Image::load('example.jpg')
     ->optimize([
         'optimizers' => [
             Jpegoptim::class => [
                 '--all-progressive',
             ],
         ],
     ])

and this is how you would use a custom timeout:

Image::load('example.jpg')
     ->optimize([
         'optimizers' => [
             Jpegoptim::class => [
                 '--all-progressive',
             ],
         ],
     ], timeout: 120)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @freekmurze 👋 Thanks for your feedback! I've updated the PR adding the $timeout parameter.

It seems that the only option I had to implement this was to reuse that array key. It's being added under the hood when the $timeout parameter is being passed:

    if ($timeout !== null) {
        $optimizationOptions['timeout'] = $timeout;
    }

The optimization options are always passed to the underlying layers with JSON serialization/deserialization:

    return $this->addManipulation('optimize', json_encode($optimizationOptions));

That's why only a single array is available for any configuration.

Moreover, in a Laravel project with the spatie/laravel-medialibrary one might want to be able to configure a timeout for optimizers. This can be achieved with the help of the image_optimizers settings key in the config/media-library.php adding e.g. 'timeout' => 120 to the array:

    'image_optimizers' => [
        'optimizers' => [
            Spatie\ImageOptimizer\Optimizers\Jpegoptim::class => [
                '-m85',
                // ...
            ],
        ],
        'timeout' => 120,
    ],

So I think the only possible way is to have both options available.

@freekmurze
Copy link
Member

Meanwhile, we tagged a new major release. Could you create a new PR that targets v3?

@golubev
Copy link
Contributor Author

golubev commented Dec 17, 2023

I was happy to see that in v3 you should pass an OptimizerChain instance to the optimize() method 👍 Instead of an array that was being JSON serialized/deserialized under the hood in v2. In v3 you may simply adjust the timeout for image optimizers via an OptimizerChain instance.

I've created #208 - updated docs and the test.

@golubev golubev deleted the optimizers-configuration branch December 25, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants