chrissimpkins / Crunch

Insane(ly slow but wicked good) PNG image optimization
Other
3.36k stars 147 forks source link

Doesnt work if spawned from another process #68

Closed milewski closed 6 years ago

milewski commented 6 years ago

When i spawn Crunch from any other process... e.g, node, php, go... the compression ratio is always on 100%

"""
Crunching ...\n
[ 100.00% ] fafdae7fd865c9ea6480e5d5037fd69d-crunch.png (943686 bytes)\n
"""

running the same process manually on bash/sh crunch fafdae7fd865c9ea6480e5d5037fd69d.png gives me

Crunching ...
[ 33.68% ] fafdae7fd865c9ea6480e5d5037fd69d-crunch.png (317815 bytes)

reproducible command

php -r 'shell_exec("crunch path/to/a/png/file.png");'
milewski commented 6 years ago

Well i figured out the reason, im running the process with a different user www-data against root however allowing the user www-data to execute python, crunch, pngquant, zopflipng + full access to the folder it is outputting to... + read on the input file.. doesn't seems to work... the process still outputs [ 100.00% ] no compression, no errors are thrown and process exist with code 0

did i miss something?

chrissimpkins commented 6 years ago

Strange. I don’t know what is going on there. Any reason that you can’t call the script with python directly rather than call it through a sub process in a different language? The ‘crunch’ executable is a Python script. If you must execute through a non-Python language it should be fairly simple to re-write the execution directly in one of those languages. There is already a JS implementation if that happens to be helpful to you.

milewski commented 6 years ago

If I login to my system as the user www-data or any other user really without root access and try to invoke crunch img, python path/to/crunch img I got the same output as the input..

For my use case I have a queue, that spawn crunch instances.. However the queue itself runs as non root account and I can't change that... As far as I know I see that I'm able to execute python and run crunch script with any user... The problem might be on pngquant or zopli not being able to access some system resource under a different account ...

chrissimpkins commented 6 years ago

I think that you are correct. The stdout reporting with the % file size calculations are coming from the Python scripting. The Python source is able to read the target file(s) to calculate the file size. For some reason, it appears that optimizations with pngquant and zopflipng are not being performed and the file size remains at 100% original size. Error messages (and non-zero exit status codes) should be propagated from those tools, but that is not happening which is odd.

1) Let's start with the simplest solution so that we've addressed it. Please confirm that the files have not been previously optimized with pngquant / zopflipng. If they have, clearly there is likely to be no further optimization and the script should appropriately print that the ratio is 100%. I think that this is what you are indicating when you execute the script directly on the command line and see a reduction to ~33% original file size but let's take this off of the table as a possibility.

2) Can you please confirm that you used the makefile to install the off system PATH versions of pngquant and zopflipng? These are expected on the paths ~/pngquant/pngquant and ~/zopfli/zopflipng, respectively.

3) Will you please confirm that you tested this with a single file approach? We may be missing error messages/exit status codes if you are batch processing multiple files with the parallel processing approach that is used. Let me know if there is still no error message with single file processing.

4) Can you confirm that the following also fails execution with the pngquant/zopflipng tools when executed with a working directory that includes the crunch.py source file on the repository path master/src/crunch.py?

php -r 'shell_exec("python crunch.py path/to/a/png/file.png");'
milewski commented 6 years ago
  1. The image is originally an output of some transformation made by GD, so there is no previously compression being made on this image.

  2. Im building from source using this instructions:

git clone https://github.com/chrissimpkins/Crunch.git --depth 1 && cd Crunch && \
make build-dependencies && make install-executable

if i remove ~/pngquant/pngquant and ~/zopfli/zopflipng i get [ERROR] pngquant executable was not identified on path '/anyone/pngquant/pngquant' so i believe crunch is picking up the correct binaries.

  1. Yes im using one single . png file to run these tests.

  2. the following all fail under a non root account for me:

php -r 'shell_exec("python /usr/local/bin/crunch ZzQou1dwjsn4mGoIbgjKFL6THz8rUnpgl0rWpEd.png");'

php -r 'shell_exec("/usr/bin/python /usr/local/bin/crunch ZzQou1dwjsn4mGoIbgjKFL6THz8rUnpgl0rWpEd.png");'

// cp  /usr/local/bin/crunch crunch.py
php -r 'shell_exec("./crunch.py 7ZzQou1dwjsn4mGoIbgjKFL6THz8rUnpgl0rWpEd.png");'

