Bug 4991

Summary:	glReadPixels with 16 bits per channel - truncation error
Product:	Mesa	Reporter:	Roy Walmsley <roy.walmsley>
Component:	Mesa core	Assignee:	mesa-dev
Status:	RESOLVED FIXED	QA Contact:
Severity:	minor
Priority:	low
Version:	git
Hardware:	x86 (IA32)
OS:	Windows (All)
Whiteboard:
i915 platform:		i915 features:

Description Roy Walmsley 2005-11-08 08:45:51 UTC

While using glReadPixels at 16 bits per channel we discovered that pixel values 
are often decremented by 1 when read from the frame buffer. This was found to 
be because the read routines called a generic function to read a row of pixels. 
The data was converted to float values using the macro CHAN_TO_FLOAT defined in 
colormac.h. These values were subsequently converted back to unsigned short 
using the macro FLOAT_TO_USHORT defined in macros.h.

So, for a pixel value of 2, the first conversion gave a float value of 3.0518E-
5. On using the second macro the first stage is a multiplication of the stored 
value by 65535 to give a value of about 1.999997. This is then truncated to 
give a result of 1.

I corrected the problem for my own use by redefining this macro as below.

#define FLOAT_TO_USHORT(X)  ((GLushort) (GLint) ((X) * 65535.0F + 1.0E-6F))

This then gives the correct values.

Comment 1 Brian Paul 2005-11-08 16:41:43 UTC

Instead of changing the FLOAT_TO_USHORT macro, would using the
CLAMPED_FLOAT_TO_USHORT() macro work instead?  I realize the return value is
handled differently.

I'm thinking that we should totally get rid of FLOAT_TO_USHORT() and use either
UNCLAMPED_FLOAT_TO_USHORT() or CLAMPED_FLOAT_TO_USHORT() in all cases.  Both of
those do rounding to the nearest int, which is what the GL spec calls for anyway.

Comment 2 Roy Walmsley 2005-11-09 02:33:00 UTC

I have undone the change in macros.h to FLOAT_TO_USHORT and instead changed the 
macros called in image.c from FLOAT_TO_USHORT to CLAMPED_FLOAT_TO_USHORT as you 
suggested and this works fine. I chose to use the CLAMPED rather than the 
UNCLAMPED macro purely in the interests of efficiency as, certainly in my case, 
values should already be clamped as they are the outputs from the rendering 
process.

Comment 3 Brian Paul 2005-11-09 08:31:14 UTC

OK, I've made this change in CVS for the next release.

Comment 4 Adam Jackson 2009-08-24 12:23:33 UTC

Mass version move, cvs -> git

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.