[Mesa-dev] [PATCH v3 5/8] mesa pack: handle packed integer formats with clamping

[Brian Paul]

On 07/23/2012 10:59 AM, Jordan Justen wrote:
> Signed-off-by: Jordan Justen<jordan.l.justen at intel.com>
> ---
>   src/mesa/main/pack.c |  549 +++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 547 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/main/pack.c b/src/mesa/main/pack.c
> index efbff5d..a599f21 100644
> --- a/src/mesa/main/pack.c
> +++ b/src/mesa/main/pack.c
> @@ -521,6 +521,8 @@ _mesa_pack_rgba_span_from_uints(struct gl_context *ctx, GLuint n, GLuint rgba[][
>                                   GLenum dstFormat, GLenum dstType,
>                                   GLvoid *dstAddr)
>   {
> +   GLuint i;
> +
>      switch(dstType) {
>      case GL_UNSIGNED_INT:
>         pack_uint_from_uint_rgba(ctx, dstAddr, dstFormat, rgba, n);
> @@ -540,7 +542,278 @@ _mesa_pack_rgba_span_from_uints(struct gl_context *ctx, GLuint n, GLuint rgba[][
>      case GL_BYTE:
>         pack_byte_from_uint_rgba(ctx, dstAddr, dstFormat, rgba, n);
>         break;
> -
> +   case GL_UNSIGNED_BYTE_3_3_2:
> +      if ((dstFormat == GL_RGB) || (dstFormat == GL_RGB_INTEGER)) {
> +         GLubyte *dst = (GLubyte *) dstAddr;
> +         for (i=0;i<n;i++) {
> +            dst[i] = (CLAMP(rgba[i][RCOMP], 0, 7)<<  5)
> +                   | (CLAMP(rgba[i][GCOMP], 0, 7)<<  2)
> +                   | (CLAMP(rgba[i][BCOMP], 0, 3)     );
> +         }
> +      }

I think we should have an else clause here and in similar places below 
to catch unexpected format/type combinations:

    else {
       _mesa_problem(ctx, "unexpected dstFormat/dstType in 
_mesa_pack_rgba_span_from_uints()");
    }

Actually, since there'd be many instances of this, maybe use a 'goto 
badformattype' error handler at the end of the function.

Otherwise, the patch looks good (though I didn't inspect every line of 
clamping/shifting in detail.  Do we have a piglit tests that hit all 
(most?) of these cases?

Reviewed-by: Brian Paul <brianp at vmware.com>