Introduces optimization level 1/2

Introduces optimization level 1/2
 default (=1) does less optimization but runs fast
 -o (=2) does more optimization but slow.
 -n just run pngcrush and skip other optimizations

default produces good enough results and runs a lot faster,
fast enough so that we can now ask people to run.

Changed to show the results for changed files only

NOTRY=true
BUG=150046
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180254

[msw, 2013-02-01 20:17:19]

Thanks for working on this and putting up with my comments/nits!

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files.sh
File build/sanitize-png-files.sh (right):

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:30: # The file larger than this file size (in bytes)
will
nit: s/The file/files/ and s/the large file/large files/

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:32: LARGE_FILE_THREASHOLD=3000
nit: no 'a' in "THRESHOLD"

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:66: for j in $(seq 0 5); do
optional nits: move the common j loop outside the OPTIMIZE_LEVEL check, and
rename i/j to match their parameters b/f

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:135: let max_blocks=$size/$min_block_size
question: why 'let' versus 'local' (here and elsewhere)?
Should 'lowercase_locals' be declared local, and 'UPPERCASE_WIDER_SCOPE_VALUES'
be set/declared with let?

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:155: local n_trials=$DEFAULT_RANDOM_TRIALS
nit: num_trials or trial_count

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:266: let TOTAL_OLD_BYTES+=$old
It seems like you should still add $old to TOTAL_OLD_BYTES, then add either $old
or $new to TOTAL_NEW_BYTES, depending on the outcome of the optimization.

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:270: if [ $OPTIMIZE_LEVEL != 0 ]; then
Hmm, you converted this line and line 243 (a negation of this check), to the
same new check; that doesn't seem right.

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:313: -n  Run only pngcrush and skip other
optimizations.
Does it make more sense to just have a single option -o that defaults to 1, but
can take values 0/1/2 and get rid of -n ? Otherwise, perhaps document that
running without -n nor -o produces the decent results quickly?

[oshima, 2013-02-01 23:55:25]

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files.sh
File build/sanitize-png-files.sh (right):

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:30: # The file larger than this file size (in bytes)
will
On 2013/02/01 20:17:19, msw wrote:
> nit: s/The file/files/ and s/the large file/large files/

Done.

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:32: LARGE_FILE_THREASHOLD=3000
On 2013/02/01 20:17:19, msw wrote:
> nit: no 'a' in "THRESHOLD"

Done.

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:66: for j in $(seq 0 5); do
On 2013/02/01 20:17:19, msw wrote:
> optional nits: move the common j loop outside the OPTIMIZE_LEVEL check, and
> rename i/j to match their parameters b/f

I wanted to keep the order that each options are applied as the order
seems to be important.

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:135: let max_blocks=$size/$min_block_size
On 2013/02/01 20:17:19, msw wrote:
> question: why 'let' versus 'local' (here and elsewhere)?
> Should 'lowercase_locals' be declared local, and
'UPPERCASE_WIDER_SCOPE_VALUES'
> be set/declared with let?

local creates a local variable while let is a set that understand arithmetic
expression.

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:155: local n_trials=$DEFAULT_RANDOM_TRIALS
On 2013/02/01 20:17:19, msw wrote:
> nit: num_trials or trial_count

Done.

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:266: let TOTAL_OLD_BYTES+=$old
On 2013/02/01 20:17:19, msw wrote:
> It seems like you should still add $old to TOTAL_OLD_BYTES, then add either
$old
> or $new to TOTAL_NEW_BYTES, depending on the outcome of the optimization.

I wanted to show the results for the processed files. I think
it's more informative than including everything that weren't processed. I can
show both tho. Please let me know.

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:270: if [ $OPTIMIZE_LEVEL != 0 ]; then
On 2013/02/01 20:17:19, msw wrote:
> Hmm, you converted this line and line 243 (a negation of this check), to the
> same new check; that doesn't seem right.

Thank you for the catch. Fixed.

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:313: -n  Run only pngcrush and skip other
optimizations.
On 2013/02/01 20:17:19, msw wrote:
> Does it make more sense to just have a single option -o that defaults to 1,
but
> can take values 0/1/2 and get rid of -n ? Otherwise, perhaps document that
> running without -n nor -o produces the decent results quickly?

