Some people with bad profiles noticed this issue at http://forums.mozillazine.org/viewtopic.php?p=4453945#p4453945 find | grep png | xargs /files/downloads/pngcheck -v | grep iCCP gives me 10 images in /files/mozilla/submit/mozilla-central/browser someone needs to pngcrush -rem iCCP them and check it in. I might get the chance to do it this afternoon

Faaborg has some automated scripts to handle some of this, presumably these images are either old or didn't go through him.

Here's everything curently in mozilla-central: ("hasiccp" script...) #!/bin/sh pngcheck -v $1 | grep -q iCCP $ find . -name \*.[pP][nN][gG] -exec ./hasiccp {} ';' -print ./browser/themes/pinstripe/browser/hud-style-button-middle-background.png ./browser/themes/pinstripe/browser/hud-style-twisties.png ./browser/themes/pinstripe/browser/places/pageStarred.png ./browser/themes/pinstripe/browser/places/starPage.png ./browser/themes/pinstripe/browser/places/toolbarDropMarker.png ./browser/themes/pinstripe/browser/tabbrowser/tab-arrow-end-bkgnd.png ./browser/themes/pinstripe/browser/tabbrowser/tab-arrow-start-bkgnd.png ./browser/themes/pinstripe/browser/Toolbar-small.png ./browser/themes/winstripe/browser/tabbrowser/tab-active-bkgnd.png ./browser/themes/winstripe/browser/tabbrowser/tab-bkgnd.png ./content/html/document/test/bug369370-popup.png ./extensions/cookie/test/image1.png ./extensions/cookie/test/image2.png ./other-licenses/branding/firefox/background.png ./testing/extensions/community/chrome/skin/highlight-end.png ./testing/extensions/community/chrome/skin/highlight-hover-end.png ./testing/extensions/community/chrome/skin/highlight-hover-mid.png ./testing/extensions/community/chrome/skin/highlight-hover-start.png ./testing/extensions/community/chrome/skin/highlight-mid.png ./testing/extensions/community/chrome/skin/highlight-start.png ./testing/extensions/community/chrome/skin/qmo-16px.png ./testing/extensions/community/chrome/skin/qmo-badge.png ./testing/extensions/community/chrome/skin/qmo.png ./toolkit/themes/pinstripe/global/icons/find-bar-background.png

Dolske - do you have time to quickly put together a patch that strips these?

Sure. Looks like there are also quite a few pngs with cHRM chunks, are those affected by the cms changes too?

Yeah. cHRM chunks end up turning into embedded profiles for all intensive purposes.

rmiccp script: #!/bin/sh rm -f /tmp/out.png ~/src/pngcrush-1.6.4/pngcrush -rem iCCP $1 /tmp/out.png mv /tmp/out.png $1 Given output from last comment (stripped of objdir): cat filelist | xargs -n1 ./rmiccp

There are a *bunch* more files with cHRM chunks, mostly in browser/themes/pinstripe and toolkit/themes/pinstripe. AFAIK we didn't use cHRM before, so these should be safe to strip out (right Bobby?), but CCing the pinstripe folks just to be sure.

Removes iCCP and cHRM chunks from most of the tree. I left these files alone: ./dom/tests/mochitest/dom-level2-html/files/w3c_main.png ./modules/libpr0n/test/reftest/pngsuite-ancillary/ccwn2c08.png ./modules/libpr0n/test/reftest/pngsuite-ancillary/ccwn3p08.png The W3C one could be changed, but I don't know if it's part of an "official" test suite or not, so I'll just leave it alone. The 2 pngsuite ones shouldn't be modified because they're intended to have cHRM chunks and are official test images.

For convenience, here's the list of modified files.

