4rn0 / statamic-v2-image-optimizer

Statamic v2 Addon to optimizes new Asset and Glide images
https://statamic.com/marketplace/addons/imageoptimizer
4 stars 2 forks source link

Optimization/Compression level pretty bad #6

Closed madsem closed 5 years ago

madsem commented 5 years ago

Thanks for the recent updates! I'm very excited that you work on this addon because it's something that is urgently needed in the community imho :)

What isn't working very well atm, is the optimization... Tested a few big JPG images, and the results are not very good tbh. There is A LOT of room for improvement, the current results will have Google send their Pagespeed SWAT team after webmasters, I think :)

This probably is much more dramatic with bigger images, I just assume here that you've tested mainly with smaller images.

Here is a concrete example:

Original image - download from unsplash.com:

Size: 3.6MB cederic-x-494198-unsplash

Image after optimized by ImageOptimizer addon: Size: 3MB cederic-x-494198-unsplash

The original Image, run once through my Gulp build process which uses MozJpeg, at quality 50.

Size: 539KB cederic-x-494198-unsplash

As you can see, there is no real loss in quality, but filesize is several orders of magnitude less. Just 539KB instead of 3MB

You can run this test yourself, here is the Gulp task:

notify = require('gulp-notify'),
    cache = require('gulp-cache'),
    imagemin = require('gulp-imagemin'),
    imageminPngquant = require('imagemin-pngquant'),
    imageminZopfli = require('imagemin-zopfli'),
    imageminMozjpeg = require('imagemin-mozjpeg'), // install with homebrew, run: brew install libpng
    imageminGiflossy = require('imagemin-giflossy'),

gulp.task('images', function () {
    return gulp.src('assets/images/**/*')
        .pipe(cache(imagemin([
            // png
            imageminPngquant({
                speed: 1,
                quality: 60 // lossy settings
            }),
            imageminZopfli(),
            /*
            // gif lossless, much bigger filesize
             imagemin.gifsicle({
                 interlaced: true,
                 optimizationLevel: 3
            }),
            */
            // gif very light lossy, use vs  gifsicle
            imageminGiflossy({
                optimizationLevel: 3,
                optimize: 3, // keep-empty: Preserve empty transparent frames
                lossy: 2
            }),
            // svg
            imagemin.svgo({
                plugins: [{
                    removeViewBox: false
                }]
            }),
            /*
            //jpg lossless, much bigger filesize
            imagemin.jpegtran({
                progressive: true
            }),
            */
            // jpg very light lossy, use vs jpegtran
            imageminMozjpeg({
                quality: 50
            })
        ])))
        .pipe(gulp.dest('images'))
        .pipe(notify({message: 'Images compressed'}));
});

Let me know if you need anything, I'm also willing to brainstorm / help any way I can

4rn0 commented 5 years ago

Thanks again for the great feedback. I'm currently using jpegoptim with the same settings the https://github.com/spatie/image-optimizer package is using. Their jpegoptim settings are based on this stackexchange post: https://webmasters.stackexchange.com/questions/102094/google-pagespeed-how-to-satisfy-the-new-image-compression-rules

I'm trying to find out if it's possible to use MozJPEG with precompiled binaries and easy install-it-yourself instructions. In the meantime, maybe you could try to tweak the jpegoptim settings to see what works for you?

The settings are currently hardcoded, but you can edit them locally in your ImageOptimizer.php file: https://github.com/4rn0/statamic-image-optimizer/blob/master/ImageOptimizer/ImageOptimizer.php#L20

A quality of 50 seems quite low in most cases IMHO, but if it that's what you want than perhaps I could add some settings for it?

4rn0 commented 5 years ago

This is your test image optimized with jpegoptim and quality 50 (-m50). Not as good as your MozJPEG setup, but a lot smaller than with the default -m85

screenshot 2018-12-31 at 13 12 20

madsem commented 5 years ago

That's still twice as large :) And not sure about quality loss. I've read a little and it seems Jpegoptim should just work with MozJpeg as a drop-in replacement. Will see if I can get the binaries pre-compiled and run some tests.

But your idea to make the settings adjustable from the CP is great, that way users can change it to suit their needs for all the image formats.

madsem commented 5 years ago

Happy new year!!

I've sat down with some coffee + doughnut and ran some tests. It pretty much works the same way, the only difference is that MozJpeg doesn't support in-place optimization to prevent image degradation when run multiple times. https://github.com/mozilla/mozjpeg/issues/248

