[Mesa-dev] [PATCH 1/2] util: add util_format_is_luminance/intensity/rgb(), etc

[Brian Paul]

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<brianp at 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