Someone commented on my blog mentioning that they had found some PNGs that firefox renders incorrectly (way too blue) but safari rendered fine. I suspected, and then confirmed, that these PNGs had embedded cHRM chunks, which we currently use to build an ICC profile on the fly but Safari just ignores. I'm not sure how widespread the problem is. However, if many of these images exist, we may want to just disable support for the cHRM chunk by default, since pretty much all serious color management is done through icc profiles (in PNGs, this is the iCCP chunk) anyway.

According to pngcheck, this has the following gAMA/cHRM chunks: chunk gAMA at offset 0x00025, length 4: 0.45470 chunk cHRM at offset 0x00035, length 32 White x = 0.31269 y = 0.31269, Red x = 0 y = 0.31269 Green x = 0 y = 0.31269, Blue x = 0 y = 0.31269

According to pngcheck, this has the following gAMA/cHRM chunks: chunk gAMA at offset 0x00025, length 4: 0.45455 chunk cHRM at offset 0x00035, length 32 White x = 0.3127 y = 0.3127, Red x = 0 y = 0.3127 Green x = 0 y = 0.3127, Blue x = 0 y = 0.3127

CCing Chris Murphy. Chris - can you have a look at those cHRM chunks and verify that they should produce the output in the screenshot? I was forcing sRGB as the output profile when I took those shots. Also, do you have any input on the importance of cHRM profiles? Does Photoshop respect them?

CCing vlad and dolske - what to you guys think on this issue?

I don't have a strong opinion. We could probably ignore cHRM, although we might find similar problems with people embedding bogus iCCP/sRGB data. I guess the interesting question how much legitimate usage of cHRM is out there -- are we just bringing needless pain by supporting something no one is using (knowingly), or are we doing something people want at the cost of having a few broken images? Not sure if such data exists or can be easily gathered. Maybe see what Gimp and Photoshop do if you save a image as a PNG with color management? Bug 455057 found a lot more cHRM than iCCP instances in our tree, but we're probably not a representative sample.

