Bug 46631

Created attachment 57646 [details] [review]
readpix: Don't disable fast path for normalized types

Mesa has a fast path for the generic fallback when using glReadPixels
for RGBA data which uses memcpy. However it was really difficult to
hit this case because it would not be used if any transferOps are
enabled. Any type apart from floating point or non-normalized integer
types (so any of the common types) would force enabling clamping so
the fast path could not be used. This patch makes it ignore clamping
when determining whether to use the fast path if the data type of the
buffer is an unsigned normalized type because in that case clamping
will not have any effect anyway.

This patch additionally makes it explicitly ignore luminance types
because in that case the conversion to and from RGB does not end up
with exactly the same values.

The transferOps change looks great to me. Good catch.

I'm unsure about LUMINANCE though.

The glReadPixels manpage indeed says that reading from RGBA renderbuffer to LUMINANCE pixels, the resulting luminance values should have the addition of R+G+B. But:

b) LUMINANCE -> LUMINANCE should still be a ordinary memcpy, and your change would prevent that

c) _mesa_format_matches_format_and_type should already fail for LUMINANCE -> RGB or RGB -> LUMINANCE . If it doesn't that there's where it needs to be fixed.

So I'd suggest you, to split the transferOps change and luminance in two patches, and provide a concrete example where the wrong things was being done for luminance.

Created attachment 57665 [details] [review]
readpix: Don't disable fast path for normalized types

Mesa has a fast path for the generic fallback when using glReadPixels
for RGBA data which uses memcpy. However it was really difficult to
hit this case because it would not be used if any transferOps are
enabled. Any type apart from floating point or non-normalized integer
types (so any of the common types) would force enabling clamping so
the fast path could not be used. This patch makes it ignore clamping
when determining whether to use the fast path if the data type of the
buffer is an unsigned normalized type because in that case clamping
will not have any effect anyway.

Created attachment 57666 [details] [review]
readpix: Don't use the fast path for luminance types

When deciding whether to use the fast memcpy path for the fallback
implementation of glReadPixels, it now explicitly ignores luminance
types because in that case the conversion to and from RGB does not end
up with exactly the same values. Although it seems counter-intuitive,
the spec implies that even when the source and dest formats are a
luminance type, it will temporarily convert to RGB by storing the
luminance value in every component and then convert back to luminance
by adding all of the components together. This effectively means the
luminance values must be multiplied by three to conform to the spec.

Created attachment 57667 [details]
Test case showing reading luminance values

Ok, I've split the patch into two.

I'm attaching a test case which shows what happens when luminance values are read from an FBO wrapping a luminance texture. The texture is created with a single pixel with the value 1. When the pixel is read back from glReadPixels it becomes 3, which implies Mesa really is adding the components together. I can only test this with the software rasterizer because the Intel driver doesn't seem to like creating an FBO to a luminance texture.

The GL spec just says it reads luminance values by putting the value in all three of the components and writes luminance values by adding the components together so I think Mesa is doing the right thing here, even though in practice this is probably not very helpful. If the spec meant for it to be any other way presumably there would have to mention a special case when the formats are both the same, but I could not find this.

Interestingly however, if I try this with the Nvidia proprietary drivers it returns 1, so it is just directly copying the data. It seems to me like this is a bug in Nvidia. Presumably nobody cares because the spec makes no sense.

