Closed Bug 53597 Opened 24 years ago Closed 16 years ago

CSS colors are missing gamma correction

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: marlon.bishop, Assigned: tor)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

(Keywords: css1, css2, html4, Whiteboard: [Hixie-P3][CSS1-6.3])

Attachments

(4 files, 22 obsolete files)

gamma labelled png2.2 on css
17.04 KB, image/gif
Details
no gamma labelled png on css colors
13.91 KB, image/gif
Details
gamma backout patch
47.31 KB, patch
dbaron
: review+
brendan
: superreview+
asa
: approval+
Details | Diff | Splinter Review
branch gamma patch
3.50 KB, patch
dbaron
: review+
brendan
: superreview+
roc
: approval+
Details | Diff | Splinter Review 
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
Summary: CSS colors are missing gamma correction → CSS colors are missing gamma correction
Blocks: 8415, 45008
*** Bug 53606 has been marked as a duplicate of this bug. ***
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])."
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.
Assignee: pierre → tor
Attached patch css/html/gif gamma correction (obsolete) — — Splinter Review 

    
    

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

Status: NEW → ASSIGNED

    
    

    
  
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.


    
    

    
  
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.

    
    

    
  


  

    
    

    
  
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 chris@w3.org for feedback.


    
    

    
  
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.

    
    

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


    
    

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

    
    

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


    
    

    
  
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.  


    
    

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

    
    

    
  
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.

    
    

    
  
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?
Keywords: qawanted

    
    

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

    
    

    
  
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.

    
    

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


    
    

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

Keywords: rtm
Whiteboard: [rtm+]

    
    

    
  
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.
URL: http://www.w3.org/TR/REC-CSS2/syndata... → http://www.libpng.org/pub/png/colorcu...

    
    

    
  
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.


    
    

    
  
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


    
    

    
  


  

    
    

    
  


  

    
    

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


    
  
Keywords: patch
Whiteboard: [rtm+]

    
    

    
  
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.


    
    

    
  
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?


    
    

    
  
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


    
    

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


    
    

    
  


  

    
    

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


    
    

    
  
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.


    
    

    
  


  

    
    

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


    
    

    
  
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.
Keywords: mozilla1.0

    
    

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


    
    

    
  
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

    
    

    
  


  

    
    

    
  
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


    
    

    
  
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

    
    

    
  


  

    
    

    
  
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


    
    

    
  
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.


    
    

    
  
rtm-, not a stop ship.
Whiteboard: [rtm-]

    
    

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

    
    

    
  


  

    
    

    
  
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


    
    

    
  
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?


    
    

    
  
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.


    
    

    
  
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.


    
    

    
  
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 <nbhatla@netscape.com>
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.

    
    

    
  
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

    
    

    
  


  

    
    

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


    
    

    
  
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

    
    

    
  
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.


    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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?



    
    

    
  
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.


    
    

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

    
    

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

    
    

    
  
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.  

    
    

    
  
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?


    
    

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


    
    

    
  
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?


    
    

    
  
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.

    
  
Target Milestone: --- → mozilla0.8

    
    

    
  
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.


    
    

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

    
    

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

    
    

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

    
    

    
  
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?

    
    

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

    
    

    
  
Disowning.

