460629 - system Display profile should be ignored/modified if R+G+B does not equal white

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[Chris Murphy]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3

The XYZs for red, green, and blue primaries should equal D50 white, if the profile is correctly built per the ICC spec. A display with a white point other than D50 has primaries based on actual display color temperature, and need to be chromatically adapted to D50. If this is not done, currently OS's will not reject the profile as invalid, and applications including FireFox will display white as something other than white. See Bug 450923.

The formula is to add up the X values for red, green and blue primaries in the display profile; Y values for red, green, and blue, and the Z values for red, green and blue. The resulting XYZ value should be: 0.96420, 1.00000, 0.82491.

Reproducible: Always

Steps to Reproduce:
gfx.color_management.mode 1 (easier to test, also occurs with 2)
set attached display profile as the current display profile


Actual Results:  
content that should display white (255,255,255) are reproduced as yellow; overall there is a yellow shift.

Expected Results:  
content that should reproduce white (255,255,255) should display white.

Comment 1

[Chris Murphy]

Attachment: sample ICC display profile

this profile aids in exhibiting the problem described in the bug. its RGB primaries are not correctly adapted. When added, they come to the 6500K white, not D50, therefore white always appears yellowish on screen when it is used, and color management is enabled.

Comment 2

[Bobby Holley (:bholley)]

Attachment: patch to detect bogus profile by chris' criterion - this has an fprintf that should be removed before seeking review

Added a patch to detect bogus profiles.

Builds with this patch are at https://build.mozilla.org/tryserver-builds/2008-10-29_19:[e-mail]-bholleycmsdetectbogus3/

If you run one of these builds from the console, it should say "Output color profile looks bogus - using sRGB" if the detection comes out positive. The detection happens both on the system profile and custom profiles configured in gfx.color_management.display_profile in about:config, so it should be pretty easy to test.