(In reply to comment #5)
> Created attachment 57667 [details]
> Test case showing reading luminance values
>
> I'm attaching a test case which shows what happens when luminance values are
> read from an FBO wrapping a luminance texture. The texture is created with a
> single pixel with the value 1. When the pixel is read back from glReadPixels it
> becomes 3, which implies Mesa really is adding the components together.
> 

This seems wrong. If you specify luminance as 1 and read back luminance, you should receive 1.

If you're reading GL_LUMINANCE from an RGB texture, adding the values L = R + G + B is certainly conformant, but if the texture *is* LUMINANCE, the sum has already been performed.

If you supply RGB values to TexImage, you would have to supply (for instance) R = G = B = 1/3 to achieve luminance 1, and adding those 3 * 1/3 back together ... it's a bug in mesa.

(In reply to comment #6)
> 
> This seems wrong. If you specify luminance as 1 and read back luminance, you
> should receive 1.

That makes sense to me too.

> If you're reading GL_LUMINANCE from an RGB texture, adding the values L = R + G
> + B is certainly conformant

I understand things it should do:
Y = (R*RedScale+RedBias)+(G*GreenScale+GreenBias)+(B*BlueScale+BlueBias)

Where you would normally want to set the scales to something sane, like 0.299, 0.587, 0.114.

Settings like 1/3 for the scales don't make sense to me if you really want luminance, but would clearly be better than the default of 1.  With a scale
of 1 you would have a problem with the clamping to [0,1].

> If you supply RGB values to TexImage, you would have to supply (for instance) R
> = G = B = 1/3 to achieve luminance 1, and adding those 3 * 1/3 back together
> ... it's a bug in mesa.

No, you set R, G and B to 1, and set the scales with glPixelTransferf() to get the desired effect.


Kurt

Ok, I think I understand this a bit better now.

I think the problem is that Mesa is doing the wrong thing when converting from the framebuffer format to the temporary floating point RGBA representation. According to the GL 3.0 spec in the section on reading pixels, it says “Internal components are converted to an RGBA color by taking each R, G, B, and A component present according to the base internal format of the buffer (as shown in table 3.15). If G, B, or A values are not present in the internal format, they are taken to be zero, zero, and one respectively”. Table 3.15 shows that the L component of an internal luminance format maps to only the R component. Mesa is currently mapping the luminance value to all three components. You can see that on the proprietary Nvidia driver it only maps to the R component because if you set the R scale to 0.0 when reading the pixels you get 0.0 back for the luminance.

So I guess we could make a patch that changes the luminance functions in format_unpack.c to only copy to the R component. However I'm not sure if this will break other things because those functions are also used in a lot of other places. It is clearly a separate bug though so for now I'll just make the luminance patch obsolete and we can file another bug sometime.

(In reply to comment #3)
> readpix: Don't disable fast path for normalized types

AFAICT this patch should avoid the problems introduced by my similar commit b11c16752a18ef8dfb96d9f0ead6ecb62bde6773 ('read_rgba_pixels: Don't force clamping if the renderbuffer is normalized.'). If it doesn't cause any regressions with piglit quick.tests, I think it's good to go.

I've filed a separate bug for the problem with luminance as bug 46679.

I tried running the patch with piglit and the following four tests regressed:

glsl-fs-fragcoord
glsl-fs-sqrt-zero
fbo-generatemipmap-formats
time-elapsed

However if I run them by hand with the command line it shows they all pass. Is running piglit normally pretty reliable? I haven't really used it that much.

Neil, the readpix patch looks good.  Can you commit it?  I'd suggest changing the commit message to start with "mesa:" instead of "readpix:" since this is a core Mesa change.

I'm not sure what's causing the sporadic piglit failures, but I doubt it's related to your patch.

Thanks for digging into this.

(In reply to comment #10)
> I've filed a separate bug for the problem with luminance as bug 46679.
> 
> I tried running the patch with piglit and the following four tests regressed:
> 
> glsl-fs-fragcoord
> glsl-fs-sqrt-zero
> fbo-generatemipmap-formats
> time-elapsed
> 
> However if I run them by hand with the command line it shows they all pass. Is
> running piglit normally pretty reliable? I haven't really used it that much.

Piglit may randomly fail some tests due to driver bugs (e.g. synchronization issues). It's quite common with some drivers.

(In reply to comment #11)
> Can you commit it?

I don't have commit access on Mesa unfortunately. I've just asked for access (bug 46683) so we can wait for that or if someone wants to commit it sooner that'd be good too :)

OK, committed.  d9c42097770f173804c7c7c40bf8bc6c4400673b

(In reply to comment #14)
> OK, committed.  d9c42097770f173804c7c7c40bf8bc6c4400673b

Would it be possible to get this into the 7.11 branch as well? For gnome-shell's screen recorder that makes the difference between useless and usable on intel hardware.