53597 - CSS colors are missing gamma correction

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[marlon bishop]

In order to complement the use of gamma correction in PNGs, as well as the 
tremendous benefit of cross platform color matching, we need to implement gamma 
corrected CSS color values as specified in both CSS1 and CSS2.

The omission of this portion of the specification has a detrimental effect on PNG 
use in the production of new Themes.  We need a gamma correction method that 
works the exactly the same throughout the application.

http://www.w3.org/TR/REC-CSS1#color-units
http://www.w3.org/TR/REC-CSS2/syndata.html#color-units

Comment 1

[tor]

*** Bug 53606 has been marked as a duplicate of this bug. ***

Comment 2

[Karl Ove Hufthammer]

From the CSS 2 rec.:

"Conforming user agents may limit their color-displaying efforts to performing 
a gamma-correction on them. sRGB specifies a display gamma of 2.2 under 
specified viewing conditions. User agents should adjust the colors given in CSS 
such that, in combination with an output device's "natural" display gamma, an 
effective display gamma of 2.2 is produced. See the section on gamma correction 
for further details. Note that only colors specified in CSS are affected; e.g., 
images are expected to carry their own color information."

"For information about gamma issues, please consult the the Gamma Tutorial in 
the PNG specification ([PNG10])."

Comment 3

[tor]

The following patch gamma corrects CSS and HTML specified colors along with
GIF colormaps.  This isn't the final version, just something for people to
play with.  The gamma value should really be requested from the platform gfx
instead of hardcoded, and calculation of the gamma correction should be moved
to one function instead of gfx and gifcom both having a copy.

JPEGs are still not gamma corrected - I'll look at that later.

Comment 4

[tor]

Attachment: css/html/gif gamma correction

Comment 5

[tor]

So I can estimate the priority of this, do we care enough about getting
gamma correction working properly to try for 6.0?

Comment 6

[marlon bishop]

nice! i will be sure that this gets looked at, however, since this isn't a 
showstopper i doubt we can get it into B3.  I definitely don't have the final say 
on this, but the tree closes tonight for B3 and anything else that wants in for 
RTM must pass extreme scrutiny.

Which doesn't mean it isn't important. any amount of effort you make will get the 
ball rolling, just don't stay up all night tonight to try to get it in.

thanks for contributing, truly appreciated.

Comment 7

[Greg Roelofs]

On the question of whether this should be a priority for 6.0, my answer is a
resounding "yes."  Not only is this a standards-compliance issue (it is required
for CSS), it is also going to be a somewhat painful change for designers to deal
with, regardless of when it happens--and with all the other major
standards-related changes in 6.0, there will never be a better time to implement
it.