So maybe it could be worth it to slightly adjust the structuring of the addon, to make it more broadly compatible with different image compressor libraries?

For example, this seems very robust: https://github.com/bensquire/php-image-optim This supports different commands on a per tool basis so you can easily also chain multiple tools for better results (as shown in the .PNG example)

PS: I'm not talking about integrating that library, I'm talking more about the structure , usage of interface etc. So that in the end it's super easy to add more tools / chain tools.

PSPS: Actually, I would even be able to sponsor this a little w. invoice from you :)

4rn0 commented 5 years ago

Hey there! Happy new year! I'm thinking about adding something like this for your use case:

screenshot 2019-01-01 at 15 51 52

That way 'normal' users can just use the default settings and included binaries and more advanced users like you could set something up to your own liking. You could add multiple optimizers or custom optimizers like MozJPEG that way.

How would you feel about that?

madsem commented 5 years ago

Great idea, that would def. work. But how would you solve having to run different CLI commands? Like I wrote above, some (incl MozJpeg) do not support in-place-replacements, and require a temporary file to be used.

edit: This is what I mean: https://github.com/bensquire/php-image-optim/blob/56c5e9311a913a17f16e3f30bbe07e18a5079e2e/src/PHPImageOptim/Tools/Jpeg/MozJpeg.php#L32

4rn0 commented 5 years ago

You can just add commands and arguments through the interface in the screenshot. They will run in order if there are multiple.

If you require a destination or outfile argument or something like that, you can just reference it with %s, like in the pngquant example.

madsem commented 5 years ago

ah understood, so for example %s -outfile %s %s in the Arguments column would take care of that? But would it also use the same file names so that it doesn't create new files and/or leaves old file corpses?

4rn0 commented 5 years ago

Yep!

madsem commented 5 years ago

That is pretty sweet! Yeah, awesome 👍

4rn0 commented 5 years ago

The correct argument depends on the tool you’re using of course!

madsem commented 5 years ago

yeah, of course, gotta be a little careful with that, in my dev environment it overwrote all assets with zero byte versions because at first I didn't realize that it requires a temporary file :D

madsem commented 5 years ago

Are you planning on including the MozJpeg binary batteries already? If so I can look how to compile it or find a reliable place that has them already. Or do you want to support it through advanced only

4rn0 commented 5 years ago

For now I’m thinking advanced settings only. I’ll release an update soonish!

madsem commented 5 years ago

awesome, sounds great! Exciting

madsem commented 5 years ago

It's going to make image heavy sites fly... Literally :)

4rn0 commented 5 years ago

I just tagged a new release with this functionality included. You're welcome to test it! :-)

madsem commented 5 years ago

wow, you're fast! Going to install right away and play around with it

madsem commented 5 years ago

Ok I think we're almost there :) I have installed mozjpeg and are able to manually run optimizations through my CLI (when ssh'd into homestead)

But the same arguments throw a sprinf error in the addon

[2019-01-02 11:05:19] dev.ERROR: ErrorException: sprintf(): Too few arguments in /home/vagrant/code/dev.vm/site/addons/ImageOptimizer/ImageOptimizer.php:147

I can run this manually in my terminal:

vagrant@homestead:~$ mozjpeg-3.2/build/jpegtran -optimize -progressive /home/vagrant/code/dev.vm/assets/apps/screenshots/cederic-x-494198-unsplash.jpg > /home/vagrant/code/dev.vm/assets/apps/screenshots/new_1.jpg
vagrant@homestead:~$ mozjpeg-3.2/build/cjpeg -quality 50 -optimize -progressive /home/vagrant/code/dev.vm/assets/apps/screenshots/cederic-x-494198-unsplash.jpg > /home/vagrant/code/dev.vm/assets/apps/screenshots/new_2.jpg

And receive the exact same results as with my Gulp build setup.

This is how I have setup the addon: screen shot 2019-01-02 at 12 08 49 pm

And it throws the above error message.

There seems to be also another bug, when I trigger the optimizations with the "Optimize again" button inside the addon settings, the logs are empty. screen shot 2019-01-02 at 12 11 24 pm

If I trigger it with the "Optimize again" link, inside the Asset manager, I get the sprintf error: screen shot 2019-01-02 at 12 12 03 pm

In both cases though, no optimizations take place.

And if I upload a new image that hasn't been in my assets before installation of the addon, I also get the sprintf error: screen shot 2019-01-02 at 12 13 32 pm

