455057 - some chrome images have embedded profiles

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[Bobby Holley (:bholley)]

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

Comment 1

[Justin Dolske [:Dolske]]

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

Comment 2

[Justin Dolske [:Dolske]]

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

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Justin Dolske [:Dolske]]

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

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

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

Comment 7

[Justin Dolske [:Dolske]]

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.

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

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.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: List of modified files

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

Comment 10

[Bobby Holley (:bholley)]

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

correct.

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

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 12

[Justin Dolske [:Dolske]]

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 13

[Mike Connor [:mconnor]]

Comment on attachment 342154 [details] [diff] [review]
Patch v.2

style nit: this is not really a useful patch.

Comment 14

[Bobby Holley (:bholley)]

(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?

Comment 15

[Justin Dolske [:Dolske]]

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)!

Comment 16

[Bobby Holley (:bholley)]

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

Comment 17

[Phil Ringnalda (:philor)]

(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.

Comment 18

[Justin Dolske [:Dolske]]

Attachment: sRGB removal v.1

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.)

Comment 19

[Justin Dolske [:Dolske]]

Attachment: List of modified files (for sRGB)

Comment 20

[Justin Dolske [:Dolske]]

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

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 22

[Bobby Holley (:bholley)]

(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.

Comment 23

[Justin Dolske [:Dolske]]

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 24

[Alex Faaborg [:faaborg] (Firefox UX)]

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 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 26

[Diego]

(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.

Comment 27

[Bobby Holley (:bholley)]

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?

Comment 28

[Justin Dolske [:Dolske]]

No, not yet.

Comment 29

[Justin Dolske [:Dolske]]

Pushed sRGB removal in changeset 673d1ba18849.

Comment 30

[Dave Townsend [:mossop]]

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.

Comment 31

[Dave Townsend [:mossop]]

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.

Comment 32

[Justin Dolske [:Dolske]]

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.

Comment 33

[Dave Townsend [:mossop]]

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.

Comment 34

[Jim Jeffery not reading bug-mail 1/2/11]

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

Comment 35

[Jim Jeffery not reading bug-mail 1/2/11]

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

Comment 36

[Bobby Holley (:bholley)]

Dolske - did you ever try relanding this?

Also, do we want this in 191? Seems like it would make sense to.

Comment 37

[Justin Dolske [:Dolske]]

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

Comment 38

[Justin Dolske [:Dolske]]

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.

Comment 39

[Bill Gianopoulos [:WG9s]]

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).

Comment 40

[Justin Dolske [:Dolske]]

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.

Comment 41

[Justin Dolske [:Dolske]]

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.

Comment 42

[Justin Dolske [:Dolske]]

Filed bug 470730 on the aforementioned weirdness.

Comment 43

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a191=beltzner