[Mesa-dev] [PATCH] mesa: introduce a clear color union to be used for int/unsigned buffers

[Brian Paul]

On 09/13/2011 04:54 AM, Dave Airlie wrote:
> From: Dave Airlie<airlied at redhat.com>
>
> This introduces a new gl_clear_color union and moves the current
> ClearColorUnclamped to use it, it removes ClearColor completely and
> all drivers are modified to expected unclamped floats instead.
>
> also fixes st to use translated color in one place it wasn't.
>
> Signed-off-by: Dave Airlie<airlied at redhat.com>
> ---
>   src/mesa/drivers/common/meta.c                |   10 ++--
>   src/mesa/drivers/dri/intel/intel_blit.c       |   10 ++--
>   src/mesa/drivers/dri/nouveau/nouveau_driver.c |    2 +-
>   src/mesa/drivers/dri/nouveau/nouveau_util.h   |   10 +++
>   src/mesa/drivers/dri/nouveau/nv20_context.c   |    2 +-
>   src/mesa/drivers/dri/r200/r200_state.c        |   11 ++--
>   src/mesa/drivers/dri/radeon/radeon_state.c    |   11 ++--
>   src/mesa/drivers/windows/gdi/wmesa.c          |    9 ++-
>   src/mesa/drivers/x11/xm_dd.c                  |   37 +++++-----
>   src/mesa/main/attrib.c                        |    8 +-
>   src/mesa/main/blend.c                         |    3 +-
>   src/mesa/main/clear.c                         |   94 ++++++++++--------------
>   src/mesa/main/dd.h                            |    3 +-
>   src/mesa/main/get.c                           |   11 ++-
>   src/mesa/main/mtypes.h                        |    9 ++-
>   src/mesa/state_tracker/st_cb_clear.c          |   14 ++--
>   src/mesa/state_tracker/st_format.c            |    9 +++
>   src/mesa/state_tracker/st_format.h            |    3 +
>   src/mesa/swrast/s_clear.c                     |   47 +++++++------
>   19 files changed, 163 insertions(+), 140 deletions(-)


Looks pretty good, Dave.

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

I'd be OK with committing as-is, but I'd suggest a few minor things:

1. Do we really need to call the field ClearColorUnclamped?  I think 
we could keep it as ClearColor and just add a comment that values are 
not clamped.  But there are a bunch of other dual clamped/unclamped 
fields in Mesa now.  We should probably review all of those and see 
where we can consolidate the clamped/unclamped values.

2. There seem to be many occurances of code like this:

   UNCLAMPED_FLOAT_TO_UBYTE(color[0], c.f[0]);
   UNCLAMPED_FLOAT_TO_UBYTE(color[1], c.f[1]);
   UNCLAMPED_FLOAT_TO_UBYTE(color[2], c.f[2]);
   UNCLAMPED_FLOAT_TO_UBYTE(color[3], c.f[3]);

I think it would be good to have a helper function like this

static inline void
_mesa_float_rgba_to_ubyte(GLubyte dst[4], const GLfloat src[4])
{
...
}

in colormac.h to simply things.  That could be done in a follow-on commit.

3. gl_texture_object::BorderColor is also a union of float/int/uint. 
Maybe we should rename gl_clear_color to something slightly more 
generic and use it for BorderColor too.  It might be used in other 
places in the future too.

4. I don't think the st_translate_clear_color() function is needed. 
It's just a wrapper for st_translate_color().

-Brian