madsem commented 5 years ago

PS: I believe the problem really is, that the advanced image compressors require in and out files. So the addon code has to use a temporary file, and after the temporary file was written, it has to delete the old file and rename the temporary one with the old file name, so that we won't end up with broken urls. :)

I think there should be custom directives instead of the standard %s ones used by sprintf. That way the script will know exactly what to replace with what, because the cli command order can change depending on OS or library used.

For example:

4rn0 commented 5 years ago

I think there should be custom directives instead of the standard %s ones used by sprintf.

I think this, combined with some extra code to handle the tempfiles is the way to go.

I’m away from home for a couple of days, but I will try to implement it in the weekend.

madsem commented 5 years ago

awesome, thanks man! Appreciate that 👍

4rn0 commented 5 years ago

I just tagged a new release with some custom directives and extra logic to handle tempfiles: https://github.com/4rn0/statamic-image-optimizer/releases/tag/v2.0.5 I also updated the documentation on how to customize your setup: https://github.com/4rn0/statamic-image-optimizer/blob/master/DOCUMENTATION.md#customization

MozJPEG is working great on my local version like this!

screenshot 2019-01-05 at 20 26 55

4rn0 commented 5 years ago

My MozJPEG setup:

screenshot 2019-01-05 at 19 58 45

madsem commented 5 years ago

Reopening this.

Sorry, it took me a week to get back to you :)

Just got around to test the newest iteration, but it's still buggy and also a new bug was introduced haha.

First, the custom optimization doesn't work anymore, at all. I only get errors that it wasn't found. With the same arguments that were working before: screen shot 2019-01-11 at 3 18 59 pm

This path clearly exists in my test install: screen shot 2019-01-11 at 3 19 46 pm

here are the error logs from Statamic:

[2019-01-11 14:16:55] production.INFO: Starting optimization: "~/mozjpeg-3.2/build/cjpeg" -quality 50 -optimize -progressive %s > %s '/home/vagrant/code/dev.vm/assets/main/50549710-5b52eb80-0c62-11e9-8807-b3f247b4f20f.jpg'  
[2019-01-11 14:16:55] production.ERROR: sh: 1: ~/mozjpeg-3.2/build/cjpeg: not found

Bug #2 is that the addon now creates a tmp file, directly inside the webroot of Statamic: screen shot 2019-01-11 at 3 21 23 pm

This should be changed just for security, also better to be safe not that something important will be deleted on accident :)

Once the optimization really works we can also look into Glide maybe, in my tests it didn't work even with "Serve cached images directly" turned off, but that's of course to be expected with the current errors.

Let me know if you need any help

madsem commented 5 years ago

PS: I used the old %s variation in my first test, now I tested again with the new :file & :temp placeholders, but same result. Images broken on new upload, when trying to generate presets errors in log and binary can not be found.

But, the %s file isn't generated anymore, that was my bad because of faulty first test.

[2019-01-11 14:31:26] production.INFO: Starting optimization: "mozjpeg-3.2/build/cjpeg" -quality 50 -optimize -progressive -outfile '/tmp/imageoptimizer7Vhiyz' '/home/vagrant/code/dev.vm/assets/main/50549710-5b52eb80-0c62-11e9-8807-b3f247b4f20f.jpg'  
[2019-01-11 14:31:26] production.ERROR: sh: 1: mozjpeg-3.2/build/cjpeg: not found

