[Mesa-dev] [PATCH 2/2] mesa: fix unpack_ARGB1555_REV()

[Brian Paul]

On 12/01/2011 07:35 AM, Michel Dänzer wrote:
> On Don, 2011-12-01 at 07:28 -0700, Brian Paul wrote:
>> On 12/01/2011 02:42 AM, Michel Dänzer wrote:
>>> On Mit, 2011-11-30 at 20:36 -0700, Brian Paul wrote:
>>>
>>>> diff --git a/src/mesa/main/format_unpack.c b/src/mesa/main/format_unpack.c
>>>> index 4b4ee6b..fc0db34 100644
>>>> --- a/src/mesa/main/format_unpack.c
>>>> +++ b/src/mesa/main/format_unpack.c
>>>> @@ -275,10 +275,11 @@ unpack_ARGB1555_REV(const void *src, GLfloat dst[][4], GLuint n)
>>>>       const GLushort *s = ((const GLushort *) src);
>>>>       GLuint i;
>>>>       for (i = 0; i<   n; i++) {
>>>> -      dst[i][RCOMP] = UBYTE_TO_FLOAT( ((s[i]>>    7)&   0xf8) | ((s[i]>>   12)&   0x7) );
>>>> -      dst[i][GCOMP] = UBYTE_TO_FLOAT( ((s[i]>>    2)&   0xf8) | ((s[i]>>    7)&   0x7) );
>>>> -      dst[i][BCOMP] = UBYTE_TO_FLOAT( ((s[i]<<    3)&   0xf8) | ((s[i]>>    2)&   0x7) );
>>>> -      dst[i][ACOMP] = UBYTE_TO_FLOAT( ((s[i]>>   15)&   0x01) * 255 );
>>>> +      GLushort tmp = (s[i]<<   8) | (s[i]>>   8); /* byteswap */
>>>
>>> A BSWAP16() macro might be nice for this, but I wouldn't let that hold
>>> up this fix.
>>>
>>>> +      dst[i][RCOMP] = UBYTE_TO_FLOAT( ((tmp>>    7)&   0xf8) | ((tmp>>   12)&   0x7) );
>>>> +      dst[i][GCOMP] = UBYTE_TO_FLOAT( ((tmp>>    2)&   0xf8) | ((tmp>>    7)&   0x7) );
>>>> +      dst[i][BCOMP] = UBYTE_TO_FLOAT( ((tmp<<    3)&   0xf8) | ((tmp>>    2)&   0x7) );
>>>
>>> Don't these lines need to be changed to be the same as in
>>> unpack_ARGB1555()?
>>
>> It should be equivalent (lookup table vs. arithmetic).  It would nice
>> if someone could write a little benchmark to see which is really faster.
>
> I still don't understand why these lines should be different between
> unpack_ARGB1555_REV() and unpack_ARGB1555(), as the only difference
> between the two formats is the swapped bytes, right? If both variants
> are equivalent, I don't particularly care which one is considered
> better, but I think the other one should be adapted to match.

It's just copy and paste from the s_texfetch_tmp.h file (I'd have to 
go through git history to see what the story is there).  I'll make 
them the same and post a new patch.

-Brian