php -r 'shell_exec("/usr/bin/python ./crunch.py 7ZzQou1dwjsn4mGoIbgjKFL6THz8rUnpgl0rWpEd.png");'

all had exit code 0 but with [ 100.00% ] compression, switching account back to root all the commands succeed with output:

Crunching ...
[ 22.14% ] 7ZzQou1dwjsn4mGoIbgjKFL6THz8rUnpgl0rWpEd-crunch.png (251325 bytes)
milewski commented 6 years ago

ok i think i got it figured out now..

when i first install the crunch with this command:

git clone https://github.com/chrissimpkins/Crunch.git --depth 1 && cd Crunch && \
make build-dependencies && make install-executable

im running as root, therefore the ~/pngquant/pngquant and ~/zopfli/zopflipng points out to /home/root/pngquant/pngquant and /home/root/zopfli/zopflipng, when i login with a different user, my home directory becomes /home/another-user/ so i copy the files from /home/root/* to /home/another-user/ then it doesnt work....(running ./pngquant and ./zopflipng manually works) however if i first install it to a different directory like this:

mkdir /data
git clone https://github.com/chrissimpkins/Crunch.git --depth 1 
cd Crunch
HOME=/data make build-dependencies && make install-executable

crunch will will install the dependencies inside /data/pngquant/pngquant and /data/zopfli/zopflipng, so when i login back as another-user i run crunch like this

HOME=/data crunch img.png
HOME=/data php -r 'shell_exec("crunch img.png");'

and it works :D

chrissimpkins commented 6 years ago

Excellent! Do you think that this is a common enough need to warrant placing your approach in the documentation?

milewski commented 6 years ago

i think the ultimate solution to avoid this problem in the future would be not installing the pngquant and zopflipng to the user ~/ home dir by default.. cuz this is very dependent on who is currently logged in.. + file permissions to access, different users will have different home directories, and as crunch is installed globally it wont be able to access the home folder of other users... perhaps move pngquant/zopflipng to /usr/bin after compilation and expects that they are available globally instead of at a specific path?

chrissimpkins commented 6 years ago

perhaps move pngquant/zopflipng to /usr/bin after compilation

The issue with this approach is that it could overwrite a system/package manager/manual install of both project dependencies. This was an intentional decision because:

milewski commented 6 years ago

uhm, understood, yeah in that case i think would be worth mentioning on the docs that the installation path can be modified by setting the HOME env to something else, although it was explicitly written $HOME/pngquant/pngquant i didn't realized that was actually a env var until i read the source.

chrissimpkins commented 6 years ago

Will update the docs to see if I can clarify this issue. Thanks for helping to get to the bottom of it. I appreciate all of your feedback here!

chrissimpkins commented 6 years ago

Note too that you can specify the path to the pngquant and zopflipng executables in these lines of the script:

https://github.com/chrissimpkins/Crunch/blob/e0bca6732bb8587b0732d45b7cdcf72097603958/src/crunch.py#L38-L39

It is not necessary to use os.path.join if you are using this on a known single platform, just replace the entire os.path.join(...) statement with a string that is the filepath to these executables.

PNGQUANT_CLI_PATH = "path/to/file" 
ZOPFLIPNG_CLI_PATH = "path/to/file"

The Python os.path.join code there makes the paths platform independent. This should eliminate the need for you to set environment variables. Sorry for the confusion!

milewski commented 6 years ago

uhm but that would require having a fork of this repo which potently could lead me for missing further updates or writing a script to find/replace on the crunch.py, currently for my use case im building it inside a docker container.. could that PNGQUANT_CLI_PATH and ZOPFLIPNG_CLI_PATH be taken from ENV something like

PNGQUANT_CLI_PATH = env.PNGQUANT_CLI_PATH || os.path.join(os.path.expanduser("~"), "pngquant", "pngquant") (sorry i dont know python)

chrissimpkins commented 6 years ago

Added documentation in the Install section of the docs/EXECUTABLE.md document in https://github.com/chrissimpkins/Crunch/commit/ee00d14be24f13e0339637aff6e4cb17c1b54f42

I am definitely open to considering environment variable support for installations and execution. Mind opening a new issue report with details on how the installation and execution should be modified to support your use case (or a PR with the proposed changes)?

Thanks again for your help here!