[2019-01-11 14:31:27] production.ERROR: ErrorException: getimagesize(): Read error! in /home/vagrant/code/dev.vm/statamic/core/Assets/DimensionBuilder.php:111
Stack trace:
#0 [internal function]: Illuminate\Foundation\Bootstrap\HandleExceptions->handleError(8, 'getimagesize():...', '/home/vagrant/c...', 111, Array)
#1 /home/vagrant/code/dev.vm/statamic/core/Assets/DimensionBuilder.php(111): getimagesize('/home/vagrant/c...')
#2 /home/vagrant/code/dev.vm/statamic/core/Assets/DimensionBuilder.php(53): Statamic\Assets\DimensionBuilder->getImageDimensions('/home/vagrant/c...')
#3 /home/vagrant/code/dev.vm/statamic/core/Assets/DimensionBuilder.php(67): Statamic\Assets\DimensionBuilder->dimensions()
#4 /home/vagrant/code/dev.vm/statamic/core/Assets/Asset.php(385): Statamic\Assets\DimensionBuilder->width()
#5 /home/vagrant/code/dev.vm/statamic/core/Http/Controllers/AssetThumbnailController.php(142): Statamic\Assets\Asset->width()
#6 /home/vagrant/code/dev.vm/statamic/core/Http/Controllers/AssetThumbnailController.php(60): Statamic\Http\Controllers\AssetThumbnailController->getPlaceholderResponse()
#7 [internal function]: Statamic\Http\Controllers\AssetThumbnailController->show('bWFpbjo6NTA1NDk...', 'small')
#8 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Routing/Controller.php(256): call_user_func_array(Array, Array)
#9 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Routing/ControllerDispatcher.php(164): Illuminate\Routing\Controller->callAction('show', Array)
#10 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Routing/ControllerDispatcher.php(112): Illuminate\Routing\ControllerDispatcher->call(Object(Statamic\Http\Controllers\AssetThumbnailController), Object(Illuminate\Routing\Route), 'show')
#11 [internal function]: Illuminate\Routing\ControllerDispatcher->Illuminate\Routing\{closure}(Object(Illuminate\Http\Request))
#12 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(139): call_user_func(Object(Closure), Object(Illuminate\Http\Request))
#13 [internal function]: Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#14 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(102): call_user_func(Object(Closure), Object(Illuminate\Http\Request))
#15 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Routing/ControllerDispatcher.php(114): Illuminate\Pipeline\Pipeline->then(Object(Closure))
#16 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Routing/ControllerDispatcher.php(68): Illuminate\Routing\ControllerDispatcher->callWithinStack(Object(Statamic\Http\Controllers\AssetThumbnailController), Object(Illuminate\Routing\Route), Object(Illuminate\Http\Request), 'show')
#17 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Routing/Route.php(203): Illuminate\Routing\ControllerDispatcher->dispatch(Object(Illuminate\Routing\Route), Object(Illuminate\Http\Request), 'Statamic\\Http\\C...', 'show')
#18 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Routing/Route.php(134): Illuminate\Routing\Route->runWithCustomDispatcher(Object(Illuminate\Http\Request))
#19 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Routing/Router.php(708): Illuminate\Routing\Route->run(Object(Illuminate\Http\Request))
#20 [internal function]: Illuminate\Routing\Router->Illuminate\Routing\{closure}(Object(Illuminate\Http\Request))
#21 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(139): call_user_func(Object(Closure), Object(Illuminate\Http\Request))
#22 /home/vagrant/code/dev.vm/statamic/core/Http/Middleware/CP/Authenticate.php(62): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#23 [internal function]: Statamic\Http\Middleware\CP\Authenticate->handle(Object(Illuminate\Http\Request), Object(Closure))
#24 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#25 /home/vagrant/code/dev.vm/statamic/core/Http/Middleware/Outpost.php(48): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#26 [internal function]: Statamic\Http\Middleware\Outpost->handle(Object(Illuminate\Http\Request), Object(Closure))
#27 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#28 /home/vagrant/code/dev.vm/statamic/core/Http/Middleware/CP/Localize.php(25): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#29 [internal function]: Statamic\Http\Middleware\CP\Localize->handle(Object(Illuminate\Http\Request), Object(Closure))
#30 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#31 /home/vagrant/code/dev.vm/statamic/core/Http/Middleware/CP/DefaultLocale.php(28): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#32 [internal function]: Statamic\Http\Middleware\CP\DefaultLocale->handle(Object(Illuminate\Http\Request), Object(Closure))
#33 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#34 /home/vagrant/code/dev.vm/statamic/core/Http/Middleware/CP/AddHeaders.php(19): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#35 [internal function]: Statamic\Http\Middleware\CP\AddHeaders->handle(Object(Illuminate\Http\Request), Object(Closure))
#36 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#37 /home/vagrant/code/dev.vm/statamic/core/Http/Middleware/CpEnabled.php(23): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#38 [internal function]: Statamic\Http\Middleware\CpEnabled->handle(Object(Illuminate\Http\Request), Object(Closure))
#39 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#40 [internal function]: Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#41 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(102): call_user_func(Object(Closure), Object(Illuminate\Http\Request))
#42 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Routing/Router.php(710): Illuminate\Pipeline\Pipeline->then(Object(Closure))
#43 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Routing/Router.php(674): Illuminate\Routing\Router->runRouteWithinStack(Object(Illuminate\Routing\Route), Object(Illuminate\Http\Request))
#44 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Routing/Router.php(635): Illuminate\Routing\Router->dispatchToRoute(Object(Illuminate\Http\Request))
#45 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(236): Illuminate\Routing\Router->dispatch(Object(Illuminate\Http\Request))
#46 [internal function]: Illuminate\Foundation\Http\Kernel->Illuminate\Foundation\Http\{closure}(Object(Illuminate\Http\Request))
#47 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(139): call_user_func(Object(Closure), Object(Illuminate\Http\Request))
#48 /home/vagrant/code/dev.vm/statamic/core/Http/Middleware/TransformsRequest.php(31): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#49 [internal function]: Statamic\Http\Middleware\TransformsRequest->handle(Object(Illuminate\Http\Request), Object(Closure))
#50 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#51 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/VerifyCsrfToken.php(50): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#52 [internal function]: Illuminate\Foundation\Http\Middleware\VerifyCsrfToken->handle(Object(Illuminate\Http\Request), Object(Closure))
#53 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#54 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/View/Middleware/ShareErrorsFromSession.php(49): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#55 [internal function]: Illuminate\View\Middleware\ShareErrorsFromSession->handle(Object(Illuminate\Http\Request), Object(Closure))
#56 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#57 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Session/Middleware/StartSession.php(62): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#58 [internal function]: Illuminate\Session\Middleware\StartSession->handle(Object(Illuminate\Http\Request), Object(Closure))
#59 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#60 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Cookie/Middleware/AddQueuedCookiesToResponse.php(37): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#61 [internal function]: Illuminate\Cookie\Middleware\AddQueuedCookiesToResponse->handle(Object(Illuminate\Http\Request), Object(Closure))
#62 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#63 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Cookie/Middleware/EncryptCookies.php(59): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#64 [internal function]: Illuminate\Cookie\Middleware\EncryptCookies->handle(Object(Illuminate\Http\Request), Object(Closure))
#65 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#66 /home/vagrant/code/dev.vm/statamic/core/StaticCaching/Middleware/Retrieve.php(33): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#67 [internal function]: Statamic\StaticCaching\Middleware\Retrieve->handle(Object(Illuminate\Http\Request), Object(Closure))
#68 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#69 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/CheckForMaintenanceMode.php(44): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#70 [internal function]: Illuminate\Foundation\Http\Middleware\CheckForMaintenanceMode->handle(Object(Illuminate\Http\Request), Object(Closure))
#71 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(124): call_user_func_array(Array, Array)
#72 [internal function]: Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request))
#73 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(102): call_user_func(Object(Closure), Object(Illuminate\Http\Request))
#74 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(122): Illuminate\Pipeline\Pipeline->then(Object(Closure))
#75 /home/vagrant/code/dev.vm/statamic/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(87): Illuminate\Foundation\Http\Kernel->sendRequestThroughRouter(Object(Illuminate\Http\Request))
#76 /home/vagrant/code/dev.vm/index.php(155): Illuminate\Foundation\Http\Kernel->handle(Object(Illuminate\Http\Request))
#77 {main}  
4rn0 commented 5 years ago