That said, it also needs to be absolutely correct--inverting the exponent would
be even worse than what Adobe did to Photoshop 5.0.  (Insofar as the code
appears to be based on that in libpng and libimg/pngcom, I don't have any
serious doubts--but I've gotten confused enough in the past that I don't trust
anything until I test it personally, and preferably Chris Lilley does, too.) 
The only way to test it effectively is to check on an SGI or NeXT console or on
a stock (non-ColorSync'd-to-gamma-2.2) Macintosh.  I don't have access to any of
those machines.

But many thanks to Marlon for entering this bug and to Tim for supplying the
patch.  This is an important one.

Comment 8

[tor]

Attachment: updated css/html/gif/jpeg gamma correction patch

Comment 9

[tor]

The updated patch attached moves all the gamma logic to one place (nsColor)
and adds gamma correction for jpeg images.  The gamma value is still hardcoded
per platform because I don't have the code needed to query the gamma value
for Win32 or MacOS, and the version for X needs the gfx connection to be
already established.

Though it might be using the wrong gamma value, at least it's now used
consistently for all of css/html/gif/jpeg/[pmj]ng.

CCing [e-mail] for feedback.

Comment 10

[Greg Roelofs]

I don'tremember who told me this, but I understand that Xcms is widely
unimplemented despite having been part of X11 for around a decade or so...

FWIW.

Comment 11

[tor]

For X I was thinking of using XSolarisGetVisualGamma() (solaris) and
XF86VidModeGetGamma() (XFree86) with the appropriate autoconf tricks.

Comment 12

[Greg Roelofs]

Oh, cool--I wasn't aware of either of those.  (Of course, I've used XF86 every
day for the last 6 years, and I've never seen any config setting or helper app
that would allow me to calibrate and set the gamma value...  Is that new?  It
doesn't have a man page on my 3.3.6 test system.)

Comment 13

[tor]

New to XFree86 4.x, I believe.  It's part of the XFree86 X server video
mode extension (XFree86-VidModeExtension - xf86vmode.h).  The program for
examining/setting the gamma value is "xgamma".

Comment 14

[marlon bishop]

bear with me on this, but how can you perform gamma correction on GIF and Jpeg if 
they have no source value.  i thought the whole algorithm was based on the gamma 
of the display device and the source value embedded in the PNG.  

Comment 15

[tor]

Just like how you deal with a [PJM]NG without any gamma information - you
assume a default file gamma value (0.45455).

Comment 16

[Greg Roelofs]

Marlon, this is something you have to do because of existing practice. 
Designers implicitly assume that their (unlabelled) GIF and JPEG images are in
the same color space as CSS and HTML colors (that is, they assume that the
colors will match when they place images on background colors).  Since HTML and
CSS colors are defined to be sRGB, this means that GIF and JPEG images are
implicitly sRGB, also.  In fact, this is precisely what the International Color
Consortium (the folks behind sRGB and ICC profiles) recommends, and I'm sure
existing practice is the reason behind the recommendation.

Consider the alternative if that doesn't convince you:  designers will go
absolutely apeshit if their carefully matched image+HTML/CSS combos suddenly
stop matching even on truecolor displays--ESPECIALLY since most of them seem to
use Macs, where it will really show up dramatically.

Comment 17

[Hixie (not reading bugmail)]

dbaron: I guess this explains the bug with the WaSP image from ~18 months ago...

Greg: We will need a thorough testcase for this, are you aware of any?

Comment 18

[Greg Roelofs]

I don't have a definitive test case (or set of them), but here are two GIF-based
ones to get you started (both from the recent Webmonkey article at
http://hotwired.lycos.com/webmonkey/00/37/index2a.html):

short version: 
http://hotwired.lycos.com/webmonkey/00/37/stuff2a/complete_websafe_216/reallysafe_palette.html

full version: 
http://hotwired.lycos.com/webmonkey/00/37/stuff2a/complete_websafe_216/test.html

It would be trivial to convert the GIFs in these to PNGs, either with or without
an sRGB chunk or the corresponding gAMA and cHRM chunks (newer versions of
libpng will treat the sRGB and gAMA+cHRM cases equivalently).  I may have time
this weekend to copy and convert the big page; if so, I'll make it available
from the PNG site.

Btw, I believe the ICC has defined some means of including ICC profile info in
both GIF and JPEG images, so my earlier comment about GIF and JPEG images being
implicitly sRGB should have qualified that as "_unlabelled_ GIF and JPEG
images."  I'm not certain, but I suspect Photoshop 5.5 may embed full ICC
profiles in all of its saved images.  (The iCCP chunk it puts in PNG images is
broken, however.  I believe libpng 1.0.8 detects and works around that breakage,
probably with an error message.)

Comment 19

[Greg Roelofs]

OK, I've created four copies of the "websafe" table here:

	http://www.libpng.org/pub/png/colorcube/gamma-consistency-test.html

One is the original GIF version (with some minor changes noted in the comments
at the top); one is the corresponding PNG version (unlabelled); one is a PNG
version using gamma 1/2.2 PNGs; and one is a PNG version using sRGB PNGs (which
also include a gamma 1/2.2 chunk--I haven't decided if that's good or bad in
this context).

The link above indicates what tests still need to be created. I'll work on those
as time permits, but if there's a CSS whiz around who can sketch out the basic
method of converting loose HTML 4.0 colors to strict HTML 4.0 + CSS, it would
save me some time perusing the specs.

Comment 20

[marlon bishop]

cc'ing joe hewitt, might be able to help us out

Comment 21

[tor]

Nominating for RTM decision, as this needs to be fixed before mozilla/Netscape
can in good faith claim full PNG support.

Comment 22

[Greg Roelofs]

And CSS-1 support!  Gamma correction is not optional for CSS conformance.

Btw, I've completed (or at least fleshed out) my gamma test suite [URL field
above]; there are now 10 cases:  GIF/HTML, PNG-nogamma/HTML, PNG-gamma16/HTML,
PNG-gamma22/HTML, PNG-sRGB/HTML, and five corresponding cases using CSS colors. 
All test cases have been validated for both CSS and HTML 4.0, I believe; in any
case, there are buttons at the top of each page to do that.

Note that testing on Windows/x86 or Linux/x86 won't do much, aside from
verifying Tim's patch; where problems will really stand out is on Macs or SGIs. 
I don't have access to either.

Also note that these pages trigger a *seriously* ugly performance problem in
Mozilla, at least on Linux--perhaps someone more familiar with its current
status knows of an open bug already.  With images already in the cache,
rendering time is roughly two orders of magnitude slower than NN 4.x, and
scrolling time is two to three orders of magnitude worse.  Mozilla is basically
unusable on these pages on a PII-266.

Comment 23

[tor]

Greg, if you want to test what happens on the Mac, just add a #define/#undef
XP_MAC pair around around NS_DisplayGammaValue.  That's how I tested the
patch when creating it on Linux.

Comment 24

[marlon bishop]

greg, thanks for that comprehensive test suite.  I tested on an unpatched current 
mac NS6 build and saw no color matching in every case except for GIF on HTML.  I 
am trying to get tor's patch installed for the mac and will test again

Comment 25

[marlon bishop]

Attachment: gamma labelled png2.2 on css

Comment 26

[marlon bishop]

Attachment: no gamma labelled png on css colors

Comment 27

[marlon bishop]

notice how even unlabelled pngs aren't matching css colors on the mac, because of 
that default value

Comment 28

[Pierre Saslawsky]

First a big THANK YOU, Tim, for doing that work...

A problem though: I feel very strongly that the "LUT_exponent" should be 
implemented in the platform-specific parts of GFX, not as #ifdefs in the Image 
Library. It would be nice too function to put comments in each platform-specific 
"GetLUTExponent()" that list what is the returned value on other platforms, just 
in case we later need to do some fine-tuning.

Comment 29

[tor]

One reason I haven't moved the display gamma query into the heart of platform
gfx is that in order to return an actual queried values the gfx system will
need to be initialized.  I'm thinking that GetColorValue() is probably being
called early in the startup before gfx is started.  True?

Comment 30

[marlon bishop]

I really want this to make it into RTM, but not sure if it is going to happen.
We can't even attack PDT with it until we've got a patch here.  cc'ing Eric
Krock, since we can't promote "full" CSS1 compliance anymore

Comment 31

[tor]

Err... we do have a patch (9/22 attachment), currently under review.

Comment 32

[tor]

Attachment: gamma correction patch with platform gamma query

Comment 33

[tor]

pierre: I've attached an updated patch that adds a GetGammaValue to nsIScreen.
Is this the sort of thing you were looking for?

Comment 34

[Pierre Saslawsky]

It looks better but you will have to implement GetGammaValue() on all the 
platforms, not just GTK.

I'm not the best guy to review this patch: Pam Nunn and Don Cone are the gfx 
specialists.

Comment 35

[tor]

Attachment: updated patch with more platform GetGammaValue()s

Comment 36

[tor]

One thing to note - this patchkit assumes that the gamma value will remain
fixed throughout the lifetime of the application.  This is probably a
forgivable assumption except maybe for multi-screen MacOS boxes.  If we
want to handle changing gamma on the fly, things get more involved:

  * gamma correction for colors will be moved up to platform gfx (setcolor)
  * system css colors will have to be pushed through an inverse gamma
      transform
  * need some way of invalidating all images so that they can be uncompressed
      and gamma corrected again (we don't want to run them through multiple
      gamma correction stages because the operation is lossy).

Comment 37

[Stuart Ballard]

Is anyone still working on this for RTM? For all the reasons described in this
bug, it's important for 6.0... but if it isn't going to happen it should
probably be rtm-'d

Failing that, nominating for mozilla1.0 in hopes of getting it onto the right
people's radars.

Comment 38

[tor]

The patch is in review limbo - I've asked for reviews, but haven't received
anything except the suggestions from pierre.

Comment 39

[Brendan Eich [:brendan]]

I started a review the other week, then had a crash while composing my comments. 
I've just now restarted the process via IRC with tor, who is preparing a new 
patch.  Sorry for the delay.

/be

Comment 40

[tor]

Attachment: another update to the patch (see comment)

Comment 41

[tor]

Updated patch based on a conversation with brendan.  Changes:

  * check screen is valid in InitializeGamma
  * rename static NS_InitializeGamma to InitializeGamma
  * use appropriate class in platform nsScreen*::nsGammaValue

Comment 42

[Greg Roelofs]

Looks good to me.  I'd invert the "double gamma" calculation in
InitializeGamma(), though--the 0.45455 is OK for PNG since PNG actually stores
the file gamma value as a fixed-point integer (45455), but for floating-point
calculations, you may as well be precise:

	double gamma = 2.2/gammaValue;

Also, for the two places where you hardcode the SGI value (1.7), I've got code
that can read that from the relevant system config file, if you wish:

    *aGammaValue = 2.2/1.7;
    FILE *infile = fopen("/etc/config/system.glGammaVal", "r");
    if (infile) {
        char tmpline[80];

        fgets(tmpline, 80, infile);
        fclose(infile);
        double sgi_gamma = atof(tmpline);
        if (sgi_gamma > 0.0)
            *aGammaValue = 2.2/sgi_gamma;
    }

And in the same two places, you could add an ifdef for NeXT, on the off-chance
that that platform ever gets a port:

    #elif defined(NeXT)
      *aGammaValue = 1.0;

You never know...

Greg

Comment 43

[tor]

Attachment: yet another patch update (see comment)

Comment 44

[tor]

Changes this time:

  * change from 1/0.45455 to 2.2 in InitializeGamma
  * adds a "gfx.gamma" pref that overrides the platform GetGammaValue().  This
     pref is a character string, as the preference service doesn't support
     floats
  * add sgi and NeXT gamma code suggested by Greg

Comment 45

[tor]

BTW: ignore the MINIMUM_DELAY_TIME bit of the gif.cpp change - that's the
patch from 55997 that got accidentally included when I made the diff.

Comment 46

[selmer (gone)]

rtm-, not a stop ship.

Comment 47

[Stuart Ballard]

Argh! I thought this was ready in plenty of time for rtm... what happened?

(well, I guess I know what happened... review limbo for too long. But why? I
didn't see any negative reviews, and the latest comments suggest that the
current patch reflects all the reviews...)

Comment 48

[tor]

Attachment: another patch with a couple minor tweaks (see comment)

Comment 49

[tor]

Another patch has been attached based on the latest feedback from Brendan.
Changes this time:

  * remove the extra space in "result =  prefs->CopyCharPref(..."
  * remove extra parens in "gammaRamp[i] = (unsigned char)(val);"
  * use sizeof(tmpline) instead of 80 in nsScreen{Gtk,Xlib}::GetGammaValue()
  * add nsScreenBeOS::GetGammaValue()
  * remove MINIMUM_DELAY_TIME patch from 55997 that was accidentally included
      in the last patch

Comment 50

[tor]

Attachment: one last tweak to make jpeg gamma correction optimizer friendly (brendan's suggestion)

Comment 51

[marlon bishop]

is it possible to predict if this will now cause any perceivable lag in display?  
it is especially critical to UI/theme elements display, which is already too 
slow.

also, how will this effect the display of modern, which is a GIF skin?  some were 
worried that fixing this now would break modern.  and, is there a way to turn it 
off?

Comment 52

[tor]

It will cause gif and jpeg image loading to be a tiny bit slower.  Once the
images come out of the image cache (like the chrome) there is no change in
speed.  Someone from the layout/style group could probably give a ballpark
estimate about how often ns{CSS,HTML}Value::GetColorValue() gets called.

modern2 changes its apparent brightness based on the gamma value returned.
I've run with a wide range of gamma values and haven't noticed any color
mismatches due to gamma correction.  You can override the gamma correction
to a linear ramp by adding "user_pref("gfx.gamma", "2.2");" to prefs.js.

Comment 53

[Stuart Ballard]

As I understand it, there is a simple reason why this change should not break
any existing gif-based skins...

The current behavior is:
- GIFs and CSS colors are NOT gamma-corrected
- PNGs with gamma ARE gamma-corrected
- PNGs without gamma ARE gamma-corrected with a default gamma, I *think*.

This patch causes the following behavior:
- GIFs, CSS colors and gamma-less PNGs are gamma-corrected with a default gamma
value.
- PNGs with gamma are gamma-corrected with the value they specify.

So both before *and* after, GIFs will match CSS colors because they are treated
the same. The only difference is that it is now possible to reliably persuade
PNGs to also match.

Comment 54

[Greg Roelofs]

I believe Stuart's analysis is accurate--that is, various chrome colors will
shift in the same way on some platforms, not causing any visible mismatches--but
there may be a slight change in the appearance from what the designers
originally intended.  That is, based on Nikhil Bhatla's <[e-mail]>
comments in mozilla.ui (which I think are what led to this bug being filed), the
GIF icons were tuned on non-sRGB Macs ("our development platform") without
CSS/HTML gamma correction, so with gamma correction turned on, they'll look
somewhat darker than that across all platforms.

Now, it doesn't sound like the absolute brightness level was a major concern (at
least, not in that thread), and the appearance on Windows and Linux/x86 boxes
is/was always that "somewhat darker" shade, so I don't *think* this should
bother anyone too much.  But Mac users with old and new versions of Mozilla
running side-by-side will be able to see a difference between the two.

Comment 55

[tor]

Attachment: another patch - add DEFAULT_CRT_GAMMA define and comments for GetGammaValue()s (waterson feedback)

Comment 56

[Greg Roelofs]

I meant to comment earlier, but I've been too busy.  The DEFAULT_CRT_GAMMA
change is a bad idea for two reasons:  (1) all CRTs have essentially the same
"gamma" value (and most of the world agrees it's close to 2.2, though Poynton
stubbornly insists 2.5); and (2) it's used as the default value for a variable
called "gammaValue" and later "gamma", which is completely misleading.

In case it wasn't obvious from the PNG changes, the term "gamma" is overused and
overloaded with meanings, with Apple, SGI, camera makers and everybody else all
defining it incompatibly or, worse yet, just using the term without ever
defining what they mean.  The split into CRT_EXPONENT and LUT_EXPONENT may have
seemed pedantic overkill, but at least it has the benefit of forcing one to
think about what's really changing across platforms (the lookup table, not the
CRT or other display device), and it has a clearly written tutorial to back it
up (http://www.libpng.org/pub/png/spec/PNG-GammaTutorial.html).

I don't have specific suggestions for fixing this, but I think the previous
patch was closer to being on track.

Greg

Comment 57

[tor]

Attachment: updated patch - removed DEFAULT_CRT_GAMMA

Comment 58

[tor]

Ok, I've removed the DEFAULT_CRT_GAMMA definition until someone really
comfortable with gamma can help us clarify our terminology.

Comment 59

[Brendan Eich [:brendan]]

My only comment is that you can optimize the GIF loop to avoid reloads from
memory in the loop control required due to memory ambiguity:


RCS file: /cvsroot/mozilla/modules/libimg/gifcom/gif.cpp,v
retrieving revision 1.35
diff -u -r1.35 gif.cpp
--- gif.cpp     2000/11/06 20:08:29     1.35
+++ gif.cpp     2000/11/10 16:56:17
@@ -1047,9 +1047,9 @@
 #endif /* M12N */
             for (i=0; i < gs->global_colormap_size; i++, map++) 
             {
-                map->red = *q++;

Put gs->global_colormap_size in a limit register.

I think this patch has many r='s, and at waterson and I have sr='d it.  That's
more than enough to get checked in, I think.  Anyone object?  Speak fast.

/be

Comment 60

[tor]

I think this is an unneeded optimization - you're attempting to speed up the
control of a loop which is exectuted infrequently, runs for <= 256 iterations,
and contains a number of function calls and memory lookups.

The last I heard (Nov 8), buster was looking at these changes.

Comment 61

[Brendan Eich [:brendan]]

Oh, ok -- I am old-school and hard-core -- I always save code space and runtime,
at the expense of a little source complexity, with such loop control memory
disambiguation.  But that's just me.

Sorry about that, buster -- can you r= soon?

/be

/be

Comment 62

[buster]

the change looks fine to me as far as layout is concerned.  I'm no expert in
this area, though.  I'd feel more comfortable if someone from gfx-land also
reviewed.  Kevin, Don?

But if waterson and/or be think that enough knowledgable folks have reviewed and
approved already, I'm fine with it as is.

Comment 63

[Kevin McCluskey (gone)]

How does this patch relate to the gamma correction code which already exists in
S:\mozilla\gfx\src\nsDeviceContext.cpp and
S:\mozilla\gfx\src\windows\nsRenderingContextWin.cpp?

Does it replace it?
The existing code does the gamma correction by adjusting the color value passed
to nsRenderingContext::SetColor method when setting the current color. The patch
adjusts the value returned for CSS colors.
If the color value is set through the CSS OM then retrieved the CSS color will
not be same as what is passed in. It will be corrected for gamma. Is this the
correct behavior?

Comment 64

[tor]

It essentially replaces the existing gamma code (which is only hooked up
half-heartedly for win32).  The current code only supported correction for
colors, and did it at the gfx endpoint where it would mess up system CSS
colors.  Nothing in mozilla ever set the gamma value used by this code to
anything other than the identity map.

Ian Hickson is a better person to answer the CSS DOM question - I don't
pretend to be a CSS expert, even in bugzilla.

Comment 65

[Hixie (not reading bugmail)]

It is my understanding that the CSS OM _is_ expected to roundtrip colour values,
so we should not be changing them.

In CSS terms, as I understand it the specified and computed values in the CSS OM
should not be gamma corrected, only the _actual_ values should be gamma
corrected. (It is not possible using the CSS OM to get to the actual values.)

David should correct me if I am wrong on this...

Comment 66

[David Baron :dbaron:]

It all depends on which is the specified value, which is the computed value, and
which is the actual value.  This is an area where the spec is in general
hopelessly vague.  However, I would expect (not according to the spec but
according to good sense) that the CSSOM would return the original value so that
the color returned could be reused somewhere else.  However, I think the
stronger argument in the spec is for the reverse - from sections 6.1.2 and 6.1.3
say that the difference between computed and actual values is only
approximations such as rounding to the nearest pixel and dealing with
unavailable fonts.  However, one could easily make the counter-argument that the
gamma-correction is something that should be handled by a layer lower than the
CSS rendering system...

Perhaps I should make a plea to the WG that CSS3 should include clear statements
throughout of which value is the computed value...

Comment 67

[buster]

Kevin makes a great point about round-trip fidelity, and I think David's first
instinct is correct.  It seems odd and unreasonable for an author to get a
different value through the CSSOM than was specified in the style rule.
After talking with Kevin, I'm convinced that, as much as possible, gamma
correction ought to happen in the output system.  Layout and style should be
oblivious.  

Comment 68

[tor]

Ok, then how should we handle system CSS colors?  Presumably (at least, this
is how mozilla handles it) they are returned as color values that can be
written directly into the framebuffer without correction.  Should they be
pumped through an inverse-gamma function before the CSS system sees them?

Comment 69

[tor]

Note that the system css color -> inverse-gamma -> gamma -> output conversion
is lossly, so the colors won't match exactly.

Comment 70

[tor]

The problem is that the style system and gfx/widget are currently locked
together - when the style system turns a css rule into a nsStyleColor it
calls nsLookAndFeel::GetColor() and stores the result as a raw color.  Since
the value returned is a framebuffer value it shouldn't be gamma corrected.
Adding a gamma flag to nsStyleColor or otherwise indicating that the color
should be evaluated lazily is likely to raise the ire footprint team.
Additionally nsStyleColor is a raw structure without accessor methods, so
all the users of it would need to be modified to do the right thing.

Any style people want to offer opinions on how to proceed?

Comment 71

[David Baron :dbaron:]

What if the gamma correction or the system color to real color lookup are both
done at the same point, sometime between extracting the value from the DOM and
rendering it?  In other words, keep the knowledge of both gamma correction and
of the actual system colors away from layout and the style system.

Comment 72

[Stuart Ballard]

Would it be possible to put some kind of internal flag on system colors in the
CSSOM? Or does CSSOM return colors as pure RRGGBB values?

Presumably CSSOM can at least *extract* the RRGGBB values, and if so, it should
probably retrieve inverse-gamma'd values. However, a flag that would at least
eliminate the issue if the result of a getColor() was used in a setColor()
(hypothetical methods since I don't know CSSOM) would cover 99% of the cases.

Additionally, on a 24bit display it would be unlikely that the difference would
be detectable by the human eye even with an off-by-one error in the R, G and B
components, so I don't think that even the worst case is disastrous.

Could we at least get a patch in place for NON-system colors (with perhaps
slightly imperfect behavior on the system colors) and then file separate bugs to
get the system color handling perfect? It seems a shame to me that this patch is
rotting.

Comment 73

[tor]

*** Bug 48772 has been marked as a duplicate of this bug. ***

Comment 74

[tor]

*** Bug 48772 has been marked as a duplicate of this bug. ***

Comment 75

[tor]

*** Bug 65356 has been marked as a duplicate of this bug. ***

Comment 76

[[e-mail]]

Off-by-one errors aren't visually acceptable. Been there.
dbaron's suggestion seems sound. How hard is this to do? Re-architect the whole
image system, or just a few tweaks?

Comment 77

[David Baron :dbaron:]

[Retyping a comment I lost by hitting back in a mid-air collision.]

It's not a huge change, but it would probably lead to a patch a number of times
larger than the current one.  What it would probably require is defining a
second color type that would hold system colors as constants rather than color
values and would never be gamma-corrected.  We could then move the
gamma-correction and system-color lookup to the site where we convert from one
type of color value to the other.

Comment 78

[tor]

Disowning.

Comment 79

[Dan Rosen]

Is this seeing work (with libpr0n, maybe? pavlov?)... If not, this should
probably be retargeted at moz0.9...

Comment 80

[chris hofmann]

time to decide on this...  ready to go for m0.8 or no???

Comment 81

[Brendan Eich [:brendan]]

tor disowned this bug, so I'm moving its TM to 0.9 and pleading with someone on
the cc: list to take ownership.  Saari?  Pavlov?

/be

Comment 82

[Hixie (not reading bugmail)]

Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...

Comment 83

[tor]

Attachment: updated patch

Comment 84

[tor]

Updated patch - moves color gamma correction into gfx.  System colors are now
gamma corrected.  I'll commit to writing a SetUncorrectedColor for the various
platforms if someone teaches the style system to use it.

Comment 85

[tor]

Attachment: merge to tip

Comment 86

[chris hofmann]

tor, have you reclaimed?  
moving nobody's bugs off the 0.9 list.
somebody assign it the right milestone

Comment 87

[chris hofmann]

moving nobody's bugs off the 0.9.1 milestone list.

Comment 88

[marlon bishop]

tor, Pav? what's up with this one? it's my favorite bug of all time, i hope we
can get it in.  Themes are patiently waiting for relief, as are billions of
designers out there 

Comment 89

[tor]

I need some help from the style people to prevent system css colors from
being gamma corrected.  I'm willing to write the required platform gfx
code, but need assistance hooking it into the style system.

Comment 90

[Brendan Eich [:brendan]]

Mebbe hyatt, who is hacking on the style system, can help tor here.

/be

Comment 91

[Stuart Ballard]

Any update on this? Did hyatt respond to Brendan's request offline? If not, are
there any other style system gurus who could step in here?

#include <std-we-need-to-clone-hyatt.h>

I believe it's really important for this fix to make it into 1.0. Since tor
appears to have most of the work done, it'd be really sad to see this feature
miss 1.0 for lack of style people stepping up to help.

Comment 92

[Kevin McCluskey (gone)]

CC'ing [e-mail] for style system help.

Comment 93

[Marc Attinasi]

I'll try to help with the style systems issues. From what I can see, we need to 
store a bit on each nsStyleColor to indicate if it is a system color or not, and 
then somebody decides based on that flag whether or not to gamma correct the 
value, right? If that is the case, then we either need a small bitfield on the 
nsStyleColor struct, or we need to create some kind of external mapping of 
nsStyleColor instances that represent system colors (yech!@#$*#@). 

I'd like to do some measurements of how many nsStyleColor instances there are on 
a given set of pages (and extrapolate the extra bloat required for the flags) 
and maybe also how many nsStyleColor instances actually represent a system 
color.

Or, am I missing something here? (sorry, I'm not Hyatt's clone so I need a 
little education)

Comment 94

[Pierre Saslawsky]

Taking the bug

Comment 95

[Pierre Saslawsky]

Moving again.... 0.9.6

Comment 96

[Pierre Saslawsky]

For the record, in response to an offer from Tim Rowley to take over the bug, I 
wrote:
----
Here are some pointers from a style system standpoint.  I haven't looked into
what we should do in ImageLib or GFX to not correct the already
gamma-corrected colors.

I wanted to implement Marc's idea of an additional bitfield that indicates
whether the color is an already gamma corrected system color (comment #93).

These colors are obtained through nsILookAndFeel::GetColor().  See in particular:
  http://lxr.mozilla.org/seamonkey/source/content/base/src/nsRuleNode.cpp#355

The work was originally fairly straightforward but it was a bit complicated
by Dave Hyatt's rule tree landing.  See the following comment:
http://lxr.mozilla.org/seamonkey/source/content/shared/public/nsStyleStruct.h#160

Depending on the changes you are going to make, please update the patch for
bug 77991 or mark it invalid:
  http://bugzilla.mozilla.org/show_bug.cgi?id=77991

Before checking in, you will probably be asked for an estimate of the
increase in terms of memory footprint

Comment 97

[David Baron :dbaron:]

This is probably one of the few standards compliance bugs that should really be
a mozilla 1.0 blocker, so I'm a little disappointed to see it moved off to
mozilla 1.2.

Comment 98

[Pierre Saslawsky]

Ooops, sorry: I'm not familiar enough with imaging and gamma correction to make 
the call on how important the problem is for users who have an interest in it.  I 
pushed it to moz1.2 because Tim Rowley offered to take the bug and I thought that 
if he wasn't done by then, I would do it.  Another reason was the low-priority 
Hixie-P3 status.  Back to moz1.0 as a "DBaron-P1" :)

Comment 99

[tor]

While adding a flag on nsStyleColor to indicate if a color needs gamma
correction seems fine from a bloat point of view (browsing a few pages
only seemed to generate a hundred or so nsStyleColors), tracking down
all the uses of nsStyleColor to call the appropriate method on the
rendering context is going to be a bit of a nightmare.

In the gross hack category to minimize the number of changes, we could
hide the flag in the alpha channel of the nscolor .  Poking through the
tree, nobody seems to be seriously using the alpha channel of the nscolor.
It's used in nsDocShell and nsViewManager to check for unset values; and
in nsCSSValue and nsHTMLValue when converting to a string.

Comment 100

[Al]

I read thru these ramblings and don't still don't know why, in theory, the color
space should be rendered with any modification at all.

Rendering of html and css colors, yes that requires gamma consider, etc.
However, the answer is simple [unfortunately] just replicate IE.  90% of the
browsers are IE, so designers pick colors that look good with IE; period.  

As I understand it, PNG is a compression algorithm.  Therefore, the
decompression algorithm should repoduce the orginal, without any modification.  

Al.............

Comment 101

[Kevin McCluskey (gone)]

Moving to Mozilla1.1. Engineers are overloaded with higher priority bugs.

Comment 102

[Kevin McCluskey (gone)]

Bulk moving from Moz1.1 to future-P1. I will pull from this list when scheduling
work post Mozilla1.0.

Comment 103

[Kevin McCluskey (gone)]

Changing the CSS colors to be gamma corrected should not be changed this close
to Mozilla1.0.  The gamma correction would have to turned off by default anyway
to maintain backward compatibility with both IE and Nav4.x. 

nsbeta1-.

Comment 104

[Greg Roelofs]

With regard to comment 103, I _hope_ you meant "would have to turned off by
default anyway [if it were fixed this close to release]."

God forbid we should ship a standards-compliant browser.

Greg

P.S.:  Read the rest of the bug.  The entire point of gamma-correcting PNGs
_and_ MNGs _and_ JNGs _and_ GIFs _and_ JPEGs _and_ HTML colors _and_ CSS colors
[and anything else that has color values other than 0 or 255] is to preserve
color-matching-by-eye as practiced by generations of web designers using MSIE
and NN 4.x and so on.  IOW, different color-things on any given page will not
suddenly become mismatched if they were matched before (except for PNGs with
stored gamma values, but fixing that bug is one of the goals of this bug, and
PNGs that *should* have matched but didn't previously, now will).  A
side-by-side comparison on a Macintosh of an old browser and the fixed Mozilla
would look different, but the same test on 99.9% of Windows and Linux/x86
machines would look identical.  And a comparison of the fixed Mozilla on Mac and
on Windows would suddenly match (assuming the system gamma values are known or
guessed correctly), which should make most designers MUCH happier.  (Note that
even on a misconfigured system, the colors on the web page will be
self-consistent; only a side-by-side comparison with an old/broken browser would
show the difference.)

Comment 105

[tor]

Like Greg says, if implemented correctly this won't alter the
consistency of any page (except those that had PNGs, which were
already inconsistent), and there isn't any reason it couldn't be
turned on by default.

drivers@mozilla are interested in getting this issue addressed for
mozilla1.0, both for standards correctness and to encourage the uptake
of PNG and related formats.

As always, the complicating factor is system css colors.  A possible
low-impact patch is to push the system colors through an inverse gamma
mapping when they are obtained, then do gamma correction on all colors
passing through gfx for output.  This of course will introduce some
error into system color reproduction, but hopefully at a fairly
imperceptible level.

I'll try to come up a patch for this worse-is-maybe-better solution
this weekend.

Comment 106

[tor]

Attachment: merge to tip

Comment 107

[Brendan Eich [:brendan]]

Comment on attachment 74548 [details] [diff] [review]
merge to tip

> 	/**
> 	 * The color used to paint with.
> 	 */
>-	attribute nscolor color;
>+    void SetColor(in nscolor aColor);

Match the rest of that .idl file, and Mozilla house IDL style: setColor, not
SetColor.

Expand tabs to 4-space equivalents if you can stand the cvs blame, while you're
there.

>@@ -298,8 +297,10 @@
> 		if (mCurrentFont == nsnull) mCurrentFont = (BFont *)be_plain_font;
> 		
> 		mView->SetFont(mCurrentFont);
>-		mView->SetHighColor(mGammaTable[NS_GET_R(mCurrentColor)],
>-			mGammaTable[NS_GET_G(mCurrentColor)], mGammaTable[NS_GET_B(mCurrentColor)], 255);
>+		mView->SetHighColor(NS_GammaCorrectComponent(NS_GET_R(mCurrentColor)),
>+                        NS_GammaCorrectComponent(NS_GET_G(mCurrentColor)),
>+                        NS_GammaCorrectComponent(NS_GET_B(mCurrentColor)),
>+                        255);

Mebbe indent underhanging args so they line up with first one's initial column.

>+++ gfx/src/gtk/nsRenderingContextGTK.h	16 Mar 2002 19:20:10 -0000
>@@ -106,7 +106,7 @@
>   NS_IMETHOD GetLineStyle(nsLineStyle &aLineStyle);
> 
>   NS_IMETHOD SetColor(nscolor aColor);
>-  NS_IMETHOD GetColor(nscolor &aColor) const;
>+//  NS_IMETHOD GetColor(nscolor &aColor) const;

Any reason to comment out rather than remove?

>+++ layout/html/base/src/nsImageFrame.cpp	16 Mar 2002 19:22:06 -0000
>@@ -1165,6 +1165,7 @@
>     }
>   }
> 
>+/*
>   // if we could not draw the image, then just draw some grafitti
>   if (!iconUsed) {
>     nscolor oldColor;
>@@ -1175,6 +1176,7 @@
>                                   NS_STATIC_CAST(int,(size/2)-(2*p2t)),NS_STATIC_CAST(int,(size/2)-(2*p2t)));
>     aRenderingContext.SetColor(oldColor);
>   }  
>+*/

#if 0, don't comment out, if you can't just remove -- but remove if possible
(cvs will remember).

>+++ embedding/browser/webBrowser/nsWebBrowser.cpp	16 Mar 2002 19:22:22 -0000
>@@ -1583,11 +1583,11 @@
>   case NS_PAINT: {
>       nsPaintEvent *paintEvent = NS_STATIC_CAST(nsPaintEvent *, aEvent);
>       nsIRenderingContext *rc = paintEvent->renderingContext;
>-      nscolor oldColor;
>-      rc->GetColor(oldColor);
>+//      nscolor oldColor;
>+//      rc->GetColor(oldColor);
>       rc->SetColor(browser->mBackgroundColor);
>       rc->FillRect(*paintEvent->rect);
>-      rc->SetColor(oldColor);
>+//      rc->SetColor(oldColor);
>       break;
>     }
> 

Ditto -- #if 0 if you can't remove -- why not remove?  A major comment
explaining the new approach should be added if you #if 0.

Fix these, and I'll sr= when there's an r=.  Thanks for doing this,

/be

Comment 108

[David Baron :dbaron:]

Where's the inverse-gamma-for-system-colors code hiding?

Comment 109

[tor]

That patch wasn't ready for review - it was just a checkpoint.  As dbaron says,
it doesn't have the system css color modifications, plus I'm not sure if we
should remove GetColor.

Comment 110

[tor]

Attachment: system color inverse gamma, macroize common trivial functions, modify init

Comment 111

[David Baron :dbaron:]

Review of attachment 76346 [details] [diff] [review]:

Although this patch has gotten a good bit of review before, I'd be a
little more comfortable if someone a little more familiar with GFX /
image code said that all the corrections were being made in the right
places.  However, I guess that testing with a non-2.2 gamma value would
show any bugs there (this should probably be done on all the different
gfx ports, although perhaps it's not worth the bother for those that
always use 2.2), although not necessarily potential performance problems
of doing it at a different stage than the optimal one (although it seems
like that's already been thought through, so I'm more worried about
potential missed cases somewhere).

(Did you check that the Windows code is actually always using the
gamma-corrected colors when it should be?  It jumps through quite a few
hoops with mCurrentColor vs. mColor.)

I'd rather see the NS_InitializeGamma call made from something like the
GFX module constructor, where it will only happen once, and isn't tied to
the docshell code, which seems like an odd dependency.  However, I think
the gfx module constructor itself won't actually work since the GetService
to get the screen manager wouldn't work yet.  Perhaps the layout module
constructor would be better?

I'm a little worried about everything linking OK with your global
|nsGammaRamp| and |nsInverseGammaRamp| and some of the stricter linkers
on the ports tinderbox, but I guess it should be OK...

In NS_InitializeGamma, these casts should be to PRUint8 (which is
probably guaranteed to be |unsigned char|) for consistency:
+      nsGammaRamp[i] = (unsigned char)val;
+      nsInverseGammaRamp[i] = (unsigned char)val;

Also, how can you possibly end up with |val>255.5| in either case?
Since you can't, why bother with the > 255.0 check?  (Or do you just
want an assertion of some sort?)

Also, any reason you're using |exp| and |log| to build the inverse gamma 
ramp rather than just using |pow| to the |1/gamma|?

The nsScreenQT.cpp change should probably look like the GTK/Xlib one,
although it might be nice (although probably not worth it for something
this size, although that code has the potential to grow) to consolidate
that somewhere (in a new file in gfx/src/x11shared/, which seems to
have a fun build mechanism, perhaps?).

The nsScreenMac.cpp change is inconsistent with what the imgContainerMNG
/ nsPNGDecoder code was doing -- shouldn't it return 2.2 * 1.8 / 2.61 ?
(Or is there an API for determining the right value?)  Isn't Mac the
major platform that's supposed to be affected by this change?

I think the nsXPLookAndFeel.cpp change deserves a clear comment
explaining why you're doing the inverse gamma correction and why it's
bad (although only because of rounding errors).

It seems like the nsRenderingContextXlib.cpp and
nsRenderingContextWin.cpp changes undo one of the things that code was
originally doing, which was setting the alpha bits of the color to 255.
It's probably worth going back to what the original code did.  (Also, in
the windows code, is the RGB macro guaranteed to be the same as NS_RGB?
It sounds to me like a Windows macro.)

Any reason you prefer using the gamma ramp directly (by copying the
pointer) in the nsJPEGDecoder changes rather than using the macros?
Might this actually compile to something faster since the pointer to the
gamma ramp will be in a local variable rather than another library -- or
do most optimzers handle this properly?

Other than that, r=dbaron.

Comment 112

[tor]

Attachment: address dbaron's comments

This patch addresses all your comments except for two:

  * nsScreenQT::GetGammaValue left as-is, since Qt is a cross-platform toolkit

  * NS_InitializeGamma left in docshell since I'm not sure where to move it

Comment 113

[David Baron :dbaron:]

Comment on attachment 76441 [details] [diff] [review]
address dbaron's comments

OK, r=dbaron, but:

 * I still think the layout module constructor would be better (Initialize in
layout/build/nsLayoutModule.cpp)

 * Do we use our Qt GFX port on anything other than Unix?  (There's GTK+ for
Windows too...)

Comment 114

[tor]

Attachment: flesh out nsScreenQT::GetGammaValue, move NS_InitializeGamma to layout

Comment 115

[Henri Sivonen (:hsivonen)]

>Isn't Mac the major platform that's supposed to be affected by this change?

Yes.

The Mac gamma is hard-coded in the patch, but 1.8 is only the default on Mac OS
/ Mac OS X. The user can calibrate the display differently in which case
hard-coding the default backfires. Getting the gamma value from the OS or at
least making configurable via a pref would be better than hard-coding.

Comment 116

[David Baron :dbaron:]

After reading

http://www.inforamp.net/~poynton/notes/colour_and_gamma/GammaFAQ.html#Macintosh
http://www.inforamp.net/~poynton/notes/color/GammaFQA.html
http://developer.apple.com/techpubs/quicktime/qtdevdocs/REF/whatsnewqt5/Max.70.htm

it seems to me that the system setting for gamma actually causes QuickDraw to do
the correction, and we're just hard-coding for the default correction, which is
not done on other systems.

Anybody more knowledgable want to comment?

Comment 117

[tor]

Request for an implementation of nsScrenMac::GetGammaValue() is bug 75133.

Comment 118

[Brendan Eich [:brendan]]

Comment on attachment 76464 [details] [diff] [review]
flesh out nsScreenQT::GetGammaValue, move NS_InitializeGamma to layout

>+// Gamma correction
>+extern PRUint8 nsGammaRamp[256], nsInverseGammaRamp[256];

Do you need PR_EXPORT_DATA(PRUint8) instead of extern PRUint8 here, and
PR_IMPLEMENT_DATA(PRUint8) in the .cpp file, for some platforms?

>+
>+double  NS_DisplayGammaValue(void);
>+void    NS_InitializeGamma(void);
>+#define NS_GammaCorrectComponent(x) (nsGammaRamp[x])
>+#define NS_GammaCorrectColor(x)  NS_RGBA(nsGammaRamp[NS_GET_R(x)], \
>+                                         nsGammaRamp[NS_GET_G(x)], \
>+                                         nsGammaRamp[NS_GET_B(x)], \
>+                                         NS_GET_A(x))
>+#define NS_InverseGammaCorrectComponent(x) (nsInverseGammaRamp[x])
>+#define NS_InverseGammaCorrectColor(x)  NS_RGBA(nsInverseGammaRamp[NS_GET_R(x)], \
>+                                                nsInverseGammaRamp[NS_GET_G(x)], \
>+                                                nsInverseGammaRamp[NS_GET_B(x)], \
>+                                                NS_GET_A(x))