changed to accept level. It was a bit tricky to handle arguments with getopts,
but I agree that this is better.

[msw, 2013-02-02 01:58:01]

LGTM with nits; thanks!

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files.sh
File build/sanitize-png-files.sh (right):

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:66: for j in $(seq 0 5); do
On 2013/02/01 23:55:25, oshima wrote:
> On 2013/02/01 20:17:19, msw wrote:
> > optional nits: move the common j loop outside the OPTIMIZE_LEVEL check, and
> > rename i/j to match their parameters b/f
> 
> I wanted to keep the order that each options are applied as the order
> seems to be important.

Ok, cool.

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:135: let max_blocks=$size/$min_block_size
On 2013/02/01 23:55:25, oshima wrote:
> On 2013/02/01 20:17:19, msw wrote:
> > question: why 'let' versus 'local' (here and elsewhere)?
> > Should 'lowercase_locals' be declared local, and
> 'UPPERCASE_WIDER_SCOPE_VALUES'
> > be set/declared with let?
> 
> local creates a local variable while let is a set that understand arithmetic
> expression.

Ok, cool; thx for the info.

https://codereview.chromium.org/12095097/diff/11001/build/sanitize-png-files....
build/sanitize-png-files.sh:266: let TOTAL_OLD_BYTES+=$old
On 2013/02/01 23:55:25, oshima wrote:
> On 2013/02/01 20:17:19, msw wrote:
> > It seems like you should still add $old to TOTAL_OLD_BYTES, then add either
> $old
> > or $new to TOTAL_NEW_BYTES, depending on the outcome of the optimization.
> 
> I wanted to show the results for the processed files. I think
> it's more informative than including everything that weren't processed. I can
> show both tho. Please let me know.

Okay, I think it makes sense as-is, since it mentions the PROCESSED_FILE count.

https://codereview.chromium.org/12095097/diff/17017/build/sanitize-png-files.sh
File build/sanitize-png-files.sh (right):

https://codereview.chromium.org/12095097/diff/17017/build/sanitize-png-files....
build/sanitize-png-files.sh:313: -o<optimize_level>  Specify optimization level:
optional nit: Add "(default is 1)" here to be extra clear (I see it w/option 1).

https://codereview.chromium.org/12095097/diff/17017/build/sanitize-png-files....
build/sanitize-png-files.sh:318: 2  Aggressively optimize the size of png files.
This may produces
nit: s/produces/produce/

https://codereview.chromium.org/12095097/diff/17017/build/sanitize-png-files....
build/sanitize-png-files.sh:336: if [[ ! "$OPTARG" =~ [012] ]]; then
no kidding, this is complex; it looks reasonable, I'll just assume it works :)

[oshima, 2013-02-02 07:57:17]

https://codereview.chromium.org/12095097/diff/17017/build/sanitize-png-files.sh
File build/sanitize-png-files.sh (right):

https://codereview.chromium.org/12095097/diff/17017/build/sanitize-png-files....
build/sanitize-png-files.sh:313: -o<optimize_level>  Specify optimization level:
On 2013/02/02 01:58:02, msw wrote:
> optional nit: Add "(default is 1)" here to be extra clear (I see it w/option
1).

Done.

https://codereview.chromium.org/12095097/diff/17017/build/sanitize-png-files....
build/sanitize-png-files.sh:318: 2  Aggressively optimize the size of png files.
This may produces
On 2013/02/02 01:58:02, msw wrote:
> nit: s/produces/produce/

Done.

https://codereview.chromium.org/12095097/diff/17017/build/sanitize-png-files....
build/sanitize-png-files.sh:336: if [[ ! "$OPTARG" =~ [012] ]]; then
On 2013/02/02 01:58:02, msw wrote:
> no kidding, this is complex; it looks reasonable, I'll just assume it works :)

Yeah, I'm amazed by what bash is capable of (putting aside that it looks odd :)

[commit-bot: I haz the power, 2013-02-02 07:58:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/12095097/42002

[commit-bot: I haz the power, 2013-02-02 08:26:59]

Change committed as 180254