Can you try providing an absolute path to your custom binary? Right now it's relative and resolves to ~/mozjpeg-3.2/build/cjpeg, but that should probably be something like /usr/bin/mozjpeg/build/cjpeg depending on your setup.

I have it running locally with executable '/usr/local/opt/mozjpeg/bin/cjpeg' and arguments '-quality 85 -optimize -outfile :temp' and it's working great!

madsem commented 5 years ago

Hey, I've tried both actually.

Check the screenshot of my terminal above, the actual location of the binary in my test environment is mozjpeg-3.2/build/cjpeg which was also working before, haven't changed the setup since our last tests.

Also, I've tried both: ~/mozjpeg-3.2/build/cjpeg & only mozjpeg-3.2/build/cjpeg but it simply doesn't find it anymore. Also haven't had time to look over the code of the addon yet.

4rn0 commented 5 years ago

Check the screenshot of my terminal above, the actual location of the binary in my test environment is mozjpeg-3.2/build/cjpeg which was also working before, haven't changed the setup since our last tests.

But that's still a relative location ... When you enter 'which cjpeg' in the terminal, what location does it give you?

madsem commented 5 years ago

Ah okay sorry, that would be /usr/bin/cjpeg Testing now again:

ok now I know what's broken... So you're right, the absolute path works and is found. But the addon destroys all images of the site when running "optimize", either by clicking the button or running the php please optimize command.