These could use an empty line between decls and related macros for readability;
also, indent the definitions to the same column.

More important, later we get calls of this form:

+		 map->red   = NS_GammaCorrectComponent(*q++);

which requires that NS_GammaCorrectComponent use its macro parameter exactly
once, but NS_GammaCorrectColor and the Inverse counterpart expand their params
more than once.  They should be named using the UNSAFE_MACRO_CONVENTION:
NS_GAMMA_CORRECT_COLOR -- or they should be inline functions.

Otherwise looks good -- sr=[e-mail] with some attention here.

/be

Comment 119

[tor]

Attachment: polish macros and fix linkage per brendan's comments

Comment 120

[Brendan Eich [:brendan]]

Comment on attachment 77316 [details] [diff] [review]
polish macros and fix linkage per brendan's comments

Carrying stamps forward.

/be

Comment 121

[Paul]

Verifying for BeOS

Comment 122

[hacker formerly known as [e-mail]]

I'm hitting some link problems on BeOS.  It seems that we only link the decoders
against gkgfx on win32.  Since the decoders use these added functions, they will
need to be linked against gkgfx on any platforms that require you to resolve
symbols at link time: BeOS, OS/2, AIX and linux when using -Wl,-Bsymbolic (which
we use).

Comment 123

[Roland Mainz]

