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

[Jose Fonseca]

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


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.


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?


This is all quite confusing..


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.

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


Jose

> +      }
> +      break;
> +   case GL_LUMINANCE_ALPHA:
> +      for (i = 0; i < n; i++) {
> +         rgba[i][GCOMP] = 0.0F;
> +         rgba[i][BCOMP] = 0.0F;
> +      }
> +      break;
> +   default:
> +      /* no-op */
> +      ;
> +   }
> +}
> +
> +
> +/**
> + * As above, but GLuint components.
> + */
> +void
> +_mesa_rebase_rgba_uint(GLuint n, GLuint rgba[][4], GLenum
> baseFormat)
> +{
> +   GLuint i;
> +
> +   switch (baseFormat) {
> +   case GL_ALPHA:
> +      for (i = 0; i < n; i++) {
> +         rgba[i][RCOMP] = 0;
> +         rgba[i][GCOMP] = 0;
> +         rgba[i][BCOMP] = 0;
> +      }
> +      break;
> +   case GL_INTENSITY:
> +      /* fall-through */
> +   case GL_LUMINANCE:
> +      for (i = 0; i < n; i++) {
> +         rgba[i][GCOMP] = 0;
> +         rgba[i][BCOMP] = 0;
> +         rgba[i][ACOMP] = 1;
> +      }
> +      break;
> +   case GL_LUMINANCE_ALPHA:
> +      for (i = 0; i < n; i++) {
> +         rgba[i][GCOMP] = 0;
> +         rgba[i][BCOMP] = 0;
> +      }
> +      break;
> +   default:
> +      /* no-op */
> +      ;
> +   }
> +}
> +
> +
> diff --git a/src/mesa/main/pack.h b/src/mesa/main/pack.h
> index b1853cd..cd49c74 100644
> --- a/src/mesa/main/pack.h
> +++ b/src/mesa/main/pack.h
> @@ -149,4 +149,11 @@ _mesa_pack_rgba_span_int(struct gl_context *ctx,
> GLuint n, GLuint rgba[][4],
>                           GLenum dstFormat, GLenum dstType,
>                           GLvoid *dstAddr);
>  
> +
> +extern void
> +_mesa_rebase_rgba_float(GLuint n, GLfloat rgba[][4], GLenum
> baseFormat);
> +
> +extern void
> +_mesa_rebase_rgba_uint(GLuint n, GLuint rgba[][4], GLenum
> baseFormat);
> +
>  #endif
> --
> 1.7.3.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>