I'm not sure if the same goes for the generate-presets command as all my images are broken now and i can't test right now :)

PS: and by destroy, I mean it overwrites all files with zero byte versions. This of course happens only when using a library with temp files.

4rn0 commented 5 years ago

Anything in the logs of this last run? What arguments did you use?

madsem commented 5 years ago

Nothing in the logs, the aguments are: /usr/bin/cjpeg -quality 50 -optimize -progressive -outfile :temp

When I now run php please optimize, all JPEG images have zero bytes...

So the core problem is, that the addon doesn't use the tempfile all the time, it should run any optimizations using the tempfile if it's using a library that works with one and is working with a file that requires this library.

Otherwise, there will always be cases like this.

4rn0 commented 5 years ago

Nothing in the logs

That's just weird, looking at the code it should at least log the command it's trying to run: https://github.com/4rn0/statamic-image-optimizer/blob/master/ImageOptimizer/ImageOptimizer.php#L230

But then if all your images are broking right now, it might not recognize any mimetypes and not run at all ... Could you try on a fresh statamic install with a few images?

So the core problem is, that the addon doesn't use the tempfile all the time, it should run any optimizations using the tempfile if it's using a library that works with one and is working with a file that requires this library.

It actually does use tempfiles now, that's what the whole :temp thing is for ... https://github.com/4rn0/statamic-image-optimizer/blob/master/ImageOptimizer/ImageOptimizer.php#L131

madsem commented 5 years ago

Ok will clone this repo in a fresh install tomorrow am and see what’s happening exactly. The images that were overwritten with zero bytes were all jpgs btw.

madsem commented 5 years ago

morning! So I've installed a brand-spanking new version of Statamic, latest 2.11.6.

I'm using the default setup, with the default images for testing: screen shot 2019-01-12 at 8 45 40 am

I have installed the plugin, and adjusted the settings: screen shot 2019-01-12 at 8 45 12 am

Logs are empty: screen shot 2019-01-12 at 8 47 35 am

Now I'm going to run php please optimize:

results: logs:

[2019-01-12 07:48:45] production.INFO: Starting optimization: "/usr/bin/cjpeg" -quality 50 -optimize -progressive -outfile '/private/var/folders/ps/8ln4f_x10t7c27_s0mddggtm0000gn/T/imageoptimizerIH5xYw' '/Users/mydev/Business/Code/fresh-statamic/assets/img/black-bear-cubs.jpg'  
[2019-01-12 07:48:45] production.ERROR: sh: /usr/bin/cjpeg: No such file or directory

[2019-01-12 07:48:45] production.INFO: Starting optimization: "/usr/bin/cjpeg" -quality 50 -optimize -progressive -outfile '/private/var/folders/ps/8ln4f_x10t7c27_s0mddggtm0000gn/T/imageoptimizerLVX6wx' '/Users/mydev/Business/Code/fresh-statamic/assets/img/coffee-mug.jpg'  
[2019-01-12 07:48:45] production.ERROR: sh: /usr/bin/cjpeg: No such file or directory

[2019-01-12 07:48:45] production.INFO: Starting optimization: "/usr/bin/cjpeg" -quality 50 -optimize -progressive -outfile '/private/var/folders/ps/8ln4f_x10t7c27_s0mddggtm0000gn/T/imageoptimizerN92D3c' '/Users/mydev/Business/Code/fresh-statamic/assets/img/dangle.jpg'  
[2019-01-12 07:48:45] production.ERROR: sh: /usr/bin/cjpeg: No such file or directory

[2019-01-12 07:48:45] production.INFO: Starting optimization: "/usr/bin/cjpeg" -quality 50 -optimize -progressive -outfile '/private/var/folders/ps/8ln4f_x10t7c27_s0mddggtm0000gn/T/imageoptimizer3dsfzE' '/Users/mydev/Business/Code/fresh-statamic/assets/img/desert.jpg'  
[2019-01-12 07:48:45] production.ERROR: sh: /usr/bin/cjpeg: No such file or directory

[2019-01-12 07:48:45] production.INFO: Starting optimization: "/usr/bin/cjpeg" -quality 50 -optimize -progressive -outfile '/private/var/folders/ps/8ln4f_x10t7c27_s0mddggtm0000gn/T/imageoptimizerqVLQUz' '/Users/mydev/Business/Code/fresh-statamic/assets/img/me.jpg'  
[2019-01-12 07:48:45] production.ERROR: sh: /usr/bin/cjpeg: No such file or directory