Filed bug 135634 ("Implement support for XSolarisGetVisualGamma()") to get
support for the gamma-correction in the Solaris Xservers (Xsun and the Solaris
version of Xprt) ...

Comment 124

[Roland Mainz]

Can someome please file a bug for the discussion and work to get support for
Xcms (I can take the bug on demand :) implemented ?

Comment 125

[Roland Mainz]

Comment on attachment 77316 [details] [diff] [review]
polish macros and fix linkage per brendan's comments

Verifying for Xlib gfx and Xprint module - both seem to work ...

Comment 126

[Roland Mainz]

Just wondering:
Where can I set the gamma value for the printer ?

Comment 127

[Roland Mainz]

bug 135634 now contains a patch to implement support for
|XSolarisGetVisualGamma()| on Solaris platforms. Who wants to r=/sr= it ? :)

Comment 128

[Roland Mainz]

Another (very minor) issue:
Which gamma value is correct: "2.2" or "2.22" ?

Some docs use "2.2", other use "2.22" - and Solaris Xsun uses 2.22 as default
gamma value (unless the hardware supplies a value - which is usually 2.22) ...

Comment 129

[Hixie (not reading bugmail)]

The CSS specs have the official answer for you. (2.22 IIRC)

Comment 130

[David Baron :dbaron:]

