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

[Jordan Justen]

On Thu, Aug 9, 2012 at 7:47 AM, Brian Paul <brianp at vmware.com> wrote:
> 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.

That sounds good. I'll make these changes.

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

These were developed in the context of adding rgb10_a2ui texture
support. I did not think that piglit adequately tested this texture
format, nor integer textures in general. (Or, perhaps I just missed
the tests.) I relied upon Intel's 'oglconform' test tool to increase
the test scope.

So, no, unfortunately I don't believe piglit will hit many of these cases.

-Jordan