Created attachment 70716 [details]
diff of the script

Just as a thought it is easier to review using the web-based ReviewBoard tool at https://svn.reviewboard.kde.org/ (but you'd need to make an account there). I tried adding it for you but it requires an svn-format diff (which is easy enough) and you'd need to have an account.

With that out of the way, my "big picture" thoughts:

1. I don't think it makes sense to *require* that all of those optimizing tools exist. Before we only needed 2 so it was OK (because if you only had 1 it was easy enough to skip using the script entirely).

This should be fixed by running the tool only if it exists. The "which" command should be used for this.

e.g.
if which optipng; then
    optipng ...
fi

Or if you'd like you could check once right at the beginning and if the tool isn't present just replace it with the "false" command

optipng_tool=$(which optipng)
optipng_tool=${optipng_tool:-false} # Sets optipng_tool to false if not set yet

and later...

${optipng_tool} ... # if not present just does nothing

2. Ideally CPU# detection would by adjustable by the user at the command line, but either way it should be checked to see if it has a valid value and set to 1 if it doesn't.

3. "If [FILE] is not defined, it optimizes all files (PNG.MNG,SVGZ,JPG)

There should be a comma between PNG and MNG.

4. At line 62 of the diff there is a commented find command, which shouldn't be included in the final code.

5. For adding parallelization you convert from a find command to a find->xargs pipeline, which works well. But, you should use the -print0 command for find and the -0 option for xargs to ensure that "weird" filename to not cause problems with the pipeline. Those two options are GNU extensions but they are supported on FreeBSD at least, possibly Mac OS as well (and it's far easier than doing it using just POSIX)

e.g. find . -print0 -name '*.png' | xargs -0 --max-args=1 --max-procs=$cpus $optipng_tool ...

6. Why is there a different compression level used with optiPNG on a single file compared to optiPNG for recursive search? It was actually like this before but the difference is even more dramatic now.

> +    echo "optimizegraphics: Losslessly optimized PNG, MNG, JPEG and SVGZ files with \"optipng -o4\" \"advdef -z -4\" \"pngout -ks\" and jpgcrush."

7. It's probably better just to not mention the specific tools used at this point, now that so many are supported. ;)

SVN commit 1332557 by mpyne:

optimizegraphics: Add support for pngout and AdvanceCOMP.

The work and testing leading to this patch were created and graciously improved
after review by Bruno George Moraes. It adds several things:

- Usage of the pngout tool, written by the author of the Duke Nukem 3D game
  engine. (pngout is free to download the binary of, but is non-Free software).
- Usage of the AdvanceCOMP tool, which is similar to OptiPNG, but uses the 7z
  library (so it optimizes not just PNG but also things like .svgz and .mng).
- Parallel execution of these tools to recursively optimize a subdirectory of
  images.

I've further improved it to meet coding style, remove a needless bash-ism, fix
some of the math code, and quiet the output a bit. Additionally it will use any
tools you have available instead of requiring you to run non-Free software to
work at all.

With parallel execution it is important to limit the number of simultaneous
processes xargs will run, we try grepping in /proc/cpu using Bruno's code, but
I limit it further to 4 tasks if that fails.

Thanks to Bruno for the patch and his patience, I figure this will probably be
one of the last SVN commits to kdesdk ever before we convert to git.

REVIEW:7000
FIXED-IN:4.11


 M  +71 -21    optimizegraphics  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1332557