http://www.w3.org/TR/REC-CSS2/syndata.html#color-units says 2.2
http://www.w3.org/TR/REC-CSS2/colors.html#gamma-correction doesn't say, but
  suggests that using 2.22 for NeXT is correct

Comment 131

[Glenn Randers-Pehrson]

Looks like a typo in transcribing from the PNG gamma tutorial which says 2.2

Glenn

Comment 132

[Greg Roelofs]

CSS -> sRGB -> 2.2

With regard to the quoted section of the CSS spec, I don't know who wrote it,
but it looks wrong to me.  Certainly it's extremely imprecise, yet again
muddying the waters by using the term "gamma" without defining precisely what it
means by that.  In Roland's context (Solaris), gamma == 2.2 or 2.22 would refer
to the combination of LUT_exponent and CRT_exponent--i.e., where there is no LUT
(just as in typical PCs), so the CRT_exponent is equal to the display gamma.  In
that same context, NeXTs are 1.0 and Macs and SGIs are 1.3 and 1.5 (though I've
forgotten which is which).

In the CSS context, it _appears_ that they're talking about the LUT_exponent
only (or, more precisely, its inverse)--i.e., 1.0 (no adjustment) for PCs, 2.2
(== CRT_exponent) for NeXTs, etc.  But I'm not sure I trust any of those
numbers, insofar as they appear to be derived in some bass-ackward way from the
information in the (out-of-date) PNG 1.0 spec.

Greg

Comment 133

[Glenn Randers-Pehrson]

>In the CSS context, it _appears_ that they're talking about the LUT_exponent
>only (or, more precisely, its inverse)--i.e., 1.0 (no adjustment) for PCs, 2.2
>(== CRT_exponent) for NeXTs, etc.  But I'm not sure I trust any of those
>numbers, insofar as they appear to be derived in some bass-ackward way from the
>information in the (out-of-date) PNG 1.0 spec.

The same numbers appear in the (current) PNG 1.2 spec, in Section 13.9.
Default LUT exponents are PC: 1.0, Mac: 1/1.45, SGI: 1/1.7, NeXT: 1/2.2

This section has however been removed from the upcoming ISO PNG spec.

