Created attachment 59866 [details]
A sample image

Created attachment 59867 [details]
the same image with contrast reduced in cinepaint 

I get a similar with other tools such as Krita and OpenOffice.

Created attachment 59868 [details]
the same image with constrast reduced by The Gimp

The only other tool I know with a similar behavior is Gthumb but it is likely that Gthumb borrowed its implementation from The Gimp.

Created attachment 59870 [details] [review]
simple patch that gives good result

The patch only changes the negative half of the constrast function. 

The positive half (the one that increases the contrast) is not modified. I do not know if the formula is correct but the result looks consistant with the other tools.

Actually there is some dodgy stuff in brightness_contrast_lut_func(). I was always under the assumption, that increasing contrast means a linear transform of the value, now when reading the code it seems that this is something polynomial?

I need to plot some graphs of what is intended there to understand this.

The patch from Stephane introduces the linear transformation I was thinking of. If we go that route we should also use this for increasing the contrast.

Created attachment 59873 [details] [review]
Patch to restore the intended behaviour of contrast reduction

Ok, It is a stupid typo, patch is attached.

However, I think it would be more sane to use extend Stephanes approach to the increasing contrast case as well: It is the more widely accepted definition of contrast and means less code.

I believe that the most important is to insure that the brightness and constrast formulas are preservig the HUE. The lut functions is not necessarily appropriate because it applies an independant function to each channel. 

If we consider a RBG color (a,b,c) where a,b,c are the R,G,B channels ordered such that a <= b <= c then 

   Value      = c
   Saturation = (c-a)/a 
   Hue        = constant + (b-a)/(c-a) = 

f(k) = k+(k-0.5)*N  is the linear lut function for reducing the contrast by a factor N.

NEW HUE = 
= constant + (f(c)-f(a))/(f(b)-f(a)) 
= constant + ((c+(c-0.5)*N)-(a+(a-0.5)*N))/((b+(b-0.5)*N)-(a+(a-0.5)*N))
= constant + (c+N*c-N/2-a-a*N+N/2)/(b+N*b-N/2-a-a*N+N/2)
= constant + (c+N*c-a-a*N)/(b+N*b-a-a*N)
= constant + ((c-a)*(N+1))/((b-a)*(N+1))
= constant + (c-a)/(b-a)
= OLD HUE

So using a linear function to reduce the constrat preserves the HUE if we except the rounding errors.

The function used to increase the contrast is a lot more complex but it is easy to see that it cannot possibly preserve the HUE. The purpose of that function is to shift the channels < 0.5 in the direction of 0.0 and the channels > 0.5 in the direction of 1.0. 
So a constrast of +1.0 is always going to produce a color in which each of the 3 channels a, b & c is either 0.0 or 1.0. That's black, white or one of the 6 primary colors. The HUE cannot be preserved.







New HUE = 

Correcting the GIMP to do what the rest of the world expects to be contrast
adjustment is probably the sanest thing to do. Attaching a patch that would
result in "normal" behavior.

Created attachment 59875 [details] [review]
Patch to implement linear brightness + contrast

The modified patch seems ok to me. 
It probably does not preserve the HUE but it is close enough.
I am not familiar with that formula but it is very quickly asymptotic with my linear formula. I do not believe that it preserves the HUE but it is close enough.
But to be honest, I suspect that this formula is probably more appropriate in HSV space than in RBG.
   

The patch 59875 is totally broken :-) 

Created attachment 59878 [details] [review]
Patch to create linear contrast behaviour.

Yeah, the patch does not care about the range of the contrast parameter given to the lut function (-1 .. 1). This patch does it correctly, even using the tangens to have a more or less smooth transition. (-1 gets mapped to slant 0, 0 gets mapped to slant 1, +1 gets mapped to slant infinity...  :)

Humm.. I do not think that the last patch is good either since the brightness is not even multiplied by the initial value.
Anyways, I do not see how both the positive and negative cases for the brightness could be described using a single linear formula. 
What do you think of that? 
   
  if (data->brightness < 0.0)
    value = value * (1.0 + data->brightness);
  else
    value = value + ((1.0 - value) * data->brightness);

  slant = tan ((data->contrast + 1) * G_PI_4);
  value = (value - 0.5) * slant + 0.5 ;

Uh, Indeed the old version applied the brightness before changing the contrast.

