<div dir="ltr">On 7 December 2013 07:42, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>> writes:<br>
<br>
> On 24 November 2013 21:00, Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>> wrote:<br>
><br>
>> Including pack/unpack and texstore code. This texture format is a<br>
>> requirement for ARB_shader_image_load_store.<br>
>> ---<br>
>> src/mesa/main/format_pack.c | 29 +++++++++++++++++++++++++++<br>
>> src/mesa/main/format_unpack.c | 32 ++++++++++++++++++++++++++++++<br>
>> src/mesa/main/formats.c | 19 ++++++++++++++++++<br>
>> src/mesa/main/formats.h | 2 ++<br>
>> src/mesa/main/texstore.c | 46<br>
>> +++++++++++++++++++++++++++++++++++++++++++<br>
>> src/mesa/swrast/s_texfetch.c | 6 ++++++<br>
>> 6 files changed, 134 insertions(+)<br>
>><br>
>> diff --git a/src/mesa/main/format_pack.c b/src/mesa/main/format_pack.c<br>
>> index 826fc10..9b6929d 100644<br>
>> --- a/src/mesa/main/format_pack.c<br>
>> +++ b/src/mesa/main/format_pack.c<br>
>> @@ -1824,6 +1824,31 @@ pack_float_XBGR32323232_FLOAT(const GLfloat src[4],<br>
>> void *dst)<br>
>> d[3] = 1.0;<br>
>> }<br>
>><br>
>> +/* MESA_FORMAT_ABGR2101010 */<br>
>> +<br>
>> +static void<br>
>> +pack_ubyte_ABGR2101010(const GLubyte src[4], void *dst)<br>
>> +{<br>
>> + GLuint *d = ((GLuint *) dst);<br>
>> + GLushort r = UBYTE_TO_USHORT(src[RCOMP]);<br>
>> + GLushort g = UBYTE_TO_USHORT(src[GCOMP]);<br>
>> + GLushort b = UBYTE_TO_USHORT(src[BCOMP]);<br>
>> + GLushort a = UBYTE_TO_USHORT(src[ACOMP]);<br>
>> + *d = PACK_COLOR_2101010_US(a, b, g, r);<br>
>><br>
><br>
> I don't know if we care, but this conversion is not as accurate as it could<br>
> be. For example, if the input has an r value of 0x3f, then a perfect<br>
> conversion would convert this to a float by dividing by 255.0 (to get<br>
> 0.24706), then convert to 10 bits by multiplying by 1023.0 (to get 252.74),<br>
> and then round to the nearest integer, which is 253, or 0xfd.<br>
><br>
> However, what the function above does is convert to 16 bits (0x3f3f), then<br>
> chop off the lower 6 bits by downshifting, to get 0xfc, or 252.<br>
><br>
<br>
</div></div>I don't think this has to do with using ushorts rather than floats, both<br>
have more than enough precision to calculate the result exactly. I<br>
believe that this is a general problem that affects many of the users of<br>
the PACK_COLOR_* macros because of their use of truncation rather than<br>
rounding. If we care, we should probably fix it there.<br></blockquote><div><br></div><div>Yeah, that's a good point. And obviously there's no need to address that in this patch series :)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
>[...]<br>
<div>>> @@ -3362,6 +3376,11 @@ _mesa_format_matches_format_and_type(gl_format<br>
>> gl_format,<br>
>> case MESA_FORMAT_XBGR32323232_UINT:<br>
>> case MESA_FORMAT_XBGR32323232_SINT:<br>
>> return GL_FALSE;<br>
>> +<br>
>> + case MESA_FORMAT_ABGR2101010:<br>
>> + return format == GL_RGBA && type == GL_UNSIGNED_INT_2_10_10_10_REV<br>
>> &&<br>
>> + !swapBytes;<br>
>> +<br>
>><br>
><br>
> I don't understand the byte ordering conventions in this code well enough<br>
> to confirm that this is correct.<br>
<br>
</div>According to the GL spec, the UNSIGNED_INT_2_10_10_10_REV type has the<br>
first component (R for the GL_RGBA format) starting from bit 0, the<br>
second component (G) from bit 10, the third component (B) from bit 20,<br>
and the third component (A) from bit 30 of a 32-bit unsigned int, which<br>
matches the Mesa format exactly.<br></blockquote><div><br></div><div>Ok, I learned a bit more about the conventions used in the MESA_FORMAT_* enum (inconsistent though they are) and double checked your logic; it looks good to me. Consider the patch:<br>
<br>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
> Do you have a unit test that validates that the format is correctly<br>
> interpreted?<br>
<br>
</div>Nope, sorry.<br>
<br>
><br>
>[...]<br>
</blockquote></div><br></div></div>