Glenn

Comment 134

[hacker formerly known as [e-mail]]

Attachment: previously mentioned makefile.in link changes

Comment 135

[David Baron :dbaron:]

Reassigning this bug to tor, since he has a patch that will hopefully be checked
in soon (right?).

Comment 136

[tor]

Attachment: merge to tip + cls' build changes

Comment 137

[Roland Mainz]

tor:
What about merging the patch in bug 135634 ("Implement support for
XSolarisGetVisualGamma()") with this one ?

Comment 138

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

changing PR_EXPORT_DATA to PR_IMPORT_DATA seems to work on windows-platforms 
too. I still need compile the rest of my tree and run before i'm compleatly sure

Comment 139

[tor]

After a rough landing this has been checked into the trunk.  Give it a shot.
QA: MacOS is the platform that really needs to be tested before verifying.

Leaving open for possible branch pickup.

Comment 140

[Brendan Eich [:brendan]]

Sorry about the PR_EXPORT_DATA bum steer -- if memory serves now, the best way
to use these macros is to define some other macro only when buildling the
library, and test that other macro with an #ifdef to decide whether to EXPORT
(if defined) or IMPORT (for consumers of the lib's .h file).  Can we do that? 
Is there XPCOM NS_COM or similar macro layering we can use?

/be

Comment 141

[tor]

Attachment: jpeg gamma correction for unix platforms

When doing the classic libimg to libpr0n update I missed gamma correction for
RGB images on unix platforms.

Comment 142

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 79136 [details] [diff] [review]
jpeg gamma correction for unix platforms

r=biesi

Comment 143

[Brendan Eich [:brendan]]

Comment on attachment 79136 [details] [diff] [review]
jpeg gamma correction for unix platforms

sr=[e-mail], good catch for 1.0 (for RC1?).

/be

Comment 144

[tor]

jpeg/rgb/unix fix checked in.

Comment 145

[Eric A. Meyer (dead account)]

Sorry if I'm spamming, but would this patch have caused a shift in the
"breakpoints" between shades of gray?  See
http://www.meyerweb.com/eric/css/tests/grayscale.html in Mozilla 2002040908 (or
earlier) versus 2002041308, for example.  Loading up that page in IE5/Mac and
2002040908 shows that they exactly match in where the breakpoints fall.  They're
inconsistent between IE5/Mac and 2002041308.  A design that relies on subtle
shades of gray-- such as, for example, alternate-row highlighting in a long
table-- could be horked by such a change.  One place where that actually
happened is
http://developer.netscape.com/evangelism/sidebar/css2qr/prop-visual.html (it's
the only reason I even noticed this bug).  In older Mozilla builds, there was a
slight variation between rows.  In 2002041308, the shades are rendered the same.
 It's a small thing, but it's also the kind of thing that can really enrage
authors when it happens, and even more when they figure out why it happened. 
(My first instinct was that my class names were being ignored.)

Again, sorry if I'm spamming on a bug unrelated to my problem, but I'm trying to
figure if this iws what caused the problem, and if so why we're making this
change when it's going to lead to inconsitencies (some would argue visual
incompatibilites) between 1.0 and previous milestones.

Comment 146

[David Baron :dbaron:]

It seems like this would have caused such a change.  However, it seems more
important to me than the colors are consistent between Mac and Windows than that
we maintain the bug (according to CSS1) that we display colors differently and
incorrectly on the Mac.

(If you use a 16-bit or whatever depth display you have on Mac, on a Windows
machine, do the threshholds look like the ones you used to see or the ones you
see now?  I'm not sure what the answer will be, but I'd be somewhat surprised if
they're consistent.  On my 24-bit display I don't see any threshholds, and I
suspect that on an 8-bit display there would be a single threshhold within that
whole panel, if that.)

Comment 147

[Henri Sivonen (:hsivonen)]

> However, it seems more important to me than the colors are consistent between 
> Mac and Windows than that we maintain the bug (according to CSS1) that we 
> display colors differently and incorrectly on the Mac.

For the record, due to bug 75133 the colors are still incorrect but in a
different way on Macs whose display gamma is different from value that is
hard-coded in Mozilla. This can make the colors quite dark on systems where the
display gamma is over 1.8 and then Mozilla applies the darkening "correction" on
top of that.

Comment 148

[marlon bishop]

just a few comments on the results of this for the Mac.   I believe we applied
an overcorrection to the gifs and html colors in both content and chrome on mac.
since the modern theme and all web sites are designed to fit between 1.8 and 2.2
as a compromise to windows and mac gammas,   what we see today on the mac is
tremendously oversaturated.  I am not sure the best way to go about fixing it
but it seems like we may just need to lower the hard coded gamma setting, or
turn off color correction of gif and html, or any inherently uncorrectable
format to stay true to what the designer intended.

then again, it wouldn't be nice to have discrepencies between html and css
either... just having PNG and CSS match is more important for designers IMHO

Comment 149

[Greg Roelofs]

As I pointed out in my (slightly testy) newsgroup posting that triggered this
bug (message-ID <8p6im9$[e-mail]>, cross-posted to
netscape.public.mozilla.ui, netscape.public.dev.skins, and
netscape.public.mozilla.xpfe on 2000-09-06), designers doing "gamma by eye" on
the Mac platform are almost guaranteed to be doing the wrong thing--i.e., that
methodology is fundamentally in conflict with specifications that require the
sRGB color space (unless all of the Macs in question have ColorSync and are set
up to use the sRGB color space).  In particular, the absolute "brightness level"
was wrong by design and shouldn't be a concern when _correcting_ the
spec-conformance bug (which is the point of this bug report and its associated
patches).  The primary concern for backward compatibility should be that
unlabelled images and (previously uncorrected) HTML/CSS colors that used to
match, still do match.