Assignee: tor → nobody
Status: ASSIGNED → NEW

    
    

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

    
    

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

    
    

    
  
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
Target Milestone: mozilla0.8 → mozilla0.9

    
    

    
  
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...
QA Contact: chrisd → ian

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated patch (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
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.

Keywords: correctness, 
          
            css2, 
          
            html4, 
          
            mozilla0.9

    
    

    
  

      
      
      
      

        Attached patch
          
          
        merge to tip (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
tor, have you reclaimed?  
moving nobody's bugs off the 0.9 list.
somebody assign it the right milestone
Target Milestone: mozilla0.9 → ---

    
  
Target Milestone: --- → mozilla0.9.1

    
    

    
  
moving nobody's bugs off the 0.9.1 milestone list.
Target Milestone: mozilla0.9.1 → ---

    
    

    
  
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 

    
    

    
  
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.

Whiteboard: [rtm-]

    
    

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

/be
Keywords: qawanted, 
          
            rtm
Whiteboard: [Hixie-P3]

    
    

    
  
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.

    
    

    
  
CC'ing attinasi@netscape.com for style system help.

    
    

    
  
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)

    
    

    
  
Taking the bug

Assignee: nobody → pierre
Target Milestone: --- → mozilla0.9.3

    
  
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.3 → mozilla0.9.4

    
  
Target Milestone: mozilla0.9.4 → mozilla0.9.5

    
    

    
  
Moving again.... 0.9.6

Target Milestone: mozilla0.9.5 → mozilla0.9.6

    
  
Target Milestone: mozilla0.9.6 → mozilla1.0

    
    

    
  
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


    
  
Target Milestone: mozilla1.0 → mozilla1.2

    
    

    
  
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.

    
    

    
  
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" :)

Severity: normal → major
Priority: P3 → P2
Target Milestone: mozilla1.2 → mozilla1.0

    
    

    
  
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.

    
    

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

    
    

    
  
Moving to Mozilla1.1. Engineers are overloaded with higher priority bugs.
Target Milestone: mozilla1.0 → mozilla1.1

    
    

    
  
Bulk moving from Moz1.1 to future-P1. I will pull from this list when scheduling
work post Mozilla1.0.
Priority: P2 → P1
Target Milestone: mozilla1.1 → Future

    
  
Keywords: mozilla1.0+

    
  
Keywords: mozilla1.0

    
    

    
  
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-.
Keywords: nsbeta1-

    
    

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

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        merge to tip (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
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

    
    

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

    
    

    
  
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.

    
    

    
  


  

        Attachment #74548 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        address dbaron's comments (obsolete)
        — 
          — Splinter Review
      

    


  
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

        Attachment #76346 -
        Attachment is obsolete: true

    
    

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

        Attachment #76441 -
        Flags: review+

    
    

    
  


  

        Attachment #76441 -
        Attachment is obsolete: true

    
    

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

    
    

    
  
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?

    
    

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

    
    

    
  
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=brendan@mozilla.org with some attention here.

/be

        Attachment #76464 -
        Flags: superreview+

    
    

    
  


  

        Attachment #76464 -
        Attachment is obsolete: true

    
    

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

Carrying stamps forward.

/be

        Attachment #77316 -
        Flags: superreview+

        Attachment #77316 -
        Flags: review+

    
    

    
  
Verifying for BeOS

    
    

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


    
  
Blocks: 135634

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

Glenn

    
    

    
  
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

    
    

    
  
>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



    
    

    
  
Reassigning this bug to tor, since he has a patch that will hopefully be checked
in soon (right?).
Assignee: pierre → tor
Status: ASSIGNED → NEW
Target Milestone: Future → mozilla1.0

    
    

    
  


  

        Attachment #77316 -
        Attachment is obsolete: true

        Attachment #78273 -
        Attachment is obsolete: true

    
    

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

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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

    
    

    
  


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

    
    

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

r=biesi

        Attachment #79136 -
        Flags: review+

    
    

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

sr=brendan@mozilla.org, good catch for 1.0 (for RC1?).

/be

        Attachment #79136 -
        Flags: superreview+

    
    

    
  
jpeg/rgb/unix fix checked in.

    
    

    
  
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.

    
    

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

    
    

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

    
    

    
  
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

    
    

    
  
As I pointed out in my (slightly testy) newsgroup posting that triggered this
bug (message-ID <8p6im9$4v21@secnews.netscape.com>, 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

    
    

    
  
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

    
    

    
  

      
      
      
      

        Attached image
          
        comparison of the results (obsolete)
        — 
      

    


  
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.

    
    

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

    
    

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

    
    

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

    
    

    
  
> 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

    
    

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

    
    

    
  

      
      
      
      

        Attached image
          
        comparison of rendered content (obsolete)
        — 
      

    


  
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.

    
    

    
  
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

    
    

    
  
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.

    
    

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

    
    

    
  
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

    
    

    
  
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
Blocks: 138000

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  
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.
Blocks: 86925

    
    

    
  
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.


    
    

    
  
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

    
    

    
  
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.


    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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?

    
    

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



    
    

    
  
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. 

    
    

    
  
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

    
    

    
  
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.

    
  
Blocks: 75133

    
  
No longer blocks: 75133

    
    

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

    
    

    
  
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.


    
    

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

    
    

    
  


  
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.

    
    

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

    
  
Depends on: 75133

    
    

    
  
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 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=brendan@mozilla.org

        Attachment #81029 -
        Flags: superreview+

    
    

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

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

        Attachment #81029 -
        Flags: approval+

    
    

    
  
gamma correction backed out on the trunk.

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

    
    

    
  
ahh. sorry for my confusion. removing from the RC2 list. 
No longer blocks: 138000

    
    

    
  


  
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.

        Attachment #15264 -
        Attachment is obsolete: true

        Attachment #15311 -
        Attachment is obsolete: true

        Attachment #16400 -
        Attachment is obsolete: true

        Attachment #16518 -
        Attachment is obsolete: true

        Attachment #17814 -
        Attachment is obsolete: true

        Attachment #17895 -
        Attachment is obsolete: true

        Attachment #18805 -
        Attachment is obsolete: true

        Attachment #18812 -
        Attachment is obsolete: true

        Attachment #18950 -
        Attachment is obsolete: true

        Attachment #19060 -
        Attachment is obsolete: true

        Attachment #29669 -
        Attachment is obsolete: true

        Attachment #31096 -
        Attachment is obsolete: true

        Attachment #78938 -
        Attachment is obsolete: true

        Attachment #79136 -
        Attachment is obsolete: true

        Attachment #79569 -
        Attachment is obsolete: true

        Attachment #79583 -
        Attachment is obsolete: true

    
    

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

sr=brendan@mozilla.org

/be

        Attachment #81936 -
        Flags: superreview+

    
    

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


    
    

    
  
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.
Keywords: fixed1.0.0

    
    

    
  
cc'ing myself

    
    

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

Gerv

    
    

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

    
    

    
  
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

    
    

    
  
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
Whiteboard: [Hixie-P3] → [Hixie-P3][CSS1-6.3]

    
  
Flags: blocking1.3a?

    
  
Flags: blocking1.3a?
Flags: blocking1.3a-

    
  
Flags: blocking1.4a?

    
  
Flags: blocking1.4a? → blocking1.4a-

    
  
Target Milestone: mozilla1.0 → ---

    
    

    
  
tor: What's the status on this?
QA Contact: ian → style-system

    
    

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

    
    

    
  
>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.
Blocks: 418538

    
    

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

    
    

    
  
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

    
    

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

    
    

    
  
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?

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
Resolving this mammoth bug as fixed. Discussion about the faint differences with iCCP chunks can continue in bug 454688.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED

    
    

    
  
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.
See Also:  → 621474

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: