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

[Dave Airlie]

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

Pushed with edits: comments below:

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

Yeah we should have a look at those, I've dropped the unclamped in the
version I pushed.

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

Done as a follow-on. Some cleanup on drivers could be done.

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

called it gl_color_union for want of a good name, took the border
color union out.

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

Done as well.

Thanks for reviewing.
Dave.