(In reply to comment #7) > AFAIK we didn't use cHRM > before, so these should be safe to strip out (right Bobby?) correct.

Comment on attachment 342154 [details] [diff] [review] Patch v.2 seems straightforward enough, no actual change to UI. Someone else might want to ensure that the changes are working correctly.

Comment on attachment 342154 [details] [diff] [review] Patch v.2 sr'ing mconnor... I think this will mostly be a rubberstamp, but since I touching some files in odd parts of the tree this seem sensible to do.

Comment on attachment 342154 [details] [diff] [review] Patch v.2 style nit: this is not really a useful patch.

(In reply to comment #13) > (From update of attachment 342154 [details] [diff] [review]) > style nit: this is not really a useful patch. Out of curiosity, what would make it more useful?

Pushed changeset 2037496284ee. Over in bug 420209 comment 1, I see that beltzer promised $1-per-KB (in beer) for removal of unused images. By my math, this patch removed 90,290 bytes of unused color profile data from images. I'm willing to accept payment in Westvleteren Abt 12 (or St. Bernardus Abt 12, in a pinch)!

*bholley* smacks himself for letting dolske have this bug.

(In reply to comment #14) > Out of curiosity, what would make it more useful? Attaching all 103 new images as separate files, for easy visual comparison. Remember, victory for mconnor is to see dolske suffer.

Oops, realized that in addition to removing iCCP and cHRM chunks, sRGB chunks can go away too. Alex's ui-r should be good for most of this, requesting a rubberstamp from dbaron for the parts in /layout/reftest. (None seem to be involved in color management tests, so this should be nice and safe.)

(Oh - saves 3,737 bytes in the tree, so bholley gets a complimentary case of natty lite!)

So what says what color space an image is in if it doesn't have an sRGB chunk (or one of the other things described in http://www.w3.org/TR/PNG/#4Concepts.ColourSpaces )? If we support the sRGB chunks, it seems based on the PNG spec that we're better off removing the gAMA chunk than the sRGB chunk. (And what color space indications do the images that you're *not* touching have?) Which rendering intent do these sRGB chunks that you're removing specify, out of curiousity?

(In reply to comment #21) > So what says what color space an image is in if it doesn't have an sRGB chunk > (or one of the other things described in > http://www.w3.org/TR/PNG/#4Concepts.ColourSpaces )? If we support the sRGB > chunks, it seems based on the PNG spec that we're better off removing the gAMA > chunk than the sRGB chunk. (And what color space indications do the images > that you're *not* touching have?) FWIW, we've always used the gAMA chunk since before the days of color management, whereas support for the sRGB chunk is new. I think the idea here is that the firefox UI was designed without color management, and so if the browser starts interpreting new information that changes the look of the UI, it makes the most sense to strip that information off our internal images. > > Which rendering intent do these sRGB chunks that you're removing specify, out > of curiousity? We override embedded rendering intents by default and force perceptual (this can be changed in about:config), so I don't think this matters.

Right, the point is that we didn't ever previously look at these chunks, so removing them should be completely safe. The main goal is to make sure end users with bad color profiles don't get messed up chrome images, I figured I would just clean the whole tree out while I was at it.

Comment on attachment 343636 [details] [diff] [review] sRGB removal v.1 This should result in no actual perceptual change to the UI, so clearly passes ui-review

Comment on attachment 343636 [details] [diff] [review] sRGB removal v.1 Seems like this is really moving in the wrong direction (in that in the long run we probably want sRGB chunks in all our images), but r=dbaron anyway.

(In reply to comment #23) > [..] The main goal is to make sure end users with bad color profiles > don't get messed up chrome images [..] A different approach to the same issue is taken at bug 460629: it detects bogus ICC color profiles and disables them. It should solve both bug 450923 and the correct visualization of chrome images with embedded profile.

Just as a clarification, we're still not sure that bug will detect all bogus profiles. We'll have to see how it plays out. dolske, did this ever land?

No, not yet.

Pushed sRGB removal in changeset 673d1ba18849.

It appears that this caused about a 30ms Ts regression on Vista. It also appears to have caused about a 30ms Ts win on XP. Apparently no other platform was impacted. Technically the Vista regression is a higher percentage than the XP win so maybe this should be backed out, but I don't know.

Actually I've gone ahead and backed it out, firstly to confirm that it is definitely the cause (there are technically two changesets in range) and secondly to ensure the appropriate decision is taken wrt relanding this.

I'm baffled as to how this would affect Ts, and yet the Talos numbers indicate that the backout changeset did return the numbers to their previous values... I *might* buy that the XP Ts gain was due to avoiding color management overhead, but Vista got slower. Only two images in this patch should even be relevant on Windows: browser/themes/winstripe/browser/tabbrowser/newtab-aero.png browser/themes/winstripe/browser/tabbrowser/newtab.png The other images are in gnomestripe/pinstripe, or not involved in Ts (eg, reftest images). I'll try later relanding parts of this to hopefully confirm that it is these two images. Not sure what to do after that; best guess is that the Ts change is either just the usual mysterious Talos wonkyness.

I don't really follow it either, but I don't believe we can blame talos oddness here given that the perf changes on landing and backout mirror each other so well. The change is having a clear effect on the browser.

Backout possibly caused https://bugzilla.mozilla.org/show_bug.cgi?id=463749 ??

Reference comment #34 , I cannot repo the problem in the linked bug above, so this bug is not the cause. Sorry for the spam.

Dolske - did you ever try relanding this? Also, do we want this in 191? Seems like it would make sense to.

Not yet, just waiting for a quiet period to try relanding.

Pushed http://hg.mozilla.org/mozilla-central/rev/b1251b9b986e This does *not* have the changes to the 2 files mentioned in comment 32, which apparently caused a Ts regression. Assuming this push is ok perf wise, I'll try pushing those two files later to see if they still cause as Ts change.

Perhaps landing just the new newtab.png and not newtab-aero.png would preserve the XP win without triggering the Vista loss (although this still makes little or no sense).

Checked the graphs.m.o Ts graphs for all platforms, no change in Ts since the comment 38 landing. Also looked at other graphs on PDB2, no other perf changes noted. Landed http://hg.mozilla.org/mozilla-central/rev/f75fec74d4b3 with only the change to newtab.png, which apparently improved Ts last time. Let's see what happens now.

Well, Ts did indeed drop again on the XP boxes (2 of 3 regular Talos boxes have reported, as well as the fast and fast-nochrome boxes). The Vista boxes show no change. I'm assuming that landing the newtab-aero.png change would show the regression again. At this point, I think it's best to close this bug, and I'll spin off a new bug for the Windows XP/Vista weirdness.

Filed bug 470730 on the aforementioned weirdness.

Comment on attachment 343637 [details] List of modified files (for sRGB) a191=beltzner