[2019-01-12 07:48:45] production.INFO: Starting optimization: "/usr/bin/cjpeg" -quality 50 -optimize -progressive -outfile '/private/var/folders/ps/8ln4f_x10t7c27_s0mddggtm0000gn/T/imageoptimizerqX9Dno' '/Users/mydev/Business/Code/fresh-statamic/assets/img/redwood-balance.jpg'  
[2019-01-12 07:48:45] production.ERROR: sh: /usr/bin/cjpeg: No such file or directory

[2019-01-12 07:48:45] production.INFO: Starting optimization: "/usr/bin/cjpeg" -quality 50 -optimize -progressive -outfile '/private/var/folders/ps/8ln4f_x10t7c27_s0mddggtm0000gn/T/imageoptimizer5didp1' '/Users/mydev/Business/Code/fresh-statamic/assets/img/redwood-james-irvine-trail.jpg'  
[2019-01-12 07:48:45] production.ERROR: sh: /usr/bin/cjpeg: No such file or directory

[2019-01-12 07:48:45] production.INFO: Starting optimization: "/usr/bin/cjpeg" -quality 50 -optimize -progressive -outfile '/private/var/folders/ps/8ln4f_x10t7c27_s0mddggtm0000gn/T/imageoptimizer4pKxxg' '/Users/mydev/Business/Code/fresh-statamic/assets/img/redwood-north-ridge-trail.jpg'  
[2019-01-12 07:48:45] production.ERROR: sh: /usr/bin/cjpeg: No such file or directory

[2019-01-12 07:48:45] production.INFO: Starting optimization: "/usr/bin/cjpeg" -quality 50 -optimize -progressive -outfile '/private/var/folders/ps/8ln4f_x10t7c27_s0mddggtm0000gn/T/imageoptimizerlgULBu' '/Users/mydev/Business/Code/fresh-statamic/assets/img/redwood-sign.jpg'  
[2019-01-12 07:48:45] production.ERROR: sh: /usr/bin/cjpeg: No such file or directory

[2019-01-12 07:48:45] production.INFO: Starting optimization: "/usr/bin/cjpeg" -quality 50 -optimize -progressive -outfile '/private/var/folders/ps/8ln4f_x10t7c27_s0mddggtm0000gn/T/imageoptimizerWemzh0' '/Users/mydev/Business/Code/fresh-statamic/assets/img/redwood-snow.jpg'  
[2019-01-12 07:48:45] production.ERROR: sh: /usr/bin/cjpeg: No such file or directory

[2019-01-12 07:48:45] production.INFO: Starting optimization: "/usr/bin/cjpeg" -quality 50 -optimize -progressive -outfile '/private/var/folders/ps/8ln4f_x10t7c27_s0mddggtm0000gn/T/imageoptimizerVbP9Ig' '/Users/mydev/Business/Code/fresh-statamic/assets/img/redwood-sunrise.jpg'  
[2019-01-12 07:48:45] production.ERROR: sh: /usr/bin/cjpeg: No such file or directory

[2019-01-12 07:48:45] production.INFO: Starting optimization: "/usr/bin/cjpeg" -quality 50 -optimize -progressive -outfile '/private/var/folders/ps/8ln4f_x10t7c27_s0mddggtm0000gn/T/imageoptimizerrtFPk1' '/Users/mydev/Business/Code/fresh-statamic/assets/img/stetson.jpg'  
[2019-01-12 07:48:45] production.ERROR: sh: /usr/bin/cjpeg: No such file or directory

image folder: screen shot 2019-01-12 at 8 50 19 am (note the zero byte filesize, all .jpg images now have zero bytes, confirmed that also)

screen shot 2019-01-12 at 8 51 46 am

Going to reinstall fresh version with new images, and test another way of writing the arguments, see if that changes anything. :file > :temp

But I think one good idea would be to check if filesize is zero, before renaming, and then output some error message. That would at least prevent images from being overwritten with zero byte versions if something goes wrong

4rn0 commented 5 years ago

Morning! Thanks for taking the time to test like this with your setup.

Looking at the logs, I'd say 'production.ERROR: sh: /usr/bin/cjpeg: No such file or directory' is the biggest problem here. It indicates that cjpeg is not installed at /usr/bin ... Is this still running on your homestead setup? How exactly did you install mozjpeg there?

