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

[Jose Fonseca]

----- Original Message -----
> 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.

Fair enough. If it doesn't cause regressions then I think its fine to go as is, and do further improvements in follow on change.

> > 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.

Yes, I think comparing against drivers it's the only way to get to the bottom of this, as the spec is a patchwork of apparently conflicting statements in this regard.

Jose