So if the browser used for the "gamma by eye" approach doesn't do any gamma
correction at all, then the colors that matched then should still match now
(with the exception of PNGs with gAMA chunks that were created that way, but
there aren't any, at least until 8415 is resolved--right?). Anyway, that much
works as intended, doesn't it?

Marlon's comment implies that the absolute "brightness level" has gotten out of
whack somehow--is that true of all 10 cases at the test URL above, or is it
really limited only to GIFs and HTML colors?  If the correction were working as
intended (and assuming the hardcoded Mac gamma value in Mozilla matches the Mac
being tested), then the brightness level should be very similar to that of a
standard PC when seen side-by-side.  Is that not true?  IOW, is there a
double-correction being applied in the Mac code somehow?

Btw, note that "Mac gamma 1.8" is really 1.5 in the sense that corresponds to
2.2 on PCs.  (There's a weird 2.61/2.2 fudge factor in there.)

Greg

Comment 150

[marlon bishop]

Greg: I would characterize the difference between what i see going from mac to
pc as: incredibly huge (btw, using the same brand of monitor on both mac and pc
and set with equivalent adjustments).  the tests you've given all pass, because
they are incapable of revealing the particular problem we're dealing with at the
moment.  no such test could detect the gamma shift that has occurred since
before and after the code was turned on. You would need a way to toggle
correction off and on to see this difference.  

there is a clear problem happening, that's all i am technically capable of
telling you.  It may be this 'double correcting' you're speaking of, at least it
sounds like an accurate description of what i am seeing.  Windows looks great
btw, good job. 

 to summarize- everthing *is* internally matching, but not cross-platform, which
is the desired result

Comment 151

[marlon bishop]

Attachment: comparison of the results

see anything peculiar about this comparison?  this image has the benefit of
being monitor independent since it was captured by each platform's system
screen video dump.  It appears that windows is now displying an overcorrected
macintosh-like gamma (i.e. looks almost the way it did *on the mac* prior to
the gamma patch); and the mac is displaying a double corrected windows gamma
(i.e. looks twice as intense as the original uncorrected windows gamma prior to
the patch).  so it seems neither hue nor brightness levels match across
platforms after the patch, when they should be identical.

otherwise everything else is fine.  the test cases pass with flying colors
(literally) and no other browser can claim to do that.

Comment 152

[David Baron :dbaron:]

> see anything peculiar about this comparison?  this image has the benefit of
> being monitor independent since it was captured by each platform's system
> screen video dump.

Well, it seems like the Macintosh video dump utility might be doing some sort of
gamma correction, although the numbers seem quite small it could have something
to do with rounding errors in the tool that merged the images.   (How were the
images merged into a single image?  Did they have gamma specified in the image
beforehand?  Afterwards?  And do you have a gamma value specified in your system
settings on the Mac that's different from the default?)

FWIW, the numbers gimp is telling me for the tree header (which is a constant
color, unlike most of the image) are:
             R   G   B
Mac before: 199 208 217
Mac after:  178 190 202
Windows:    198 211 222
The Mac before and Windows numbers seem too close together to reflect what was
really going on then.

Comment 153

[David Baron :dbaron:]

> It appears that windows is now displying an overcorrected
> macintosh-like gamma (i.e. looks almost the way it did *on the mac* prior to
> the gamma patch)

The patch should not have changed the behavior on Windows at all, and I don't
think it did.  Why do you think the appearance is different?

Comment 154

[marlon bishop]

>Well, it seems like the Macintosh video dump utility might be doing some sort
>of gamma correction,

nope, not using a utility it is being done by the OS.  it sends exactly what is
being rendered by the system to a file.

>(How were the images merged into a single image?  Did they have gamma specified
>in the image beforehand?  Afterwards?  And do you have a gamma value specified
in >your system settings on the Mac that's different from the default?)

not that it matters since all the images were produced in the same way, the
example in the comparison are are all correct relative to each other and
relative to the the gamma space of photoshop (which is notoriously suppling a
strange gamma value to pngs; however most browsers until now ignore that value
so the colors are 'real') 

anyway, here's how i created the attachment: screen capture from Mac OS
(colorsync does not change any values here, since the capture is pre-colorsync),
open in photoshop (with no colorspace correction turned on).  next step, screen
capture from win [print scrn] command pasted into an uncolorcorrected photoshop
environment and exported as PNG (with weird gAMA chunk).  Transfer to mac,
composite images in photoshop (reads it's own weirdness fine).  Export as PNG,
post in bugzilla.

>The Mac before and Windows numbers seem too close together to reflect what was
>really going on then.

true the numbers are too close when they should be the same, which probably has
something to do with the adobe's PNG

>The patch should not have changed the behavior on Windows at all, and I don't
>think it did.  Why do you think the appearance is different?

you are correct i jumped to conclusion on that.

Comment 155

[Greg Roelofs]

> see anything peculiar about this comparison?  this image has the benefit of
> being monitor independent since it was captured by each platform's system
> screen video dump

Well, first of all, keep in mind that capturing the framebuffer isn't definitive
since we're trying to account for the entire display system, and the fb
certainly has no knowledge of the monitor (though all of those behave pretty
much the same, aside from color-temperature settings in some higher-end models)
and probably not of the lookup table in the graphics card.  It probably _does_
include any modifications done by the color-management system (e.g., ColorSync),
since CMSes generally sit between the application and the framebuffer.  So it's
a useful check on that (assuming whatever utility was used to read the memory
didn't also get adjusted by the CMS).

Thus the fact that the "after" Windows capture looks pretty much the same as the
"before" Mac shot is expected:  Windows/Linux x86 boxes tend to be pretty close
to sRGB already, so no correction is necessary, and that matches the behavior of
the "before" code.  The fact that the "after" Mac shot is darker is also to be
expected:  images that look "normal" on a Mac need to be lightened on a PC, and
correspondingly images that look normal on a PC (i.e., sRGB images) need to be
darkened on a Mac.  And, in fact, the "after" Mac shot looks great to me, as
displayed on my somewhat too-bright (i.e., Mac-like) Linux display system.  The
other two images look a bit washed out, which is how I would expect most sRGB
chrome would look on a Mac without gamma correction.

So I don't see any obvious problems with this attachment.  The only definitive
test would be to photograph both monitors with exactly the same exposure and
aperture and upload those photos for comparison.  Obviously that's kind of a
pain (not to mention tricky, given the scanning pattern of the electron beam),
but I don't have access to a Mac to see for myself. :-/

Greg

Comment 156

[marlon bishop]

>It probably _does_include any modifications done by the color-management system
>(e.g., ColorSync), since CMSes generally sit between the application and the
>framebuffer.  So it's a useful check on that (assuming whatever utility was
used >to read the memory didn't also get adjusted by the CMS)

ok i will rephrase, the screen capture is unaffected by colorsync -  i've tested
that before.  apple's DigitalColor meter utility is also unaffected by
colorsync, so i am not sure where colorsync lies in the
application-framebuffer-screencapture relationship, but i know for sure that the
screenshot i produced is a relatively accurate representation of the differences.


>The fact that the "after" Mac shot is darker is also to be
>expected:  images that look "normal" on a Mac need to be lightened on a PC, and
>correspondingly images that look normal on a PC (i.e., sRGB images) need to be
>darkened on a Mac.  And, in fact, the "after" Mac shot looks great to me, as
>displayed on my somewhat too-bright (i.e., Mac-like) Linux display system. 

it's most definitely not 'great'.  In fact if i apply a 2.2 gamma curve to the
original Mac 'before' shot, i don't get a result as dark as what our gamma
correction has done for us.  As a matter of fact, the difference in brightness
is 79 compared to 82 on a scale of 100 (100 is full brightness): 3 point
discrepency in brightness is quite substantial.  We're also off by one degree on
the hue scale.

also note that 'lightness' is not a good way to describe gamma conversion
because it's not linear, and involves saturation as well.

>So I don't see any obvious problems with this attachment.  The only definitive
>test would be to photograph both monitors with exactly the same exposure and
>aperture and upload those photos for comparison.

taking photos is not going to be a good test, i recommend we either hire an ISF
technican to measure our results or lease a colorimeter ourselves.   shouldn't
be a problem getting Netscape to do that, since this affects how accurately we
render the entire web too.

Comment 157

[marlon bishop]

Attachment: comparison of rendered content

here's a more dramatic comparison which i hope will get my point across. look
at the numbers measured in the blue area - in this case the darkness on the mac
has increased by 68%!!!   this proves that the problem is not linear.

we need to make an extremely clear point here, the judgement: 'looks great' is
not yours or mine to make.  you can take virtually any image and make it
subjectively 'look great' by tweaking the gamma curve in photoshop.  but does
that mean if i keep boosting gamma left and right that i am making an image
look better?  ah - nope.  In this case i'll take 'accuracy' over 'looks great'
any time, because i don't want to mess with intent of some company to produce
their trademark color of blue.

Comment 158

[marlon bishop]

btw, since i am not sure what's going on with our PNG reading on builds with
this code checked in, you might want to view my PNG attachments in another
viewer which won't pick up the faulty photoshop gAMA chunk.  try using a
previous build or whatever else you have that ignores gAMA

Comment 159

[Glenn Randers-Pehrson]

If the gAMA chunks are bogus you can use pngcrush to remove or correct them, 
and post a new set without them or with corrected gAMA chunks.

Comment 160

[Kevin McCluskey (gone)]

bug 137975 appears to be a dup of the over correction issue on Mac.

Comment 161

[marlon bishop]

I'll take future comments over to bug 137975

>in this case the darkness on the mac has increased by 68%!!!  this 
>proves that the problem is not linear.

btw, i should correct myself  - that's 32% darker not 68 - sorry for my 'fuzzy 
math'

randeg: i couldn't get pngcrush to change gAMA on my windows machine 
last time i tried.. it did basically nothing,  and i don't have a linux box.   
Know of any other tools on mac or win?  Debabelizer 5 is out perhaps it 
corrected previous png problems

Comment 162

[Greg Roelofs]

Marlon:  Glenn's been busy with libpng releases this week, but basically you do
this:

	pngcrush -rem gAMA -d outputdir foo.png

That will put the modified foo.png in outputdir.  I don't think the -rem option
is case-sensitive, so "gama" should work, too.

Greg

Comment 163

[Eric A. Meyer (dead account)]

I'm confused now.  In 2002041308, there was the reported shifting in shading
levels on a "thousands of colors" display.  In RC1, there isn't, which means the
CSS-based color shading seems to be back to its 0.9.9 behavior.

The attachments show that Before and Windows After match fairly well, whereas
Mac After does not.  That seems like a step backwards, since we're going from a
close match to a noticeable separation.  Plus, when I load the attached PNGs
into IE5.1.3/Mac and RC1/Mac, the images are clearly different shades in the two
browsers; RC1 yields darker RGB levels than IE when the pixels are checked. 
When comparing the same images between IE5.1.3/Mac to 0.9.9, the shades look the
same and in fact have the same on-screen RGB values.  (I used ZoomLens to
compare the RGB values at each step.)

Unless someone can clearly explain why this is a needed step in the real world
(as in: site performance/appearance will be improved by this patch), and show
that it isn't making the overall problem worse, my recommendation is that we
push this patch back to 1.1.  I recognize that PNG is not a widespread format,
but this is a potentially major change that could affect all color-handling, and
so far the evidence I see is that it's a change for the worse.

Comment 164

[Christopher Aillon (sabbatical, not receiving bugmail)]

Eric, unless I'm mistaken, this hasn't been checked in on the branch yet.  Which
explains why you're seeing the 0.9.9 behavior on RC1... Note this bug is on the
"Make RC2 not suck" bug.

Comment 165

[David Baron :dbaron:]

I still think we definitely want this for 1.0.  I question the validity of the
images because image editing software and transferring files from system to
system is highly unlikely to reflect what's actually displayed.

There is a possibility that we need to change the mac gamma correction constant
to something weaker (i.e., closer to 2.2), but all the experts seem to be saying
that we should be doing some correction on the mac.  *If* the number we're using
is wrong (I'm not yet convinced by the comments here), we should fix it, rather
than using the incorrect number only for PNG and MNG images and doing all other
colors with no correction at all (which is also wrong).

(The comments above, in comment 151, suggest to me that PNG and CSS colors are
now matching, which would mean that an incorrect gamma value seems like the only
possibility of what went wrong, if anything is wrong.  These tests do pass, right?)

Has anyone compared our rendering of PNGs to IE5/Mac's to see whether the same
gamma correction is used?

Can anyone point to documentation on what the correct correction for Mac should be?

Comment 166

[nnooiissee]

Adding bug 86925 as being blocked by this bug. http://www.debian.org/ looks good
to me (although I can't check the exact values) with trunk build 2002041903 on
mac os x with default ColorSync settings.

Comment 167

[Eric A. Meyer (dead account)]

David asked in comment 165, "Has anyone compared our rendering of PNGs to
IE5/Mac's to see whether the same gamma correction is used?"  Yes.  See my
attachments (in comments 19 and 20 of that bug) to bug 75133.  I'm less
concerned about this than I was, since my RC1 comparisons should be thrown out
(I was mistaken about the patch being applied there).  Unfortunately, the
attachments in 75133 do show small but noticeable color differences between
IE5.1.3 and the 2002041903 trunk build, so I still have some concern.  If we get
the desired gamma correction and not introduce inconsistencies between browsers,
I'll happily withdraw the  opposition expressed in comment 163.

Comment 168

[Greg Roelofs]

I just posted a lengthy comment and proposal/request for Mac sample code in bug
75133 comment 21 ( http://bugzilla.mozilla.org/show_bug.cgi?id=75133#c21 , in
case bugzilla's autolinking doesn't work across bug boundaries).  This is purely
for determining the precise location and nature of the bug--i.e., is it IE or
Moz or both?--which is important to know before attempting to fix it.

Greg

Comment 169

[David Hyatt]

I'm coming in here without really having read any of the previous comments.  I 
thought I'd quickly add some "user feedback."  I released Chimera on the Mac 
with this gamma correction included, and I'm receiving tons of complaints and 
feedback that Web sites are now "way too dark."  Given the volume of the 
complaints, it's clear there's a problem on the Mac with the current settings, 
but I'll leave it to smarter people than I to figure out how to fix the 
problem.

Comment 170

[tor]

Possible ugly compromise: perform no correction on jpeg/gif/css/etc...
and always pass in a gamma of 2.2 for libpng and libmng.  This would
keep the overbright images that people are used to on the Mac and
still allow *NG colors to be matched.

Comment 171

[Eric A. Meyer (dead account)]

Just to add more information, see attachment 80399 [details] (to bug 75133) where I
compare IE5.1.3/Mac and 2002041903 under MacOS9.1-- I haven't installed OS X
yet-- and find that straight HTML+CSS with no images of any kind yields
different shades of the same colors.

Comment 172

[Stuart Ballard]

I don't know a whole lot about gamma correction, so forgive me if this is a
stupid idea.

If Mac users like the way the web renders *now*, wouldn't it be possible to
choose a Gamma value that gives the equivalent result to what they see today for
GIFs, HTML colors and CSS colors? That way PNGs would get gamma-corrected to
match the existing colors that they seem to be happy with.

This may not be technically correct, but it would probably give the desired
effect?

Comment 173

[David Hyatt]

Feedback on Chimera 0.2.3 for OS X:

"Browser is still showing heaps of promise and I can use it most of the time 
as my browser with few problems. The only problem is the digusting new gamma 
settings which render images like a 10 year old PC!....UGH! "

"Shows great promise... but the new gamma settings make everything seem too 
dark and sludgy"

"The gamma in 0.2.3 is significantly darker - not great for us web designers 
that already have our monitors set to a windows gamma.... Otherwise, this is 
one great browser"

"Very promising and fast. However, 0.2.3 now displays everything darker 
(windows gamma?)."

etc.  The complaints are all the same: "too dark".  :)

Comment 174

[nnooiissee]

As to comment 173, at least one of those four must be seeing double correction.

I think this is feature is an improvement (OS X). http://www.debian.org/ looks
better, and the insect image http://bugzilla.mozilla.org/ does not look as
horribly washed out, which hides some of the brutal artifacting (and the moz
dino looks better red than orange). The darker chrome is pleasant as well. 

Comment 175

[Greg Roelofs]

W.r.t. the third item in comment 173, that's a known bug in the implementation:
the code assumes the system has a standard Mac gamma setting (1.5, a.k.a.
"1.8"), and the user has set a standard PC setting (presumably 2.2, a.k.a.
"2.61"). Not much to be done there except figure out how to extract the system
gamma from the ICC profile returned by the ColorSync API. 

W.r.t. comment 170 (basically the same as 172), that's probably a reasonable
option--but it should be selectable, and insofar as it's technically incorrect,
it should _not_ be the default option.

But before much of anything changes, it would be *really* good to determine
whether this is working as intended or not.  Given the Mac MSIE PNG results, it
sounds like it's a simple bug in Moz's Mac front end.  Don't any of the Mac
folks have (or know someone who has) basic C code to display a bitmap using the
standard Mac APIs? (bug 75133)

Greg

Comment 176

[Jason Kersey]

Unsure if this is related, but on windows all images with a white background are
showing up with a green/pink background, and show up when the page background is
white.  The easiest testcase for me is google, where the head image has a white
background, along with the page.  I shouldn't be able to see the where the image
ends and the page starts, but I can, because of the image not being true white.
 Some screenshot comparisons:  http://www.mozillazine.org/jason/white.png -
shows how mozilla shows the image, and how a normal image program shows it;
http://www.mozillazine.org/jason/white.png - shows the difference between the
page and the image whites.

Comment 177

[nnooiissee]

Why does bug 135634 depend on this and not bug 75133?

Comment 178

[David Hyatt]

Can someone tell me an easy way of disabling gamma correction in my build so 
that I can see how it was behaving before the correction?  I'd also like to 
disable it in Chimera releases until these Mac problems are resolved.

I don't want to back the whole huge patch out.  I just want to know if there's 
a simple hack I can do in my tree to revert to the old behavior.

Comment 179

[tor]

hyatt: just change nsScreenMac::GetGammaValue() to set a gamma of 2.2.
Alternatively modify NS_InitializeGamma() to set a linear ramp.

Comment 180

[tor]

Attachment: gamma backout patch

The comments here and in bug 75133 make it clear that the gamma in mozilla
needs more thought.  Since hyatt is turning it off for Chimera, the gamma
code is essentially a wasteful no-op at this point.

This backout patch removes correction for gif/jpeg/bmp/ico/css/html, and
sets a display gamma of 2.2 for libpng and libmng.

Comment 181

[David Baron :dbaron:]

Maybe it wouldn't be turned off for Chimera if we used a constant that led to
less gamma correction?

Comment 182

[David Baron :dbaron:]

Comment on attachment 81029 [details] [diff] [review]
gamma backout patch

r=dbaron

Comment 183

[Andrew Thompson]

I believe we should try a few days with my fix on bug 75133 and see how this is 
received before backing out tor's changes.

Comment 184

[Brendan Eich [:brendan]]

Comment on attachment 81029 [details] [diff] [review]
gamma backout patch

Sorry to see us back out -- what is the plan to get the right fix in, once we
have backed out?

rs=[e-mail]

Comment 185

[Asa Dotzler [:asa]]

Comment on attachment 81029 [details] [diff] [review]
gamma backout patch

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 186

[tor]

gamma correction backed out on the trunk.

Asa: gamma correction never made it to the 1.0 branch.

Comment 187

[Asa Dotzler [:asa]]

ahh. sorry for my confusion. removing from the RC2 list. 

Comment 188

[tor]

Attachment: branch gamma patch

If gamma is going to be wrong on the branch, let's at least be consistently
wrong.	Patch sets a display gamma of 2.2 in the PNG and MNG decoders
regardless of platform.  This is the same as what's on the trunk since
the gamma backout.

Comment 189

[David Baron :dbaron:]

Comment on attachment 81936 [details] [diff] [review]
branch gamma patch

r=dbaron

Comment 190

[Brendan Eich [:brendan]]

Comment on attachment 81936 [details] [diff] [review]
branch gamma patch

sr=[e-mail]

/be

Comment 191

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 81936 [details] [diff] [review]
branch gamma patch

a=roc+moz

Comment 192

[tor]

Attachment 81936 [details] [diff] checked into branch.

Comment 193

[Asa Dotzler [:asa]]

adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it
was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify
the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword
with verified1.0.0.

Comment 194

[Madhur Bhatia]

cc'ing myself

Comment 195

[Gervase Markham [:gerv]]

tor: is there a plan to revive this patch for the trunk?

Gerv

Comment 196

[tor]

Gerv: no immediate plans to do so - any reason you ask?

Comment 197

[Gervase Markham [:gerv]]

Because (as far as I can tell) it what is blocking any sort of PNG chrome
effort. Have I got the wrong end of the stick?

Gerv

Comment 198

[Madhur Bhatia]

I am somehow unable to reproduce the bug in a build earlier than 05/02/2002
(that is when the patch for the fix was moved into branch, according to comment
192), on all platforms. 

Previous build, as well as, the current branch builds display the testcases
(mentioned in the above url) as desired (with gamma  correction).

Anyone can tell me which build to look at to reproduce the bug before i mark it
verified for the branch

Comment 199

[Hixie (not reading bugmail)]

tor: What's the status on this?

Comment 200

[Ryan VanderMeulen [:RyanVM]]

tor, any update 4 years later? Does having lcms on the trunk help with this bug?

Comment 201

[Glenn Randers-Pehrson]

>Does having lcms on the trunk help with this bug?

Looking at the colorcube URL above with FF-3.0-beta2, it seems that PNG images with the iCCP chunk are improved but not perfect, while PNG images with the sRGB chunk are worse (similar to iCCP), when gfx.color_management.enabled is "true".  Incredibly, even pure white is rendered incorrectly when the sRGB or iCCP chunk is present.

Comment 202

[Bobby Holley (:bholley)]

Is this still a blocker for turning color management on by default?

Comment 203

[Glenn Randers-Pehrson]

The referenced URL is still displayed incorrectly for me using
FF-3.0 with color_management enabled and using the default Windows
sRGB screen profile.  Of the 216 colors on the color cube, only
pure black is displayed correctly.  Even pure white is wrong.
I consider it still a blocker.

Glenn

Comment 204

[Glenn Randers-Pehrson]

It will be interesting to see if Argyllcms does a better job
of rendering this URL.  See bug #445468.

Comment 205

[Bobby Holley (:bholley)]

I recently landed bug 452676, which fixed some problems in how we were handling gAMA chunks (among other things) when color management was turned on.

Looking at the color squares, they all look perfect (including the sRGB ones), except for the iCCP ones, which look off in a few places. Glenn, how do they look for you? Are the issues you were seeing before resolved?

Comment 206

[Glenn Randers-Pehrson]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080909032504 Minefield/3.1b1pre, gfx.color_management=1,
Windows-supplied sRGB profile, looks good except as you say there
are some very faint differences on the iCCP tests.

Comment 207

[philippe (part-time)]

On my Mac, with gfx.color_management=1, and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080909020334 Minefield/3.1b1pre
Perfect, except for the iCCP ones.

I have a Camino 2.0a1pre/gecko 1.9.0.3 pre build next to it to compare. Some squares are off on 1.9.0.3, but correct on 1.9.1b1pre.

Comment 208

[Bobby Holley (:bholley)]

Resolving this mammoth bug as fixed. Discussion about the faint differences with iCCP chunks can continue in bug 454688.

Comment 209

[Glenn Randers-Pehrson]

Google chrome apparently has color management enabled but the test URL looks
just the same as unpatched Firefox 3.0.1 with color management mode 1.  Evidently it needs the patch from bug #452676.