(In reply to comment #7) > Chris - can you have a look at those cHRM chunks and verify that they should > produce the output in the screenshot? I'm not sure how to extract chunks or understand what the values mean. > I was forcing sRGB as the output profile > when I took those shots. Also, do you have any input on the importance of cHRM > profiles? *shrug* Issue 1: ICC profiles are huge in comparison to cHRM and gAMA chunks which serve the same purpose. I've been critical of the ICC not allowing for much smaller ICC profiles, but I think it's considered unimportant. If you won't dedicate the space, it's not worth tagging in a unique colorspace (one other than sRGB). I think that's probably fair even though I'd like things to work differently, including URL based embedded ICC profiles that reference the profile elsewhere. Issue 2: I think cHRM and gAMMA are redundant. I'd remove them from the spec. If there are problems where applications are writing out bogus cHRM or gAMMA data then they're polluting the spec intent and solutions around the idea. Although I'm in favor of following specs, that ends when there's sufficient pollution of the idea. So I'm not married to the idea of honoring it, I don't have a problem with ignoring it and assuming the image is sRGB if there isn't an iCCP chunk. The PNG folks may disagree but there is speculatively around 100 orders of magnitude more scrutinity with embedded ICC profiles being correctly formed, in comparison to any validation of cHRM and gAMA floating around in the world which are largely ignored. That is, any ICC embedded in PNG is vastly more likely to be a known good profile than cHRM or gAMA data just because ICC profiles exist in other applications and do get tested, especially the most common ones: sRGB, Adobe RGB (1998), etc. >Does Photoshop respect them? Will investigate and report back.

(In reply to comment #10) > (In reply to comment #7) > > Chris - can you have a look at those cHRM chunks and verify that they should > > produce the output in the screenshot? > > I'm not sure how to extract chunks or understand what the values mean. > I pasted a dump of the data in the comments when I uploaded the images. According to the png spec, the cHRM chunk "specif[ies] the 1931 CIE x,y chromaticities of the red, green, and blue primaries used in the image, and the referenced white point". Is that enough to interpret the information?

(In reply to comment #10) > (In reply to comment #7) > Issue 1: ICC profiles are huge in comparison to cHRM and gAMA chunks which > serve the same purpose. Actually an ICC profile that contains a simple gamma curve and chrm values can be pretty small. I think the Adobe 1998 ICCP chunk is only about 300 bytes. Reason being that the gamma curves can be conveyed directly in an ICC profile without having to write a large lookup table, as is required to convey sRGB (because of the linear "toe" in the curve, which Adobe 1998 doesn't have).

(In reply to comment #1) > According to pngcheck, this has the following gAMA/cHRM chunks: > > chunk gAMA at offset 0x00025, length 4: 0.45470 According to spec: EXAMPLE A gamma of 1/2.2 would be stored as the integer 45455. http://www.libpng.org/pub/png/spec/iso/index-object.html#11gAMA > chunk cHRM at offset 0x00035, length 32 > White x = 0.31269 y = 0.31269, Red x = 0 y = 0.31269 > Green x = 0 y = 0.31269, Blue x = 0 y = 0.31269 Bogus. The x,y values are impossible (i.e. that point isn't a real color that we're even capable of seeing, even if a real device could reproduce it it's outside the gamut of human vision), and the same value is used for all three primaries therefore it's a gamutless device.

(In reply to comment #12) > Actually an ICC profile that contains a simple gamma curve and chrm values > can be pretty small. I think the Adobe 1998 ICCP chunk is only about 300 > bytes. It's 560 bytes, but I get the point. cHRM and gAMA combined are 8 bytes. So "huge" is relative to that, which is pretty dinky. A lot of images end up with sRGB embedded in it which isn't so small, 3144 bytes. An ICC v4 version of sRGB could describe the TRC as a parametric function instead of using a table as is currently the case with v2 sRGB (at least the real versions of it; some are floating out there with TRC defined as gamma 2.2 function). The parametric function would conserve a lot more space compared to its table equivalent as you point out.

Confirm images appear as I would expect them to appear with cHRM data honored as valid source (which it is not), which is why they reproduce completely cyan. The reported primaries for the image are all red (laser beam red, not a single other color). Confirm Preview on OS X ignores this, as does Photoshop CS3. Baffling why someone would create an application that intentionally inserts bogus cHRM data.

Added a patch to remove cHRM handling. A build is in the pipe, and I'll post it for testing once it's done. We're seeing similar stuff in bug 462834, so I thought it might be good to get moving on this.

I have no problem with removing cHRM handling. But we could also come up with some metric to determine whether it's possible or not. So far the bad cHRM values have been ludicrous, and thus really easy to ignore. But I have no data on the value of cHRM (how often is it useful vs. less than useful); nor do I have data on the instance of cHRM being present but ICCP not present. If ICCP is present then cHRM an gAMA should be ignored.

(In reply to comment #17) > If ICCP is present then cHRM an gAMA should be ignored. That's correct. The PNG spec says "An sRGB chunk or iCCP chunk, when present and recognized, overrides the cHRM chunk." There's a similar statement about the gAMA chunk. Also, "PNG decoders that are used in an environment that is incapable of full-fledged colour management should use the gAMA and cHRM chunks if present."

(In reply to comment #18) >Also, "PNG decoders that are used in an environment that is incapable > of full-fledged colour management should use the gAMA and cHRM > chunks if present." The spec is imperfect. It assumes the encoder always writes out valid cHRM data, so it doesn't give any advice on what to do in the instance of invalid cHRM data. I think ignoring is fine, there are better ways to do this. If the PNG folks want to suggest what to do in this situation, great. Otherwise, I'd say a.) ignore it; b.) if we really care to change someone else's filled diapers, come up with a metric to ignore/honor on a rational basis. Personally, I prefer a plan to find the developer of the PNG encoder that's writing out invalid cHRM data, and pantsing him in public. He went to the trouble of writing out cHRM in the first place, but then decided to encode red, green and blue with the same value, which also happens to not be a real color. Gamutless and colorless color space? That's a first. What a joker.

a.) ignore it By it, I mean cHRM. As in, always ignore cHRM. (Strict reading of the spec would allow for this as FF is capable of full-fledge color management, therefore it isn't obligated to ever honor cHRM.) b.) conditional ignore, which requires code with a pass/fail threshold, kinda like for bug 460629.

My general feeling on this is that if apple and adobe products ignore these chunks, then we probably should too. Given that Mozilla isn't really the canonical image viewing tool in the graphics industry, I don't think we'll get very far trailblazing on this issue. Builds of Firefox with cHRM support disabled are here: https://build.mozilla.org/tryserver-builds/2008-11-03_11:38-bobbyholley@stanford.edu-bholleycmsnochrm/ Can someone give them a go?

Comment on attachment 346088 [details] [diff] [review] patch to remove cHRM support from libpr0n color management routines Fixes the problems over at bug 462834. Flagging vlad for review.

(In reply to comment #21) > My general feeling on this is that if apple and adobe products ignore these > chunks, then we probably should too. Given that Mozilla isn't really the > canonical image viewing tool in the graphics industry, I don't think we'll get > very far trailblazing on this issue. I agree. > Can someone give them a go? Correct renders images with known faulty cHRM. PNG without cHRM unaffected.

Comment on attachment 346088 [details] [diff] [review] patch to remove cHRM support from libpr0n color management routines I'm not really a superreviewer, but we'll pretend that Vlad and I switched spots on this one. :) From a purely standards perspective, I *might* prefer to have a "detect bad cHRM chunks," but I don't feel that strongly about it. Let's remove support for it and see if anyone complains.

The patch should also disable PNG_READ_cHRM_SUPPORTED in libimg/png/mozpngconf.h and should return cHRM to the list of unused chunks in nsPNGDecoder.cpp (just move it from the color_chunks list to the unused_chunks list).

Glenn - Can you be more specific about what needs to happen in mozpngconf.h?

(In reply to comment #27) > Glenn - Can you be more specific about what needs to happen in mozpngconf.h? at line 59 add #define PNG_NO_READ_cHRM

moves cHRM to "unused chunks" list and disables cHRM support in the embedded libimg/png library.

Comment on attachment 346291 [details] [diff] [review] Remove cHRM support from libpr0n color management routines (v1) Looks good - flagging joe for re-sr

I pointed the libpng group to this discussion. The libpng group seems to be of the opinion that it is much better to reject bogus cHRM chunks with a warning message than to arbitrarily reject all cHRM chunks. We will probably have code in libpng-1.4.0 to do that. Libpng already checks for a valid range of data. The new tests will check for a non-zero area of the RGB triangle and will check that the white-point is inside the triangle. The code will be small enough that we could put it in libpr0n/decoders/png rather than waiting for libpng-1.4.0 to be released.

I think this is simply a bug in the error checking in little-cms. In cmswtpnt.c it checks the result of MAT3Inverse as though it was a boolean (LCMSBOOL), but MAT3inverse is declared int and returns (-1) on error. (The same bug exists in cmsmtrx.c itself, but not in cmsform.c where the checks are for <0). The end points in the given PNGs cannot be correctly converted into XYZ form because they are not distinct, so the attempt to create the profile should fail. So far as I can see little-cms does everything correctly apart from getting the error check wrong. It's a safe fix anyway - the two boolean tests are obviously wrong given the actual MAT3inverse code. BTW, when it fails MAT3inverse returns garbage - identity or a partially inverted matrix - so the code definately shouldn't proceed on error.

This patch fixes the lcms bug that John Bowler reported in the previous comment. If it works then we would not need to remove cHRM support.

Bobby, attachment 348202 [details] [diff] [review] seems like a much better fix -- can you comment on whether it's likely to fix the general case?

I'm swamped this week - I'll comment after thursday.

The patch is good and should be applied regardless of what happens - the function returns -1, not 0, in the singular case, so the current code is flat-out wrong. The more interesting and less answerable question has to do with whether singular matrix detection alone is enough to handle the bogus chrm chunks out there. If there's really only one app out there that spits out garbage for cHRM chunks and that garbage always occurs in a form which produces non-invertible matrices, then yes, we're good. However, it's quite possible that this is not the case. Either way, I think the best course of action is to land this patch, put this bug on hold, and see what happens. If we receive more cHRM-ish complaints, then we can re-open the issue and reconsider dropping cHRM support.

Comment on attachment 348202 [details] [diff] [review] Fix bug in lcms error detection r+ing this patch. If nobody objects, I'll land it (along with the rest of my cms patches) once the tree re-opens this week.

Err, I realized it's not exactly clear from the bug history that this fixes the test cases. Has anyone confirmed this?

Also detects yn==0 which would lead to a floating point divide-by-zero

Comment on attachment 350030 [details] [diff] [review] Revised to detect additional error condition in cHRM data nitpick, but can you use 0.0 instead to make it more clear? Also, can you comment on comment 38?

Re: comment #38, the PNG group has not tested with Mozilla but only verified with other programs that the sample PNGs will be rejected. We'd be happy to do further tests if someone would make a try-server set of builds.

(In reply to comment #41) > Re: comment #38, the PNG group has not tested with Mozilla but only verified > with other programs that the sample PNGs will be rejected. More accurately, that the erroneous cHRM chunks in the sample PNGs will be rejected.

understood. I've kicked off a tryserver build. I'll post again once they become available (they should show up at https://build.mozilla.org/tryserver-builds/?C=M;O=D )

Comment on attachment 350057 [details] [diff] [review] Fix bug in lcms error detection (v1, with 0.0) Err, the MAT3inverse return value checks should still check against 0 and not 0.0, since those functions return an int. I was merely referring to the yn check, where you compared yn (a double) to 0 rather than 0.0.

Comment on attachment 350099 [details] [diff] [review] Fix bug in lcms error detection (v2, with 0 and 0.0) Looks good. r=bholley Builds are available at https://build.mozilla.org/tryserver-builds/2008-11-25_15:21-bobbyholley@stanford.edu-glennrpchrmpatch/ Can you give em a go? If we have a confirmed bug fix, I'll nominate this for 1.9.1 approval and try to get it landed this week.

The test images "players.png" and "Macbook.png" look OK with the tryserver build for windows, using Windows XP and setting gfx.color_management.mode to 1 or 2.

This test case renders fine with the patch on Windows XP. Without the patch it is rendered in black-and-white or nearly so.

Comment on attachment 350099 [details] [diff] [review] Fix bug in lcms error detection (v2, with 0 and 0.0) Flagging for 1.9.1 approval. This patch is important since it seems to fix the issues with bogus cHRM profiles we were seeing on the web (see the test cases attached) which made firefox appear to render some images completely incorrectly while other browsers handled them fine. This patch is very very low risk, since it just corrects 2 incorrect error checks (the error code to check for is -1, not 0) and checks for a divide-by-zero error before it occurs.

Glenn - Can you also post the profiles associated with the testcases so that i can add tests for them to bug 466875? I'm not sure how to use pngcheck to extract a raw .ICC profile from a png (how would one go about that?)

(In reply to comment #50) > Glenn - Can you also post the profiles associated with the testcases They don't have profiles. That's the whole point: if there is no iCCP chunk then the decoder falls back on the gAMA and cHRM chunks to obtain colorspace info. If you are asking about the cHRM chunk contents, then see comment #1 and multiply the floating point numbers by 100000 to get the integers that were in the cHRM chunk. For these erroneous cHRM chunks there is no equivalent profile. For legitimate cHRM chunks, then those would represent an equivalent profile. So the test program needs to be working with the actual cHRM chunks.

Oh, duh - I had a momentary brain lapse. I suppose in this case the best thing to do would be to manually create the profile somehow using that information and make sure lcms rejects it. This might be tricky, but it's probably doable.

These cHRM chunks are underspecified. It isn't possible to create a profile from them - that's why lcms gives effectively random results. There are many values of display device chromaticity that cannot be converted into a unique set of display device primary colors. There are also many chromaticities (even with the restrictions imposed by the PNG representation) that correspond to impossible primaries - e.g. primaries with a negative luminosity. These *can* be handled, but the gamut mapping of a 'negative' color is, at best, somewhat academic. To avoid lcms trying to process bogus cHRM chunks both the fixes in this fix *and* some changes to libpng are required. At present lcms does not detect all underspecified cases as a result of internal floating point rounding. The first two images on the following test page are (currently) being color managed on firefox but should not be: http://www.kerbyville.com/large/images/png/bad-chrm/bad-chrm.html IMO that's a separate bug, because it requires a fix in libpng (fixing it in lcms is impossible because lcms takes floating point values which cannot exactly represent the PNG fixed point values in question.)

(In reply to comment #52) > Oh, duh - I had a momentary brain lapse. I suppose in this case the best thing > to do would be to manually create the profile somehow using that information > and make sure lcms rejects it. This might be tricky, but it's probably doable. It is not doable because there is no such profile.

(In reply to comment #54) > It is not doable because there is no such profile. Why can't we just make the same calls that libpng makes to create the profile and make sure that it returns NULL?

Libpng does not create a profile. It just stores/retrieves the cHRM parameters. It is up to the application to convert those to a profile if it so desires. The NULL is coming from little-cms, when it fails to create a profile for some reason.

Comment on attachment 350099 [details] [diff] [review] Fix bug in lcms error detection (v2, with 0 and 0.0) a191=beltzner

Glenn - Gah, another brain-fart. I meant libpr0n. Right now we're getting some numbers back from libpng when we read the cHRM chunk, and then passing that all into lcms (this all happens in libpr0n). My suggestion is that we manually input those numbers rather than getting them from libpng, which would mean that we could then write a standalone test to make sure that lcms rejects those types of parameters.

Added a version of the patch with the proper commit message and glennrp as the author so that this can be added to the metered checkin queue.

Added a version of the patch with the proper commit message and glennrp as the author so that this can be added to the metered checkin queue.

Comment on attachment 350407 [details] [diff] [review] Same final patch as above, Modified with user/commit message Gah, stupid bugzilla. Sorry for the doublepost. The patch has been added to the metered checkin queue at https://wiki.mozilla.org/MeteredCheckins

I screwed up the username string on the previous patch. Updating.

(In reply to comment #53) > > To avoid lcms trying to process bogus cHRM chunks both the fixes in this fix > *and* some changes to libpng are required. At present lcms does not detect all > underspecified cases as a result of internal floating point rounding. The > first two images on the following test page are (currently) being color managed > on firefox but should not be: > > http://www.kerbyville.com/large/images/png/bad-chrm/bad-chrm.html > > IMO that's a separate bug, because it requires a fix in libpng (fixing it in > lcms is impossible because lcms takes floating point values which cannot > exactly represent the PNG fixed point values in question.) Libpng-1.2.34, which is due out on about December 12, will contain the fix for low cHRM parameter values that lcms cannot detect. I suppose it would be possible to copy the new libpng code into libpr0n to avoid having a dependency on libpng-1.2.34.

Backed out to investigate bug 467102: http://hg.mozilla.org/mozilla-central/rev/a3f9376ede1e

See bug 467102 comment 6 for an explanation about the perf regression.

I have uploaded a patch to bug #461935 (Update trunk to libpng-1.2.34) which actually updates the trunk to libpng-1.2.24beta04, that you can use to test that the changes mentioned in comment #53 are working.

pushed to mozilla-central in 15ad240fd7ab Assuming we see no perf regression, I'll push it to 191 tomorrow.

Uh, why didn't the patch get super-review before landing?

gfx stuff isn't always superreviewed before landing, depending on who reviews it and how big the patch is. Either way, I'm the lcms owner (stuart says it's too much of a pain to put me in despot, but he still acknowledges my ownership of it), and this patch only touches LCMS (furthermore, the changes are trivial). So r=bholley is more or less an sr.

Oh hm - I just looked up the rules and you're right - I didn't realize that module owners still needed sr. I'll check with vlad and make sure to do that in the future, unless you're seriously going to make me back out this patch.

Comment on attachment 350099 [details] [diff] [review] Fix bug in lcms error detection (v2, with 0 and 0.0) I won't make you back it out unless vlad or stuart denies superreview for it. Tagging vlad for this for now.

resolving as fixed, flagging for 191 landing

sr on the patch before 191 landing, please.

Comment on attachment 350099 [details] [diff] [review] Fix bug in lcms error detection (v2, with 0 and 0.0) Not everything needs superreview. In areas like this or gfx, it acts as more like an addl-review.

pushed to releases/mozilla-1.9.1 in 54564dffc958

Working off the latest nightlies on Trunk on Shiretoko, test cases 1-3 works fine on Trunk build on XP and OS X: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090504 Minefield/3.6a1pre ID:20090504031236 So, I'm going to push this to verified FIXED. BUT, test cases 1-3 on Shiretoko build do not render for images w/ the cHRM chunk on OS X and XP: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090504 Shiretoko/3.5b5pre ID:20090504031110 After a preliminary check from Fx3.1b3 release and Fx3.5b4 release, there's a regression that occurred from those two releases as Fx3.1b3 works fine and the latter shows a black image for each test case. I'm removing the fixed1.9.1 tag because of this result.

Should a new bug, with a dependency, be created for this issue on 1.9.1?

I believe that's bug 489133.

looks good now on Shiretoko, thanks! Verifying the other bug as well. Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Shiretoko/3.5pre ID:20090527031214