Closed Bug 1234008 Opened 8 years ago Closed 6 years ago

Use zopflipng to compress png images

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: catlee, Assigned: timdream)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

Firefox 43.0.1 (on Windows) ships with about 4.4MB of png images. Some of these are fairly large, and benefit from further compression using zopflipng [1].

If we process all the png files with `zopflipng -m`, we reduce the total size of the png images to 3.3MB.

[1] https://github.com/google/zopfli
See Also: → 1234009
more precisely:
original: 4,534,076 bytes
zopflipng -m: 3,414,027 bytes (-24.7%)
zopflipng --iterations 500 --filters=01234mepb --lossy_transparent: 3,375,165 bytes (-25.6%)
See Also: → 1237709
saves about 400k off the linux .tar.bz2 download
Attached file zopflipngwrapper
Attached file fix_pngs.sh
To use:
- Get or build yourself a copy of zopflipng, and put it into tools/zopflipng
- Run tools/zopflipng/fix_pngs.sh

I ran into several issues when working on this. Firstly, some pngs shouldn't be modified, particularly those in the png test suites! The fix_pngs.sh script skips touching pngs in test directories, python directories, and the objdir.

Secondly, zopflipng's default behaviour strips out many png chunks that it seems we rely on. It also seems to corrupt animated pngs. The zopflipngwrapper script is used to skip animated pngs and to preserve important png chunks.
Blocks: 1303172
I did a try push here based on recent code from central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4d82ceb82341b35ae151febec964e22e766b2e6

I think all the tests look good.
platform orig_size new_size  delta

macosx   111620334 111177360 -442974
win64     40055608  39634224 -421384
linux64   55614039  55011581 -602458
See Also: → 1377705
Can we try running these images through ImageOptim (https://imageoptim.com/mac) as well, which uses a number of different compression tools. From my experience, it manages to shave off more than any other tool.
Attachment #8909592 - Flags: review?(dolske)
To identify the images to process, I took a recent win64 build and processed all the pngs in it, and then found all the images that shrunk by more than 1k. I then ran the images' parent directories through zopflipng.
ahal: do you think it is worth establishing a lint task for images? We could even have it produce an artifact that is a diff/bundle that could be applied to fix things :)
Flags: needinfo?(ahalberstadt)
Yeah, I think that sounds worthwhile. Even better than producing the artifacts would be adding the ability to fix them in-place (./mach lint --fix is a thing). I don't know much about your new docker configuration mechanism, but if you wanted an example, the python compat linter would be a good one to follow.
Flags: needinfo?(ahalberstadt)
(In reply to Gregory Szorc [:gps] from comment #11)
> ahal: do you think it is worth establishing a lint task for images? We could
> even have it produce an artifact that is a diff/bundle that could be applied
> to fix things :)

That's bug 1363489
Any updates on this patch to recompress PNG images? Now that the new Photon UI has shipped in Firefox 57, this patch will need to be rebased.

Stylo has been enabled on Android, causing a ~2 MB regression on the compressed APK file size. It would be nice if could claw back some size savings by recompressing our PNG images.
I've reprocessed the files on 59 and done a few try pushes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dc56b854ed471e5bf14b786ba8dd0cbc4cf4f2b
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b650836dc5607eddb38b11ea91f0abddc28cbbf3

It doesn't seem to impact APK file sizes much. Bug 1400953 would have a bigger impact, by not even including those assets.
Not quite sure why the reference to APK file sizes - isn't this bug about the desktop app size?

As for the recompression effort - did you try with just one compression method, or did you try ImageOptim (which does multiple passes)?
Flags: needinfo?(catlee)
This bug is about png files in general. :cpeterson was commenting about APK file sizes in c#14.

I don't have access to ImageOptim, so I didn't try it.
Flags: needinfo?(catlee)
It runs on Mac. I'm happy to help.
Whelp. I think it's (well past) time for me to declare review bankruptcy here. :( I was hoping to do this over the holiday break for fun (!), but clearly that didn't happen.

The 2 things I think would be good for a reviewer to look at are:

1) Think a bit on what the script/tool is doing, and if there's risk of it causing any undesired changes to PNGs. (I don't think we rely on any PNG-specific stuff like gamma or color correction, but that kind of thing.) It occurs to me that the mozscreenshots might be an easy way to verify there are no visual changes. (https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots)

2) Double check that no animated PNGs (APNGs) got caught up in this, since most tools tend to "optimize" out the animation (due to unrecognized chunks).

