[Mesa-dev] [PATCH 1/3] mesa: add _mesa_rebase_rgba_float/uint() functions

[Brian Paul]

On 03/01/2012 12:42 AM, Jose Fonseca wrote:
> ----- Original Message -----
>> These will be used by glReadPixels() and glGetTexImage() to fix
>> issues
>> with reading GL_LUMINANCE and other formats.
>> ---
>>   src/mesa/main/pack.c |   91
>>   ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   src/mesa/main/pack.h |    7 ++++
>>   2 files changed, 98 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/mesa/main/pack.c b/src/mesa/main/pack.c
>> index 41485a1..4d4b4a8 100644
>> --- a/src/mesa/main/pack.c
>> +++ b/src/mesa/main/pack.c
>> @@ -5250,3 +5250,94 @@ _mesa_unpack_image( GLuint dimensions,
>>      }
>>   }
>>
>> +
>> +
>> +/**
>> + * If we unpack colors from a luminance surface, we'll get pixel
>> colors
>> + * such as (l, l, l, a).
>> + * When we call _mesa_pack_rgba_span_float(format=GL_LUMINANCE),
>> that
>> + * function will compute L=R+G+B before packing.  The net effect is
>> we'll
>> + * accidentally store luminance values = 3*l.
>> + * This function compensates for that by converting (aka rebasing)
>> (l,l,l,a)
>> + * to be (l,0,0,a).
>> + * It's a similar story for other formats such as LUMINANCE_ALPHA,
>> ALPHA
>> + * and INTENSITY.
>> + *
>> + * Finally, we also need to do this when the actual surface format
>> does
>> + * not match the logical surface format.  For example, suppose the
>> user
>> + * requests a GL_LUMINANCE texture but the driver stores it as RGBA.
>> + * Again, we'll get pixel values like (l,l,l,a).
>> + */
>> +void
>> +_mesa_rebase_rgba_float(GLuint n, GLfloat rgba[][4], GLenum
>> baseFormat)
>> +{
>> +   GLuint i;
>> +
>> +   switch (baseFormat) {
>> +   case GL_ALPHA:
>> +      for (i = 0; i<  n; i++) {
>> +         rgba[i][RCOMP] = 0.0F;
>> +         rgba[i][GCOMP] = 0.0F;
>> +         rgba[i][BCOMP] = 0.0F;
>> +      }
>> +      break;
>> +   case GL_INTENSITY:
>> +      /* fall-through */
>> +   case GL_LUMINANCE:
>> +      for (i = 0; i<  n; i++) {
>> +         rgba[i][GCOMP] = 0.0F;
>> +         rgba[i][BCOMP] = 0.0F;
>> +         rgba[i][ACOMP] = 1.0F;
>
> Is it really OK to set A == 1 for GL_INTENSITY? Imagine that baseFormat == INTENSITY, and the user is requesting LUMINANCE_ALPHA. It seems this would return L = I, A = 1, but I'd expect to get L = A = I.
>
> I think that the
>
>    rgba[i][ACOMP] = 1.0F
>
> should be simply removed here, as alpha values either have the right value, or will be ignored.

Actually, table 6.1 of the 2.1 spec says that luminance and intensity 
values both map to rgba as (x, 0, 0, 1).

But also note that GL_INTENSITY is not a legal format for glReadPixels 
or glGetTexImage().

In any case I was just moving the code over from texgetimage.c and 
consolidated the two cases when I saw they were doing the same thing. 
  So, we've been doing this for a while and nobody's complained. 
These are fairly obscure paths.

I'll have to write a test program to see what other drivers do.


> Also, the 2.1 spec, section 4.3.2 "Reading Pixels", says
>
>    Conversion to L
>
>    This step applies only to RGBA component groups, and only if the format is either
>    LUMINANCE or LUMINANCE_ALPHA. A value L is computed as
>
>      L=R+G+B
>
>    where R, G, and B are the values of the R, G, and B components. The single
>    computed L component replaces the R, G, and B components in the group.
>
> I couldn't find similar text about INTENSITY.  So I wonder if the INTENSITY is really the same -- perhaps intensity packing functions should not do the R + G + B operation at all, and it looks they don't.

Since you can't 'get' GL_INTENSITY images, it's moot.  I think a user 
would just use GL_LUMINANCE instead if they wanted to read back an 
intensity surface and they'd see the expected values.


> Also, when baseFormat = GL_LUMINANCE and the user is calling glReadPixels(GL_RGBA), won't _mesa_rebase_rgba_float() wrongly zero G and B components?

I'll have to test that with another driver to be sure.  But again, 
table 6.1 would seem to say "no".


> This is all quite confusing..

Yup.


> I wonder if instead of doing this way (ie, always do the L = R + G + B, but clear G&  B components when L = R is intended), it would be simpler (and more efficient) to do the opposite: have the L/LA pack functions do just L = R, and when L = R + G + B is really intended either:
> - insert a R = R + G + B step between pack and unpack calls
> - only do the L = R + G + B inside the transfer ops case, and fallback to that we.
> - or pass a flag (or baseFormat) to the L/LA pack functions to choose.

I considered that too, but I wanted to do the least invasive change 
for the time being, basically reusing the code from glGetTexImage for 
glReadPixels.


> This would be not only simpler but faster, as I get the impression that the most common case is L = R.

I'll do some test with another driver and see what happens.  This 
might have to wait until next week though.

-Brian