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

RESOLVED FIXED in mozilla1.9.1b3

Status

()

RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: bugzilla, Assigned: bholley)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1b3
fixed1.9.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Updated

10 years ago
Blocks: 45077
Version: unspecified → 3.0 Branch
(Reporter)

Updated

10 years ago
Blocks: 455077
No longer blocks: 45077
(Reporter)

Comment 1

10 years ago
Created attachment 343762 [details]
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.
(Assignee)

Updated

10 years ago
Assignee: nobody → bholley
Blocks: 455056
No longer blocks: 455077
Target Milestone: --- → Firefox 3.1
Version: 3.0 Branch → Trunk
Assignee: bholley → nobody
Component: General → General
Product: Firefox → Core
QA Contact: general → general
Target Milestone: Firefox 3.1 → ---
(Assignee)

Updated

10 years ago
Assignee: nobody → bholley
(Assignee)

Updated

10 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 2

10 years ago
Created attachment 345437 [details] [diff] [review]
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:33-bobbyholley@stanford.edu-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.
(Reporter)

Comment 3

10 years ago
(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.
(Assignee)

Comment 4

10 years ago
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.
(Reporter)

Comment 5

10 years ago
(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?
(Reporter)

Comment 6

10 years ago
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).
(Assignee)

Comment 7

10 years ago
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.
(Reporter)

Comment 8

10 years ago
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.
(Assignee)

Comment 9

10 years ago
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.
(Reporter)

Comment 10

10 years ago
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
(Reporter)

Comment 11

10 years ago
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.
(Assignee)

Comment 12

10 years ago
Created attachment 345981 [details] [diff] [review]
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.
Attachment #345437 - Attachment is obsolete: true
(Assignee)

Comment 13

10 years ago
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:20-bobbyholley@stanford.edu-bholleycmsdetectbogus4/

Still work ok?
(Reporter)

Comment 14

10 years ago
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.
(Assignee)

Comment 15

10 years ago
Created attachment 345993 [details] [diff] [review]
bogus profile detection patch - ready for review

Ok - sounds good.

Uploading a patch without the fprintf, flagging vlad for review.
Attachment #345981 - Attachment is obsolete: true
Attachment #345993 - Flags: review?(vladimir)
(Assignee)

Comment 16

10 years ago
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.
Attachment #345993 - Flags: approval1.9.1?
Comment on attachment 345993 [details] [diff] [review]
bogus profile detection patch - ready for review

a191=beltzner
Attachment #345993 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Comment 18

10 years ago
Created attachment 350412 [details] [diff] [review]
Same final patch as above, Modified with user/commit message

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

Comment 19

10 years ago
Created attachment 350415 [details] [diff] [review]
MQ formatted, proper username
[Checkin: Comment 23 & 26]

Screwed up the username string. Fixed.
Attachment #350412 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/0586ee185c87
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
Backed out to investigate bug 467102:
http://hg.mozilla.org/mozilla-central/rev/a6d8a0bcac50
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
See bug 467102 comment 6 for an explanation about the perf regression.
(Assignee)

Comment 23

10 years ago
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.

Updated

10 years ago
Component: General → GFX: Color Management
QA Contact: general → color-management
Target Milestone: mozilla1.9.1b3 → ---
(Assignee)

Comment 24

10 years ago
Resolving as fixed - marked as needing 191 landing
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [needs 191 landing]
(Assignee)

Comment 25

10 years ago
5e280f6121b1
Whiteboard: [needs 191 landing]
(Assignee)

Comment 26

10 years ago
Sorry for the cryptic comment - that meant to say:

Pushed to releases/mozilla-1.9.1 in 5e280f6121b1

Updated

10 years ago
Duplicate of this bug: 450923
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.1b3
Attachment #350412 - Attachment description: Same final patch as above, Modified with user/commit message. [Check me in]. → Same final patch as above, Modified with user/commit message
Attachment #350415 - Attachment description: MQ formatted, proper username. [Check Me In] → MQ formatted, proper username [Checkin: Comment 23 & 26]
(Assignee)

Updated

10 years ago
Duplicate of this bug: 468589

Comment 29

10 years ago
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.
(Assignee)

Updated

10 years ago
Duplicate of this bug: 479358

Updated

10 years ago
Duplicate of this bug: 469315

Updated

10 years ago
Duplicate of this bug: 487840

Updated

10 years ago
Duplicate of this bug: 497581
You need to log in before you can comment on or make changes to this bug.