liip / LiipImagineBundle

Symfony Bundle to assist in image manipulation using the imagine library
http://liip.ch
MIT License
1.66k stars 378 forks source link

Add a warning message (or even error message) if cjpeg binary is from libjpeg-turbo instead of mozjpeg #1532

Open welcoMattic opened 1 year ago

welcoMattic commented 1 year ago

Is your feature request related to a problem? Please describe. Emit a warning or throw an error if the cjpeg binary is from libjpeg-turbo instead of mozjpeg.

Describe the solution you'd like A possible solution could be to run cjpeg -version (which is available on both binaries) and check on the output. If it contains libjpeg-turbo, a warning message can be logged, or an error can throw to alert the developer that the current binary is not supported by the bundle.

The main difference in the bundle usage context is the -quant-table argument, which is not available on cjpeg from libjpeg-turbo. Using this binary leads to a 500 error on my application. If needed I can make a reproducer easily.

Describe alternatives you've considered I've considered to version a standalone binary of mozjpeg, but I have applications that runs on debian, exherbo, arch, macOS … It's not ok to have a specific version of mozjpeg for all OS.

dbu commented 1 year ago

hi, thanks for the clear report. is the rest of cjpeg between the two libraries compatible (for what imagine is using)? i wonder if we break things for people if we throw an exception and people who did not use features that use the -quant-table argument would see it as regression.

what if we catch the error of binary excecution and do the check if things failed, to be able to do a better error message?

the other thing this brought to my mind is that we should have some "check setup" command that checks if all the binaries are configured correctly (and that could check the version and warn about the wrong cjpeg binary)

welcoMattic commented 1 year ago

For what I tested, the rest of the features between binaries seems identical, but we can expect more divergence in the future.

Unless the user set quant_table to false in the bundle configuration, -quant-table argument is always added to the cjpeg command. But to avoid breaking things, yes we can log a non-blocking error message. (In reality, running libjpeg-tubro cjpeg with -quant-table fail all the time and does not generate any image, so it's already broken).

I agree to add a check-setup command, that can be ran immediately after the bundle installation. And add to the documentation to run this command on every environment to ensure the presence of the right binary.

I can open a PR for that in coming weeks.

dbu commented 1 year ago

fantastic for the check-setup command :rocket:

if the -quant-table is enabled and calling cjpeg fails, i would continue to throw an exception, but instead of the generic exception it could be a more specific one that points the user into the right direction.

welcoMattic commented 1 year ago

The further I go, the more I wonder if the solution wouldn't be to add libjpeg-turbo support to the bundle? As it is said in MozJPEG Readme:

MozJPEG is a patch for libjpeg-turbo. Please send pull requests to libjpeg-turbo if the changes aren't specific to newly-added MozJPEG-only compression code. This project aims to keep differences with libjpeg-turbo minimal, so whenever possible, improvements and bug fixes should go there first. MozJPEG is meant to be used as a library in graphics programs and image processing tools. We include a demo cjpeg command-line tool, but it's not intended for serious use. We encourage authors of graphics programs to use libjpeg's C API and link with MozJPEG library instead.

I understand that their cjpeg is for demo purpose only, and devs should rely on libjpeg-turbo instead of MozJPEG.

What about adding another post-processor for libjpeg-turbo?

dbu commented 1 year ago

so the mozjpeg patch also adds functionality? does the quant-table option exist in turbo too, but with a different name?

are there other features we rely on that are not provided by turbo library?

if there are not too many missing things, i would agree that a processor for turbo is a good idea. it will mean the user will need to configure which variant they have - but having to choose also prompts people to figure out what they have.