I am not sure if this is correct at all. Changing the brightness usually refers to an additive offset to the values. The version you sketched (and as in the GIMP originally) does add an offset and reduces contrast simultaneously. This seems wrong.

Not sure what to do.

It just depends how you define the brightness. 
If you define Brightness as the HSV Value then the formula is correct since it increases/decreases the Value while preserving Saturation and Hue.
 
  

After a good night of sleep, I came to the conclusion that implementing the brightness using an additive offset could make sense.
However, the only way to achieve that without changing the HUE by clipping the R,G,B channels would be to drop the 'lut' approach.

The brightness/Contrast tools should be implemented with a function that operates in HSV space:

   gimp_rgb_to_hsv(&a,&b,&c) ;
   c = curve(c) ;
   gimp_hsv_to_rgb(&a,&b,&c) ;  
      
More generally, I thing that Gimp needs a real HSV Value curve which is more appropriate than the current RGB Value(s) curve for that kind of manipulations.
From that curve, it should be trivial to implement a Saturation curve.
And of course a Hue curve which would be very cool for changing colors. 

LUTs operating in different colorspaces would make a lot of sense and should be implemented at some point. However, for this bug it probably is overkill.

Stephane: adjusting brightness and contrast should be done in RGB since that is the native color model of the GIMP, extra conversions will introduce extra quantization errors. HSV is probably one of the worse options if it was to be done with a different model since Value = MAX(R,MAX(G,B)), and not a weighted average of linearized components which would be a proper lumninance calculation.

I started development on a plug-in[1] for doing color corrections with the YCbCr color model a couple of years ago. I still think this approach would provide a good color correction tool, but both the ui code and the actual implementation has issues.

1: http://pippin.gimp.org/plug-ins/color_correct/

Øyvind: going through HSV for adjusting the brigthness won't introduce any quantization because only the Value has to be adjusted.

I proposed the following formula:

   gimp_rgb_to_hsv(&a,&b,&c) ;
   c = curve(c) ;
   gimp_hsv_to_rgb(&a,&b,&c) ;  


But in fact an equivalent implementation is 
   h  = MAX(r,g,b) 
   coef = curve(h)/h ;
   r = r*coef ;
   v = v*coef ;   
   b = b*coef ;

As you can see, the r, g & b channels are simply multipled by a coefficient.
This is not very difference from the LUT approach except that applying the same  coefficient to r, g & b insures that any curve (even a non-linear one) preserves the hue and saturation.

For example, Simon proposed to implement the brightness with an additive offset. That implementation would be
 
   h  = MAX(r,g,b) 
   h2 = h+OFFSET ;
   if (h2>255) h2=255 ;
   else if (h2<0) h2=0 ;
   coef = h2/h ;
   r = r*coef ;
   v = v*coef ;   
   b = b*coef ;

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

To address the bugginess of the current behaviour for lowering contrast, I've gone the route in comment #13:

2006-02-24  Simon Budig  <simon@gimp.org>

        * app/base/lut-funcs.c: change the LUT function for the contrast.
        Fixes the buggy behaviour as described in bug #319872 and
        bug #332068 and makes the behaviour consistent with the standard
        contrast formula. However, I am leaving the bug open to discuss
        further improvements.

Are further improvements needed or can we close this report now?

Quite remarkable to have 22 comments and a fix, all for a bug report that is still "unconfirmed"!  I am going to resolve as FIXED since two months have gone by without any issues being raised.  Can reopen if necessary, or start a new bug report if there are new problems.

I'm implementing contrast manipulation in gnome-mag and I did it in a simpler manner, but take almost the same results as with the one implemented in gimp, although I think that I make the filter linear. I did it in the following way:

#define CLAMP_LOW_MID(v) (t = (v), CLAMP (t, 0, 127))
#define CLAMP_MID_HIGH(v) (t = (v), CLAMP (t, 127, 255))

if (value <= 127) then
    value = CLAMP_LOW_MID (value - data->contrast * 127);
else
    value = CLAMP_MID_HIGH (value + data->contrast * 127);

Comments on this are very appreciate. If someone want's to look the patch it can be found in http://bugzilla.gnome.org/show_bug.cgi?id=348629

Carlos, perhaps you could open a new bug report and start it with a small summary of the current state, taking into account what has been discussed here and what's in CVS ? This report has grown quite large and I would like to avoid having to reopen it.