The 'check if filesize is zero' thing is clever and I will add that! The ':file > :temp' thing won't work by the way. The filepath gets added to the command as the last argument automatically.

madsem commented 5 years ago

Hey, the install is def. in /usr/bin/cjpeg.

I will make a pull request in a bit, rewrote some small parts that seem to have been the issue. Then we can discuss the changes.

Regarding the ':file > :temp' this is actually what cjpeg says how it should be used on unix-like systems: https://github.com/mozilla/mozjpeg/blob/master/usage.txt

An alternative way listed under non-unix systems is cjpeg [switches] -outfile jpegfile imagefile

With the changes I have made, both ways work now dynamically. I am testing the other compressor libraries now if things are smooth there as well and will then make the pull request.

madsem commented 5 years ago

Okay won't make it anymore today, took longer than I thought. Initially I wanted to add a feature to be able to convert images from one mime type to another on the fly with a new command structure, that also works flawlessly, but Statamic sadly has issues with that. If a jog is uploaded, it's already saved in Stache and god knows where, before the addon gets the event, so it doesn't seem possible to on-the-fly make conversions like that.

screen shot 2019-01-12 at 7 06 25 pm

Going to remove the conversion feature again and just leave the command structure so that in advanced mode you can run any command you wish in any order.

what do you think?

4rn0 commented 5 years ago

So you want to make the advanced optimizers simpler by just having the mimetype and the complete CLI command? I think that's probably a good idea and I could make it that way for you.

Using my original setup that would result in something like:

Type Command
image/jpeg /usr/local/opt/mozjpeg/bin/cjpeg -quality 85 -optimize -outfile :temp :file

As you can see it still uses my current tempfile / outfile construction where I copy the contents of the tempfile back to the original file after the optimization.

What do you think? Would this make the addon easier to use? For you and in general?

madsem commented 5 years ago

Oh that is already done, i’ll make pull request tomorrow then you can check it out.

I just want to remove the mime type conversion feature I added first. Because it didn’t work well

madsem commented 5 years ago

also fixed a few bugs I found, plus the zero byte files etc

4rn0 commented 5 years ago

Ah, I've also committed some of it to master already. I'll await your pull request and compare it with my changes when you make it! :-)

madsem commented 5 years ago

Morning :)

I have just created the pull request.

This new version uses placeholders by default for all optimizers, that makes it easier to maintain instead of keeping two ways of building commands (one for default optimizers without dynamic placeholders, and one with).

Also improved the binary finder slightly, you were passing in a full path, which made the binary finder return false when using custom libraries. It also throws an error now when binary isn't installed on system and logs it.

For binaries, you only need to pass the name in now, path will be automatically found with the binaryfinder method, also for custom optimizers.

Improved the way temp files are moved, and added a check for filesize. Since then I have made many tests but never again had zero byte files.

Give it a whirl and let me know what you think.

Next I would like to go over the part that handles glide and come up with some method that it handles those correctly if possible

madsem commented 5 years ago

The ':file > :temp' thing won't work by the way. The filepath gets added to the command as the last argument automatically.

PS: this actually created issues with the custom optimizers, because the filepath was added twice (with some setups), which also played part in the zero bytes bug I think. Now that commands are completely flexible and only placeholders are swapped out at runtime, this seems to work much smoother.

madsem commented 5 years ago

Added a workaround for the glide.generated event, just committed it.

madsem commented 5 years ago

And there is another issue I just figured out, the glide.generated event, is also fired on every page load when images are served direct from cache...

That's kind of a problem, because every page load of course triggers the image optimizers to run, on pages with 20,30+ images that can easily cause the server to explode :)

Got any ideas how we could get around this and detect if an image was already optimized once, and if $optimized) { exit(0); }

So that only necessary requests trigger the optimizer, i.e: when not previously optimized, asset changed or cache TTL is in the past

madsem commented 5 years ago

ah man, of course! got it, you already save this for every asset, so I can just check if an asset exists & was optimized already...

madsem commented 5 years ago

alright, I think we're good now!

glide optimization is fully cached now. Optimizers will only run if the image wasn't optimized yet, or if the image was replaced (i.e: cache was cleared).

That actually sped up the homepage for the redwood theme visibly already.

4rn0 commented 5 years ago

Wow, you've been really busy while I was away! Awesome stuff man! I'll close this issue and respond in your PR soonish!