Closed
Bug 1234008
Opened 9 years ago
Closed 7 years ago
Use zopflipng to compress png images
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
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
Reporter | ||
Comment 1•9 years ago
|
||
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%)
Reporter | ||
Comment 2•9 years ago
|
||
saves about 400k off the linux .tar.bz2 download
Reporter | ||
Comment 3•9 years ago
|
||
Reporter | ||
Comment 4•9 years ago
|
||
Reporter | ||
Comment 5•9 years ago
|
||
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.
Reporter | ||
Comment 6•8 years ago
|
||
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.
Reporter | ||
Comment 7•8 years ago
|
||
platform orig_size new_size delta
macosx 111620334 111177360 -442974
win64 40055608 39634224 -421384
linux64 55614039 55011581 -602458
Comment 8•8 years ago
|
||
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.
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Attachment #8909592 -
Flags: review?(dolske)
Reporter | ||
Comment 10•7 years ago
|
||
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.
Comment 11•7 years ago
|
||
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)
Comment 12•7 years ago
|
||
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)
Reporter | ||
Comment 13•7 years ago
|
||
(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
Comment 14•7 years ago
|
||
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.
status-firefox59:
--- → affected
Reporter | ||
Comment 15•7 years ago
|
||
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.
Comment 16•7 years ago
|
||
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)
Reporter | ||
Comment 17•7 years ago
|
||
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)
Comment 18•7 years ago
|
||
It runs on Mac. I'm happy to help.
Comment 19•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8909592 -
Flags: review?(dolske)
Comment 20•7 years ago
|
||
Tim, you interested in picking this up when you're back?
Flags: needinfo?(timdream)
Comment 21•7 years ago
|
||
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)
Comment 22•7 years ago
|
||
Sure, happy to help. But what kind of bot are you thinking about? Thanks!
Flags: needinfo?(sledru)
Comment 23•7 years ago
|
||
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.
Assignee | ||
Comment 24•7 years ago
|
||
Sure. I can take it.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•7 years ago
|
||
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 27•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 29•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 30•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 32•7 years ago
|
||
mozreview-review |
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+
Comment 33•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment 35•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment 37•7 years ago
|
||
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0de748c25f49
Recompress some png images with zopflipng r=Dolske,Grisha,jryans
Comment 38•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•7 years ago
|
status-firefox59:
affected → ---
Comment 39•7 years ago
|
||
Comment 40•7 years ago
|
||
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)
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•