Otherwise, I would recommend biasing towards getting this landed ASAP. We can always spin out some followup work for automation, finding the most optimal tool/settings, etc.
Attachment #8909592 - Flags: review?(dolske)
Tim, you interested in picking this up when you're back?
Flags: needinfo?(timdream)
Sylvestre: I'm needinfo'ing you so this feature request is on your radar. Seems like something the PI side of the world cares about and something your army of bots could help with.
Flags: needinfo?(sledru)
Sure, happy to help. But what kind of bot are you thinking about? Thanks!
Flags: needinfo?(sledru)
I was thinking a lint bot that ran png files through zopflipng and verified the to-be-checked-in file is good might be a good idea.
Sure. I can take it.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
I have run the script against the latest central. There are a few changes I need to do to adopt the script on Mac.

I ran the following command with ImageMagick to ensure there is no visual differences:

hg status --change tip | cut -f 2 -d ' ' | xargs -I {} magick compare -verbose -metric AE {} ../central2/{} /dev/null 2>&1 | grep all:

(In reply to Andreas Bovens [:abovens] from comment #16)
> As for the recompression effort - did you try with just one compression
> method, or did you try ImageOptim (which does multiple passes)?

ImageOptim is just a front-end to multiple tools, including optipng etc.; we can certainly pass the file against multiple tools at once, since they are all lossless, and they drop their results when the resulting file is bigger than the input. I, however, don't think it's worthwhile...
Comment on attachment 8946175 [details]
Bug 1234008 - Recompress some png images with zopflipng

https://reviewboard.mozilla.org/r/216140/#review223008

Mostly a rubberstamp, but a few things:

* Please get a courtesy review from a mobile and devtools peer for those bits (mostly just so they're aware)
* Stuff under /browser/extensions may need coordinated with any canonical upstream repos (probably ignore it here, and handle in a followup?)
* Stuff under /gfx and /js should probably just be excluded sine it doesn't ship, but doesn't matter much either way
* Let's also exclude things under /third-party, since none of that looks useful enough to bother upstreaming.
Attachment #8946175 - Flags: review?(dolske) → review+
Comment on attachment 8946175 [details]
Bug 1234008 - Recompress some png images with zopflipng

https://reviewboard.mozilla.org/r/216140/#review223070

Thanks, seems like a good thing to do!
Attachment #8946175 - Flags: review?(jryans) → review+
It looks like with the exception of Firefox Screenshots, m-c is considered the upstream repo of other extensions. I will re-include them in my patch.
Comment on attachment 8946175 [details]
Bug 1234008 - Recompress some png images with zopflipng

https://reviewboard.mozilla.org/r/216140/#review226682

This looks sensible to me. mobile/android/app/src/photon/res/drawable-xxhdpi/alert_download_animation_4.png and friends looked suspicious at first, but they're individual frames, not animated pngs.
Attachment #8946175 - Flags: review?(gkruglov) → review+
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 827 changes to 827 files
remote: (d66de817cc0d modifies servo/resources/rippy.png from Servo; not enforcing peer review)
remote: (d66de817cc0d modifies servo/resources/servo.png from Servo; not enforcing peer review)
remote: (d66de817cc0d modifies servo/resources/tumbeast.png from Servo; not enforcing peer review)
remote: (d66de817cc0d modifies servo/support/android/apk/app/src/main/res/mipmap/servo.png from Servo; not enforcing peer review)
remote: (1 changesets contain changes to protected servo/ directory: d66de817cc0d)
remote: ************************************************************************
remote: you do not have permissions to modify files under servo/
remote: 
remote: the servo/ directory is kept in sync with the canonical upstream
remote: repository at https://github.com/servo/servo
remote: 
remote: changes to servo/ are only allowed by the syncing tool and by sheriffs
remote: performing cross-repository "merges"
remote: 
remote: to make changes to servo/, submit a Pull Request against the servo/servo
remote: GitHub project
remote: ************************************************************************
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.e_prevent_vendored hook failed
abort: push failed on remote
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 823 changes to 823 files
remote: (1 changesets contain changes to protected security/nss/ directory: 37964d07ac0b)
remote: ************************************************************************
remote: You do not have permissions to modify files under nsprpub/ or security/nss/
remote: 
remote: These directories are kept in sync with the canonical upstream repositories at
remote: https://hg.mozilla.org/projects/nspr and https://hg.mozilla.org/projects/nss
remote: 
remote: Please contact the NSPR/NSS maintainers at nss-dev@mozilla.org or on IRC
remote: channel #nss to request that your changes are merged, released and uplifted.
remote: ************************************************************************
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.f_prevent_nspr_nss_changes hook failed
abort: push failed on remote
See Also: → 1438843
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0de748c25f49
Recompress some png images with zopflipng r=Dolske,Grisha,jryans
https://hg.mozilla.org/mozilla-central/rev/0de748c25f49
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/8ac108c96aa06ed3c03d7cd024c920c991fc1617
chore(mc): Port Bug 1234008 - Recompress some png images with zopflipng r=Dolske,Grisha,jryans (#3991)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.