I've managed to get an economy of 31.2% with lossless optimization (aka no quality loss) on http://kraken.io.

I've made sure that APNG files (syncProgress-*) didn't get changed, since kraken.io didn't support APNG.

I don't have time to review this until tomorrow, but this really needs a try push with a baseline, with talos checks to ensure we don't regress perf (which is a known issue when doing very strong compression. I'd also be interested to know how kraken.io compares to imageoptim.app and/or what tools they use internally. We should ideally fix this some better way.

Try : https://tbpl.mozilla.org/?tree=Try&rev=fe51145e343a I'm not sure of what kraken.io or imageoptim.app use internally, but kraken.io has been giving me the best compression rates so far (for lossless compression). Brian Grinstead suggested in bug 1062991#c5 to periodically run an automatic task (grunt for example), or do the image optimization work during our build process.

(In reply to Tim Nguyen [:ntim] from comment #3) > Try : https://tbpl.mozilla.org/?tree=Try&rev=fe51145e343a This has random other patches in the try push, and no baseline... so we can't compare the talos results against anything else. :-\

Plain try push : https://tbpl.mozilla.org/?tree=Try&rev=b5ffc834605e Try push with patch applied : https://tbpl.mozilla.org/?tree=Try&rev=186a956764bd

Newer try pushes : Plain : https://tbpl.mozilla.org/?tree=Try&rev=02ba3db33daa With patch : https://tbpl.mozilla.org/?tree=Try&rev=b64fe1e17c39

(In reply to Tim Nguyen [:ntim] from comment #6) > Newer try pushes : > > Plain : https://tbpl.mozilla.org/?tree=Try&rev=02ba3db33daa > > With patch : https://tbpl.mozilla.org/?tree=Try&rev=b64fe1e17c39 The tpaint/ts_paint sections in http://compare-talos.mattn.ca/?oldRevs=02ba3db33daa&newRev=b64fe1e17c39&server=graphs.mozilla.org&submit=true look like a wash, so we should be good there...

Comment on attachment 8484305 [details] [diff] [review] Patch v1 As best I can tell this is sane - though it took a while; reviewing image patches is hard. :-( Here's what I found to be a useful trick, for posterity: gkruitbosch-16516:fx-team gkruitbosch$ cat compare-image.sh #!/bin/sh echo $1 convert $1 txt:- | cut -f4 -d' ' | grep -v '^......00$' > /tmp/new-icon.txt convert "../mozilla-inbound/$1" txt:- | cut -f4 -d' ' | grep -v '^......00$' > /tmp/old-icon.txt diff -wpU6 /tmp/old-icon.txt /tmp/new-icon.txt I ran this against images in an fx-team tree with patch, with a sibling m-i tree without patch, where there hadn't been any other changes to the files, and using beyondcompare to filter out only images which had materially changed - the grep call is used to filter out fully transparent pixels which beyondcompare otherwise reports as being different. The convert (imagemagick) calls create a pixel-by-pixel output of the image contents, and the cut calls select just the #RRGGBB(AA)? column in the output.

(In reply to :Gijs Kruitbosch from comment #8) Thanks for the review and tip :)

(In reply to :Gijs Kruitbosch from comment #7) > (In reply to Tim Nguyen [:ntim] from comment #6) > > Newer try pushes : > > > > Plain : https://tbpl.mozilla.org/?tree=Try&rev=02ba3db33daa > > > > With patch : https://tbpl.mozilla.org/?tree=Try&rev=b64fe1e17c39 > > The tpaint/ts_paint sections in > http://compare-talos.mattn.ca/ > ?oldRevs=02ba3db33daa&newRev=b64fe1e17c39&server=graphs.mozilla. > org&submit=true look like a wash, so we should be good there... How should I read those comparisons ? Is less better, or more better ?

(In reply to Tim Nguyen [:ntim] from comment #10) > (In reply to :Gijs Kruitbosch from comment #7) > > (In reply to Tim Nguyen [:ntim] from comment #6) > > > Newer try pushes : > > > > > > Plain : https://tbpl.mozilla.org/?tree=Try&rev=02ba3db33daa > > > > > > With patch : https://tbpl.mozilla.org/?tree=Try&rev=b64fe1e17c39 > > > > The tpaint/ts_paint sections in > > http://compare-talos.mattn.ca/ > > ?oldRevs=02ba3db33daa&newRev=b64fe1e17c39&server=graphs.mozilla. > > org&submit=true look like a wash, so we should be good there... > > How should I read those comparisons ? Is less better, or more better ? The "oldRevs" is the try push without the patch, and the "newRev" is the one with the patch, and so negative numbers is good, and positive numbers is bad (in the comparison column). ts_paint and tpaint both output a number in ms, which is how long it took to open a new browser or a new window, respectively.

(In reply to :Gijs Kruitbosch from comment #8) > As best I can tell this is sane - though it took a while; reviewing image > patches is hard. :-( This is why I'm not thrilled to be taking giant one-shot binary changes (see also bug 631392 comment 11). I'd _much_ rather see someone work on getting an automated (or at least reproducible) mechanism implemented for image optimization. > and using beyondcompare to filter out only images which had materially > changed - the grep call is used to filter out fully transparent pixels which > beyondcompare otherwise reports as being different. The convert > (imagemagick) calls create a pixel-by-pixel output of the image contents, > and the cut calls select just the #RRGGBB(AA)? column in the output. I feel like I'm missing something here. Your script seems to be generating ascii pixel values and comparing old/new -- I assume the result of this patch is that the image pixels values are unchanged? What was the role of beyondcompare, then?

(In reply to Justin Dolske [:Dolske] from comment #12) > > and using beyondcompare to filter out only images which had materially > > changed - the grep call is used to filter out fully transparent pixels which > > beyondcompare otherwise reports as being different. The convert > > (imagemagick) calls create a pixel-by-pixel output of the image contents, > > and the cut calls select just the #RRGGBB(AA)? column in the output. > > I feel like I'm missing something here. Your script seems to be generating > ascii pixel values and comparing old/new -- I assume the result of this > patch is that the image pixels values are unchanged? Yes > What was the role of > beyondcompare, then? Well, I tried to use that to do the same, and found that it didn't like the patch changing: rgba(0,0,0,0) to rgba(255,255,255,0) and similar. There were only a few images that fell in this category, and so I used the script to check those. After I had the script, I guess I could have used it against all the images (with some more bash work), but considering beyondcompare said they were the same, I figured that was unnecessary.

(In reply to Justin Dolske [:Dolske] from comment #12) > (In reply to :Gijs Kruitbosch from comment #8) > > > As best I can tell this is sane - though it took a while; reviewing image > > patches is hard. :-( > > This is why I'm not thrilled to be taking giant one-shot binary changes (see > also bug 631392 comment 11). I'd _much_ rather see someone work on getting > an automated (or at least reproducible) mechanism implemented for image > optimization. I agree (see also: comment #2 / comment #3) but I also don't want to leave a 30% win lying around because we should have a better solution still.

(In reply to :Gijs Kruitbosch from comment #14) > > I agree (see also: comment #2 / comment #3) but I also don't want to leave a > 30% win lying around because we should have a better solution still. Hey Guys, given the discussion in the last 3 comments is still patch still ready for checkin ?

(In reply to Carsten Book [:Tomcat] from comment #15) > (In reply to :Gijs Kruitbosch from comment #14) > > > > > I agree (see also: comment #2 / comment #3) but I also don't want to leave a > > 30% win lying around because we should have a better solution still. > > Hey Guys, given the discussion in the last 3 comments is still patch still > ready for checkin ? Yes. If not, we would have removed the checkin-needed keyword...