Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11905002:
Add -o option to optimize png file size (Closed)
|
Created:
7 years, 11 months ago by oshima Modified:
7 years, 11 months ago CC:
chromium-reviews, Nico Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd -o option to optimize png file size
The optimize code is based on pngslim. A few steps are omitted, but it runs all other processes. I'll check if it
worth fixing them (for example, advdef didn't make much difference)
Using -o option requires new third party program: optpng, pngout and advdef.
The script asks to install if they're not installed.
So for, I've got ~30% reduction in ash/resources (it's probably because pngslim was never run on this directory), I'd expect less
on other places where pngslim was run before.
Print results
BUG=150046
TEST=none
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177202
Patch Set 1 : #
Total comments: 4
Patch Set 2 : fix typo, added comments #Patch Set 3 : #Patch Set 4 : #
Total comments: 70
Patch Set 5 : fix comments #Patch Set 6 : #Patch Set 7 : #
Total comments: 6
Patch Set 8 : fix comments #Messages
Total messages: 15 (0 generated)
Cool! You're planning to run this on all of chrome's resources right? https://codereview.chromium.org/11905002/diff/3002/build/sanitize-png-files.sh File build/sanitize-png-files.sh (right): https://codereview.chromium.org/11905002/diff/3002/build/sanitize-png-files.s... build/sanitize-png-files.sh:21: # Constnats used for optimization typo Constnats https://codereview.chromium.org/11905002/diff/3002/build/sanitize-png-files.s... build/sanitize-png-files.sh:206: read -p "Couldn't fnd $program. Do you want to install? (y/n)" typo fnd
…and from another tab:
High-level:
This is now looking like a real program. Is bash script still the right language
for this? Now that the apt-get calls are gone, this would run easier on windows
if it was a python script. If you decide to keep bash, please add documents to
all functions, especially the expected parameters (since bash doesn't have real
parameter lists).
Also, can you expand the CL description a bit? Instead of saying "this is like
pngslim", say "this runs a compression pipeline similar to pngslim's: It first
calls pngcrush to remove unneeded file chunks, then pngout with various
color-dependent optimization settings to reduce the color chunk, and finally
optipng for XXX and advdef to optimize the embedded zlib stream" or similiar.
Also mention which new dependencies this adds ("This CL makes this script
dependent on pngout, optipng and advdef in addition to the existing dependency
on pngcrush").
If advdef didn't make much of a difference, consider omitting it (mention that
you tried it in the CL description and quantify the difference. If you keep it,
also say much it helped in the CL description or in a comment).
Yes, i'm going to run this on all files. https://codereview.chromium.org/11905002/diff/3002/build/sanitize-png-files.sh File build/sanitize-png-files.sh (right): https://codereview.chromium.org/11905002/diff/3002/build/sanitize-png-files.s... build/sanitize-png-files.sh:21: # Constnats used for optimization On 2013/01/15 17:23:58, Nico wrote: > typo Constnats Done. https://codereview.chromium.org/11905002/diff/3002/build/sanitize-png-files.s... build/sanitize-png-files.sh:206: read -p "Couldn't fnd $program. Do you want to install? (y/n)" On 2013/01/15 17:23:58, Nico wrote: > typo fnd Done.
On 2013/01/15 17:24:18, Nico wrote:
> …and from another tab:
>
> High-level:
>
> This is now looking like a real program. Is bash script still the right
language
> for this? Now that the apt-get calls are gone, this would run easier on
windows
> if it was a python script. If you decide to keep bash, please add documents to
> all functions, especially the expected parameters (since bash doesn't have
real
> parameter lists).
I'd like to keep it for following reasons:
1) What it's doing is still executing bunch of external program and process
files,
so bash is still good (if not best).
2) apt-get is still there.
3) Given that it takes really long (hours or can take a day), I don't expect
normal
developer would run this script. For the same reason, I don't think we can
and should
automate this in bot. It's (unfortunately) better to run this periodically by
someone
(I'd do that for the time being), probably before branching. If someone wants
to
run this on windows, one still can run it on cygwin.
4) I'm less familiar with python than bash :p
If there is a demand for Windows + python, I'm happy to work on it tho.
mike what do you think?
>
> Also, can you expand the CL description a bit? Instead of saying "this is like
> pngslim", say "this runs a compression pipeline similar to pngslim's: It first
> calls pngcrush to remove unneeded file chunks, then pngout with various
> color-dependent optimization settings to reduce the color chunk, and finally
> optipng for XXX and advdef to optimize the embedded zlib stream" or similiar.
> Also mention which new dependencies this adds ("This CL makes this script
> dependent on pngout, optipng and advdef in addition to the existing dependency
> on pngcrush").
I updated the description about new dependency.
I added comment in the script tho. I think it's better than in the description.
>
> If advdef didn't make much of a difference, consider omitting it (mention that
> you tried it in the CL description and quantify the difference. If you keep
it,
> also say much it helped in the CL description or in a comment).
I'll keep it as it's already there and it runs quickly (compare to others).
Hm, "a day" sounds fairly long. Do you know where most of the time is spent? How much bigger to files get if you just run `pngout -i` (instead of looping through all kinds of block sizes etc), how much faster does run time get?
On 2013/01/15 19:53:27, Nico wrote: > Hm, "a day" sounds fairly long. Do you know where most of the time is spent? How > much bigger to files get if you just run `pngout -i` (instead of looping through > all kinds of block sizes etc), how much faster does run time get? My hope is that we can adjust min/max blocks and num trials for larger images and reduce the execution time, at least to the level that is not painful, and still get reasonably good compression. This is space vs speed, and I need more data to make judgement. (which I'm planning to do as said in TODO). At the end of the day, the goal of the script is to reduce the size of png file as much as possible, so making the execution time short isn't going to be the main goal. In any case, optimizing the optimization process is out of the scope of this CL. I'll revisit the automation issue when we found more about how much we can improve runtime against space.
Ok, lgtm. On Tue, Jan 15, 2013 at 12:49 PM, <oshima@chromium.org> wrote: > On 2013/01/15 19:53:27, Nico wrote: >> >> Hm, "a day" sounds fairly long. Do you know where most of the time is >> spent? > > How >> >> much bigger to files get if you just run `pngout -i` (instead of looping > > through >> >> all kinds of block sizes etc), how much faster does run time get? > > > My hope is that we can adjust min/max blocks and num trials for larger > images > and reduce > the execution time, at least to the level that is not painful, and still get > reasonably > good compression. This is space vs speed, and I need more data to make > judgement. > (which I'm planning to do as said in TODO). At the end of the day, the goal > of > the script > is to reduce the size of png file as much as possible, so making the > execution > time short > isn't going to be the main goal. > > In any case, optimizing the optimization process is out of the scope of this > CL. > I'll revisit the automation issue when we found more about how much we can > improve > runtime against space. > > https://codereview.chromium.org/11905002/
Sorry for all the nits. Also, I'm not a shell scripting guru, so I didn't understand all the code, like some of the control logic in *_if_not_installed. 1) I recommend adding a single pngout call with no (default) options: "pngout -q $file" It's odd in that all of its calls, pngslim never actually tries pngout it with the default options. In particular, running with the default -s0 Strategy and omitting overrides for the -b "block split threshold" and related -n "number of block splits to use" (note that -ks is supplied in other cases, to keep these settings from the input file, or -s# is supplied to override the strategy to something lower than default, most intense, -s0). 2) Similarly, I recommend adding a single optipng call with no (default) options: optipng -q $file Apparently, the default -o <optimization level> varies by version and the docs don't specify an upper bound, so it might also be worth trying an optipng call with just -o 10 or similar (if we try hundreds of pngout runs, 10 optipngs IDAT compression runs might be reasonable). https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.sh File build/sanitize-png-files.sh (right): https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:7: # and executes similar pipleline to optimize the png file size. nit: "a similar" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:8: # The steps that requires pngoptimizercl/pngrewrite/deflopts are omitted, nits: "that require" and "deflopt" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:9: # but this runs allother processes, including: nit: "all other" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:10: # 1) various color-dependent optimization using optpng. nits: "optimizations" "optipng" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:11: # 2) optimize number of huffman blocks. nit: "optimize the" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:12: # 3) randomized huffman table. nit: "randomize the" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:13: # 4) Further optimize using optipng () and advdef (zlib stream). nit: remove "()" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:16: # TOTO(oshima): Use different parameters for large file to reduce nits: "TODO" and "files" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:17: # runtime. nit: this fits on the line above. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:19: # for now as it does take much time to run. nit: Did you mean "doesn't"? Otherwise, this doesn't make sense. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:52: # Usage: process_rgb <file> <png_out_options> ... s/process_rgb/pngout_loop/ https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:53: # Optimize the png file using pngout with given option nit: "the given options" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:54: # using vairous block split threshold and filter types. nits: "various" and "thresholds" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:68: # Optimize grayscale image for all color bit depth. nit: "images" and "depths" here and lines 77 and 87. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:71: for opt in -d1 -d2 -d4 -d8; do Can we use -d0 to minimize the bit depth without looping (here and below)? It's okay if you just want to follow pngslim's example in this first CL, but then maybe add a TODO to experiment with and perhaps adopt -d0 without looping. From: http://advsys.net/ken/util/pngout.htm https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:72: pngout_loop $file -c0 $opt According to the documentation, "If no /c# option is specified, PNGOUT will choose the color type that can most efficiently store the image." So, can we avoid overriding the color type or add a TODO to experiment with that? https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:77: # Optimize grayscale+alpha image for all color bit depth. nit: "(with alpha)", "(w/ alpha)", or similar. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:87: # Optimize rgb (w w/o alpha) image for all color bit depth. nit: "(with or without alpha)", "(w/ or w/o alpha)", or similar. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:98: # Optimize huffman blocks nit: "the huffman", and add a trailing period. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:113: # Usage: random_huffmantable_trial <file> nit: random_huffman_table https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:114: # Try compressing using randomized initial huffman table. nit: "a randomized" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:115: function random_huffmantable_trial { Wow! This has got to be why this script is so slow. -s0 is the "Xtreme! (Slowest)" strategy, but here is run 100 times, and so long as that produces a smaller file, it's run another 100 times. I feel like we could probably devise a more prudent algorithm/heuristic if we roughly understood the variability in the results. Possibly add a TODO here to that effect? https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:121: pngout -q -k -ks -s0 -r $file This -k option should be -k1 if you're following pngslim; -k doesn't appear to have any meaning without a number from the doc. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:130: # Further compreess using optpng and advdef. nits: "compress" and "optipng" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:134: for i in 32k 16k 8k 4k 2k 1k 512; do Should we also try 256 here? I guess pngslim doesn't. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:136: optipng -q -nb -nc -zw$i -zc1-9 -zm1-9 -zs0-3 -f0-5 $file optipng docs claim that quiet mode is -quiet, not -q: http://optipng.sourceforge.net/optipng-0.7.4.man.pdf But I guess pngslim uses -q, and if it works, it's fine. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:140: advdef -q -z -$i $file Strange, your code looks correct according to advdef docs: http://advancemame.sourceforge.net/doc-advdef.html But pngslim seems to use (incorrect?) "-z$i" instead of "-z -$i". https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:149: echo -n "$file " Does this work? (using $file before it's defined in the next line) https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:176: for i in 3 2 0; do Should we also run Strategy 1 here? I guess pngslim doesn't. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:185: # Usage: process_fie <file> nit: "process_file" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:259: unnecessary chunks and compress the image. nit: "compressing" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:262: -o Agressively optimize file size. Warnig: this is *VERY* slow and nits: "Aggressively" and "Warning" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:299: # If no directories specified, sanitize all directories. nit: "directories are specified" https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:309: # Print the results nit: Add a trailing period. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:314: echo "Result : $TOTAL_OLD_BYTES => $TOTAL_NEW_BYTES" \ nit: Specify the units like "$TOTAL_OLD_BYTES bytes"
I run pngout and optipng with default parameter on files that are already processed but it made no improvement. I also run small experiment with optpng -d0. It did make very small difference (<1%) with about 10% runtime increase. I'll keep it as is for now as I want to keep it same as pngslim as starting point. I'll run more thorough test and follow up with your guys. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.sh File build/sanitize-png-files.sh (right): https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:7: # and executes similar pipleline to optimize the png file size. On 2013/01/16 03:43:11, msw wrote: > nit: "a similar" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:8: # The steps that requires pngoptimizercl/pngrewrite/deflopts are omitted, On 2013/01/16 03:43:11, msw wrote: > nits: "that require" and "deflopt" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:9: # but this runs allother processes, including: On 2013/01/16 03:43:11, msw wrote: > nit: "all other" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:10: # 1) various color-dependent optimization using optpng. On 2013/01/16 03:43:11, msw wrote: > nits: "optimizations" "optipng" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:11: # 2) optimize number of huffman blocks. On 2013/01/16 03:43:11, msw wrote: > nit: "optimize the" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:12: # 3) randomized huffman table. On 2013/01/16 03:43:11, msw wrote: > nit: "randomize the" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:13: # 4) Further optimize using optipng () and advdef (zlib stream). On 2013/01/16 03:43:11, msw wrote: > nit: remove "()" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:16: # TOTO(oshima): Use different parameters for large file to reduce On 2013/01/16 03:43:11, msw wrote: > nits: "TODO" and "files" Done. (moved to random_huffman_table_trial) https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:17: # runtime. On 2013/01/16 03:43:11, msw wrote: > nit: this fits on the line above. Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:19: # for now as it does take much time to run. On 2013/01/16 03:43:11, msw wrote: > nit: Did you mean "doesn't"? Otherwise, this doesn't make sense. Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:52: # Usage: process_rgb <file> <png_out_options> ... On 2013/01/16 03:43:11, msw wrote: > s/process_rgb/pngout_loop/ Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:53: # Optimize the png file using pngout with given option On 2013/01/16 03:43:11, msw wrote: > nit: "the given options" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:54: # using vairous block split threshold and filter types. On 2013/01/16 03:43:11, msw wrote: > nits: "various" and "thresholds" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:68: # Optimize grayscale image for all color bit depth. On 2013/01/16 03:43:11, msw wrote: > nit: "images" and "depths" here and lines 77 and 87. Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:71: for opt in -d1 -d2 -d4 -d8; do On 2013/01/16 03:43:11, msw wrote: > Can we use -d0 to minimize the bit depth without looping (here and below)? > It's okay if you just want to follow pngslim's example in this first CL, but > then maybe add a TODO to experiment with and perhaps adopt -d0 without looping. > From: http://advsys.net/ken/util/pngout.htm I added TODO. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:72: pngout_loop $file -c0 $opt On 2013/01/16 03:43:11, msw wrote: > According to the documentation, "If no /c# option is specified, PNGOUT will > choose the color type that can most efficiently store the image." So, can we > avoid overriding the color type or add a TODO to experiment with that? added TODO https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:77: # Optimize grayscale+alpha image for all color bit depth. On 2013/01/16 03:43:11, msw wrote: > nit: "(with alpha)", "(w/ alpha)", or similar. Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:87: # Optimize rgb (w w/o alpha) image for all color bit depth. On 2013/01/16 03:43:11, msw wrote: > nit: "(with or without alpha)", "(w/ or w/o alpha)", or similar. Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:98: # Optimize huffman blocks On 2013/01/16 03:43:11, msw wrote: > nit: "the huffman", and add a trailing period. Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:113: # Usage: random_huffmantable_trial <file> On 2013/01/16 03:43:11, msw wrote: > nit: random_huffman_table Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:114: # Try compressing using randomized initial huffman table. On 2013/01/16 03:43:11, msw wrote: > nit: "a randomized" changed to Try compressing by randomizing initial huffman table. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:115: function random_huffmantable_trial { On 2013/01/16 03:43:11, msw wrote: > Wow! This has got to be why this script is so slow. -s0 is the "Xtreme! > (Slowest)" strategy, but here is run 100 times, and so long as that produces a > smaller file, it's run another 100 times. I feel like we could probably devise a > more prudent algorithm/heuristic if we roughly understood the variability in the > results. Possibly add a TODO here to that effect? what makes it really slow is that pngout takes really long to process large files. I guess it has O(n^2) in it. And yes, I'm going to do some experiment to reduce the number of loops. I moved TODO from top to this function's comment. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:121: pngout -q -k -ks -s0 -r $file On 2013/01/16 03:43:11, msw wrote: > This -k option should be -k1 if you're following pngslim; -k doesn't appear to > have any meaning without a number from the doc. thank you for the catch! Fixed. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:130: # Further compreess using optpng and advdef. On 2013/01/16 03:43:11, msw wrote: > nits: "compress" and "optipng" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:134: for i in 32k 16k 8k 4k 2k 1k 512; do On 2013/01/16 03:43:11, msw wrote: > Should we also try 256 here? I guess pngslim doesn't. I'll try. I added TODO https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:136: optipng -q -nb -nc -zw$i -zc1-9 -zm1-9 -zs0-3 -f0-5 $file On 2013/01/16 03:43:11, msw wrote: > optipng docs claim that quiet mode is -quiet, not -q: > http://optipng.sourceforge.net/optipng-0.7.4.man.pdf > But I guess pngslim uses -q, and if it works, it's fine. yes, it -q does work. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:140: advdef -q -z -$i $file On 2013/01/16 03:43:11, msw wrote: > Strange, your code looks correct according to advdef docs: > http://advancemame.sourceforge.net/doc-advdef.html > But pngslim seems to use (incorrect?) "-z$i" instead of "-z -$i". yes, I looked at the doc and fixed. I guess both works. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:149: echo -n "$file " On 2013/01/16 03:43:11, msw wrote: > Does this work? (using $file before it's defined in the next line) thank you for the catch. Fixed. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:176: for i in 3 2 0; do On 2013/01/16 03:43:11, msw wrote: > Should we also run Strategy 1 here? I guess pngslim doesn't. Added TODO https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:185: # Usage: process_fie <file> On 2013/01/16 03:43:11, msw wrote: > nit: "process_file" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:259: unnecessary chunks and compress the image. On 2013/01/16 03:43:11, msw wrote: > nit: "compressing" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:262: -o Agressively optimize file size. Warnig: this is *VERY* slow and On 2013/01/16 03:43:11, msw wrote: > nits: "Aggressively" and "Warning" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:299: # If no directories specified, sanitize all directories. On 2013/01/16 03:43:11, msw wrote: > nit: "directories are specified" Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:309: # Print the results On 2013/01/16 03:43:11, msw wrote: > nit: Add a trailing period. Done. https://codereview.chromium.org/11905002/diff/13001/build/sanitize-png-files.... build/sanitize-png-files.sh:314: echo "Result : $TOTAL_OLD_BYTES => $TOTAL_NEW_BYTES" \ On 2013/01/16 03:43:11, msw wrote: > nit: Specify the units like "$TOTAL_OLD_BYTES bytes" Done.
by the way, I'm also working on a script to download png file directory from google drive and it looks promising. If it really works, I'll port this to python and we can run this when it download files.
Great; LGTM with optional grammar nits, thanks! https://codereview.chromium.org/11905002/diff/22001/build/sanitize-png-files.sh File build/sanitize-png-files.sh (right): https://codereview.chromium.org/11905002/diff/22001/build/sanitize-png-files.... build/sanitize-png-files.sh:11: # 2) optimize number of the huffman blocks. nit: "optimize the number of huffman blocks." ('the' is in the wrong spot) https://codereview.chromium.org/11905002/diff/22001/build/sanitize-png-files.... build/sanitize-png-files.sh:12: # 3) randomize huffman table. nit: "randomize the" https://codereview.chromium.org/11905002/diff/22001/build/sanitize-png-files.... build/sanitize-png-files.sh:114: # Try compressing by randomizing initial huffman table. nit: "randomizing the"
https://codereview.chromium.org/11905002/diff/22001/build/sanitize-png-files.sh File build/sanitize-png-files.sh (right): https://codereview.chromium.org/11905002/diff/22001/build/sanitize-png-files.... build/sanitize-png-files.sh:11: # 2) optimize number of the huffman blocks. On 2013/01/16 19:21:40, msw wrote: > nit: "optimize the number of huffman blocks." ('the' is in the wrong spot) Done. https://codereview.chromium.org/11905002/diff/22001/build/sanitize-png-files.... build/sanitize-png-files.sh:12: # 3) randomize huffman table. On 2013/01/16 19:21:40, msw wrote: > nit: "randomize the" Done. https://codereview.chromium.org/11905002/diff/22001/build/sanitize-png-files.... build/sanitize-png-files.sh:114: # Try compressing by randomizing initial huffman table. On 2013/01/16 19:21:40, msw wrote: > nit: "randomizing the" Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/11905002/24001
Message was sent while issue was closed.
Change committed as 177202 |
