[PATCH 2/3] xrandr: fix gamma == 1.0 && sigbits != 8

[Andy Ritger]

On Wed, Aug 15, 2012 at 08:55:40AM -0700, walter harms wrote:
> 
> 
> Am 14.08.2012 01:25, schrieb Andy Ritger:
> > The gamma-correction lookup table managed through XRR[GS]etCrtcGamma is
> > 2^n in size, where 'n' is the number of significant bits in the X Color.
> > Each element in the gamma-correction lookup table is a 16:16:16 X Color
> > (i.e., in the range [0,65536) ).  The significant bits of each component
> > of each element in the lookup table are programmed into the hardware
> > lookup table.  Meaningful values in the gamma-correction lookup table
> > are thus in the range [0,2^sigbits), where all values are shifted into
> > the MSBs (i.e., left shifted by (16 - sigificant bits)).
> > 
> > Signed-off-by: Andy Ritger <aritger at nvidia.com>
> > Reviewed-by: Aaron Plattner <aplattner at nvidia.com>
> > ---
> >  xrandr.c |   15 +++++++++++----
> >  1 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xrandr.c b/xrandr.c
> > index 75ed2ee..5709ded 100644
> > --- a/xrandr.c
> > +++ b/xrandr.c
> > @@ -1323,7 +1323,7 @@ set_gamma(void)
> >      output_t	*output;
> >  
> >      for (output = outputs; output; output = output->next) {
> > -	int i, size;
> > +	int i, size, shift;
> >  	crtc_t *crtc;
> >  	XRRCrtcGamma *gamma;
> >  	float gammaRed;
> > @@ -1347,6 +1347,13 @@ set_gamma(void)
> >  	    continue;
> >  	}
> >  
> > +	/*
> > +	 * The hardware color lookup table has a number of significant
> > +	 * bits equal to ffs(size) - 1; shift values so that they
> > +	 * occupy the MSBs of the 16-bit X Color.
> > +	 */
> > +	shift = 16 - (ffs(size) - 1);
> > +
> 
>        you assume ffs(size) >16, is that save ?

Thanks for taking a look.

I believe it is safe to assume that (ffs(size) - 1) <= 16; here is why:

> > The gamma-correction lookup table managed through XRR[GS]etCrtcGamma is
> > 2^n in size, where 'n' is the number of significant bits in the X Color.

Since a single component of an X Color is 16 bits, the largest number of
possible significant bits is 16.  Thus, the largest possible size returned
by XRRGetCrtcGammaSize() would be 2^16, and (ffs(2^16) - 1) == 16.

Maybe putting that in a comment and adding a sanity check would help?

    /*
     * The gamma-correction lookup table managed through XRR[GS]etCrtcGamma
     * is 2^n in size, where 'n' is the number of significant bits in
     * the X Color.  Because an X Color is 16 bits, size cannot be larger
     * than 2^16.
     */
    if (size > 65536) {
        fatal("Gamma correction table is impossibly large.\n");
        continue;
    }

Thanks,
- Andy


> 	re,
>  	wh
> 
> 
> >  	gamma = XRRAllocGamma(size);
> >  	if (!gamma) {
> >  	    fatal("Gamma allocation failed.\n");
> > @@ -1366,21 +1373,21 @@ set_gamma(void)
> >  
> >  	for (i = 0; i < size; i++) {
> >  	    if (gammaRed == 1.0 && output->brightness == 1.0)
> > -		gamma->red[i] = (i << 8) + i;
> > +		gamma->red[i] = (i << shift);
> >  	    else
> >  		gamma->red[i] = dmin(pow((double)i/(double)(size - 1),
> >  					 gammaRed) * output->brightness,
> >  				     1.0) * 65535.0;
> >  
> >  	    if (gammaGreen == 1.0 && output->brightness == 1.0)
> > -		gamma->green[i] = (i << 8) + i;
> > +		gamma->green[i] = (i << shift);
> >  	    else
> >  		gamma->green[i] = dmin(pow((double)i/(double)(size - 1),
> >  					   gammaGreen) * output->brightness,
> >  				       1.0) * 65535.0;
> >  
> >  	    if (gammaBlue == 1.0 && output->brightness == 1.0)
> > -		gamma->blue[i] = (i << 8) + i;
> > +		gamma->blue[i] = (i << shift);
> >  	    else
> >  		gamma->blue[i] = dmin(pow((double)i/(double)(size - 1),
> >  					  gammaBlue) * output->brightness,
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel