Thanks for taking a look at this, Jose.  Comments/questions below...

On 09/14/2011 09:19 AM, Jose Fonseca wrote:
Hi Brian,

----- Original Message -----
From: Brian Paul<bri...@vmware.com>

---
  src/gallium/auxiliary/util/u_format.c |   77
  +++++++++++++++++++++++++++++++++
  src/gallium/auxiliary/util/u_format.h |   16 +++++++
  2 files changed, 93 insertions(+), 0 deletions(-)

diff --git a/src/gallium/auxiliary/util/u_format.c
b/src/gallium/auxiliary/util/u_format.c
index 34922ab..2bd3737 100644
--- a/src/gallium/auxiliary/util/u_format.c
+++ b/src/gallium/auxiliary/util/u_format.c
@@ -67,6 +67,83 @@ util_format_is_float(enum pipe_format format)
  }


+/** Test if the format contains RGB, but not alpha */
+boolean

It might be better to change  the name to util_format_is_rgb_no_alpha, as 
is_rgb sounds more like a RGB colorspace inquiry.

will do.


+util_format_is_rgb(enum pipe_format format)
+{
+   const struct util_format_description *desc =
+      util_format_description(format);

For callers that call these helpers a lot, all these internal calls to util_format_description() 
will be inefficient. In such cases it might be preferrable for these functions to be declared 
inline, and take a "const struct util_format_description *desc" argument instead of 
"enum pipe_format format", allowing the caller will call util_format_description only 
once.

The only caller of these functions now is the sp blend code and it saves the results to avoid frequent calls.

Most of the existing util_format_is_XXX() calls take a pipe_format now. I think we should be consistent and have all of them take a util_format_description or pipe_format. Either way is fine with me. We can revisit that later.


+
+   if ((desc->colorspace == UTIL_FORMAT_COLORSPACE_RGB ||
+        desc->colorspace == UTIL_FORMAT_COLORSPACE_SRGB)&&
+       desc->nr_channels == 4&&
+       desc->swizzle[0]<= UTIL_FORMAT_SWIZZLE_W&&
+       desc->swizzle[1]<= UTIL_FORMAT_SWIZZLE_W&&
+       desc->swizzle[2]<= UTIL_FORMAT_SWIZZLE_W&&
+       desc->swizzle[3] == UTIL_FORMAT_SWIZZLE_1) {
+      return TRUE;
+   }
+   return FALSE;
+}

The above logic will produce the following unexpected results:
- FALSE for PIPE_FORMAT_R8G8B8_UNORM, because nr_channels == 3.

I'll fix that case.

- FALSE for PIPE_FORMAT_R8G8_UNORM, because nr_channels == 2.
- FALSE for PIPE_FORMAT_R8_UNORM, because nr_channels == 1.

I don't want to return TRUE for R or RG formats.

- FALSE for PIPE_FORMAT_DXT1_RGB, becuase nr_channels == 1 (nr_channels is 
always 1 for non PLAIN formats and not really very meaningful in such format 
types).

Hmm, ok, I thought nr_channels indicate the logical number of color channels in the format, even for compressed formats. Could I count the number of unique swizzles that are <= SWIZZLE_W to determine the number of logical channels?


- TRUE for signed and mixed signed formats such as 
PIPE_FORMAT_R8SG8SB8UX8U_NORM (not sure if this is intended or matters)

I think that's OK. The intention of the function is to return true if the format logically stores 3 color components, but not alpha.


It might be useful to extend/adapt a test like such as 
src/gallium/tests/unit/u_format_compatible_test.c to ensure that this gives the 
expected results for all formats.

+boolean
+util_format_is_luminance(enum pipe_format format)
+{
+   const struct util_format_description *desc =
+      util_format_description(format);
+
+   if ((desc->colorspace == UTIL_FORMAT_COLORSPACE_RGB ||
+        desc->colorspace == UTIL_FORMAT_COLORSPACE_SRGB)&&
+       desc->nr_channels == 1&&
+       desc->swizzle[0] == UTIL_FORMAT_SWIZZLE_X&&
+       desc->swizzle[1] == UTIL_FORMAT_SWIZZLE_X&&
+       desc->swizzle[2] == UTIL_FORMAT_SWIZZLE_X&&
+       desc->swizzle[3] == UTIL_FORMAT_SWIZZLE_1) {
+      return TRUE;
+   }
+   return FALSE;
+}

This will produce TRUE for PIPE_FORMAT_LATC1_UNORM...

+
+boolean
+util_format_is_luminance_alpha(enum pipe_format format)
+{
+   const struct util_format_description *desc =
+      util_format_description(format);
+
+   if ((desc->colorspace == UTIL_FORMAT_COLORSPACE_RGB ||
+        desc->colorspace == UTIL_FORMAT_COLORSPACE_SRGB)&&
+       desc->nr_channels == 2&&
+       desc->swizzle[0] == UTIL_FORMAT_SWIZZLE_X&&
+       desc->swizzle[1] == UTIL_FORMAT_SWIZZLE_X&&
+       desc->swizzle[2] == UTIL_FORMAT_SWIZZLE_X&&
+       desc->swizzle[3] == UTIL_FORMAT_SWIZZLE_Y) {
+      return TRUE;
+   }
+   return FALSE;
+}

.. but this will produce FALSE for PIPE_FORMAT_LATC2_UNORM (nr_channels != 2).

Hmmm, ok.  Maybe ignore nr_channels and just check the swizzles as-is?



+
+boolean
+util_format_is_intensity(enum pipe_format format)
+{
+   const struct util_format_description *desc =
+      util_format_description(format);
+
+   if ((desc->colorspace == UTIL_FORMAT_COLORSPACE_RGB ||
+        desc->colorspace == UTIL_FORMAT_COLORSPACE_SRGB)&&
+       desc->nr_channels == 1&&
+       desc->swizzle[0] == UTIL_FORMAT_SWIZZLE_X&&
+       desc->swizzle[1] == UTIL_FORMAT_SWIZZLE_X&&
+       desc->swizzle[2] == UTIL_FORMAT_SWIZZLE_X&&
+       desc->swizzle[3] == UTIL_FORMAT_SWIZZLE_X) {
+      return TRUE;
+   }
+   return FALSE;
+}

This looks alright.

Jose


-Brian
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to