Chris, can you grab one of the builds and give it a shot? It seems to work (rejects the attached ICC profile, accepts my monitor's profile and sRGB), but it would be good to check it on a range of profiles.

Comment 3

[Chris Murphy]

(In reply to comment #2)

> If you run one of these builds from the console, it should say "Output color
> profile looks bogus - using sRGB"

Suggest the following: "Display profile is invalid – using sRGB instead."

Of for more information: "Display profile is invalid (unadapted primaries) – using sRGB instead."

There are three device classes for ICC profiles: display, input, output. So I think the error should explicitly point out the problem is with a display profile. I'm OK with including either "color" or "ICC" as a qualifier.

Comment 4

[Bobby Holley (:bholley)]

As a clarification - the message is only to help testers of this patch/build determine whether this patch correctly identifies bogus profiles and resolves issue - it will be removed before this patch is landed in trunk.

Comment 5

[Chris Murphy]

(In reply to comment #2)
> Chris, can you grab one of the builds and give it a shot? It seems to work
> (rejects the attached ICC profile, accepts my monitor's profile and sRGB), but
> it would be good to check it on a range of profiles.

I'm not getting a message of any sort. Should I always get a message if I have a bad display profile selected? Or is there some other condition that needs to be fulfilled?

Comment 6

[Chris Murphy]

No message, but it appears to be working in that I'm not getting a white shift, and also test tagged content is still being color managed (i.e. the ICC web site's v4 test page works fine).

Comment 7

[Bobby Holley (:bholley)]

The message should print only in cases when a bad profile is selected.

Are you running firefox from the console? The message is just a printf, so you won't see it unless you launch it from a command line.

Comment 8

[Chris Murphy]

Ahh. OK Mac OS has a separate Console application that shows these kinds of messages as they get logged. I just launched it and I see the following entry...


11/2/08 4:20:13 PM [0x0-0x17d17d].org.mozilla.firefox[4883] Output color profile looks bogus. Using sRGB... 

Everything seems to be working as expected then.

Comment 9

[Bobby Holley (:bholley)]

Does it still seem to respect any valid profiles you have lying around? I just want to make sure I don't need to twiddle with the tolerance threshold (currently .01) to prevent legitimate profiles from being disabled.

Comment 10

[Chris Murphy]

Is is respecting valid profiles. But all of the ones I have, have exactly correctly adapted primaries.

If the primaries were adapted imprecisely, I can imagine some error is possible, but I can't imagine it would be much more than +- 0.002 in normalized XYZ.

Still, 0.01 is a pretty tight tolerance. Tighter than what Adobe is using (not more than 3 in any one channel in LAB space). The approximate equivalent would be:

+ 0.017460 for X
- 0.017252

+ 0.018108 for Y
- 0.017891

+ 0.037694 for Z
- 0.036580

Comment 11

[Chris Murphy]

So you could use 0.02 for X and Y, and .04 for Z and be close to, but slightly more tolerant than what Adobe is doing.

Comment 12

[Bobby Holley (:bholley)]

Attachment: patch with more intelligent tolerance

I've altered the patch per Chris' recommendations. A build is in the pipe, and I'll post a link to it here once it's finished.

Comment 13

[Bobby Holley (:bholley)]

Builds came through, but the win32 builder is down. Mac and linux builds are here:
https://build.mozilla.org/tryserver-builds/2008-11-02_14:[e-mail]-bholleycmsdetectbogus4/

Still work ok?

Comment 14

[Chris Murphy]

Still rejects sample bad profiles, and honors known good profiles. We could use a larger sample size, but I'm confident in the tolerance factor at this point, and in the solution.

Comment 15

[Bobby Holley (:bholley)]

Attachment: bogus profile detection patch - ready for review

Ok - sounds good.

Uploading a patch without the fprintf, flagging vlad for review.

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 345993 [details] [diff] [review]
bogus profile detection patch - ready for review

Requesting approval 1.9.1.

This is a very important feature for LCMS, as it detects and disables bogus profiles, which are probably the #1 user complaint we hear when it comes to color management. Shipping without this could potentially cause a lot of people to garbled images when they have embedded profiles. We know that this patch detects a lot of the reference bogus profiles we have, so the biggest risk we run is disabling some legitimate profiles that exist out in the wild. We haven't yet encountered a legitimate profile that this code rejects, and it's unclear what that would even mean, since according to Chris a legitimate profile _must_ follow this criterion. The only subjective issue here is the tolerance/fuzzfactor that we allow. We chose one copied from adobe, which is probably a safe bet.

Comment 17

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

Comment on attachment 345993 [details] [diff] [review]
bogus profile detection patch - ready for review

a191=beltzner

Comment 18

[Bobby Holley (:bholley)]

Attachment: Same final patch as above, Modified with user/commit message

Added an identical patch with a user and commit message set for metered checkin.

Comment 19

[Bobby Holley (:bholley)]

Attachment: MQ formatted, proper username [Checkin: Comment 23 & 26]

Screwed up the username string. Fixed.

Comment 20

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/0586ee185c87

Comment 21

[Shawn Wilsher :sdwilsh]

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

Comment 22

[Shawn Wilsher :sdwilsh]

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

Comment 23

[Bobby Holley (:bholley)]

pushed to mozilla-central in 31f8780c1963

We'll let this bake tonight, and hopefully we won't see that perf regression again (I was always a bit skeptical of it). Assuming it all looks good, I'll push to 191 tomorrow.

Comment 24

[Bobby Holley (:bholley)]

Resolving as fixed - marked as needing 191 landing

Comment 25

[Bobby Holley (:bholley)]

5e280f6121b1

Comment 26

[Bobby Holley (:bholley)]

Sorry for the cryptic comment - that meant to say:

Pushed to releases/mozilla-1.9.1 in 5e280f6121b1

Comment 29

[Seth Spitzer]

in bug #450923 Bobby asked: 

"This should be fixed in the 3.2 nightlies already, and in the 3.1 nightlies
once they roll tonight. Can someone confirm that this is the case? If so, I'll
close this bug."

I see the bug with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1) Gecko/20081007 Firefox/3.1b1

I don't see it with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20081211 Minefield/3.2a1pre

Hope that helps.