[Mesa-dev] [PATCH 06/11] mesa: Add MESA_FORMAT_ABGR2101010.

[Mark Mueller]

On Mon, Dec 9, 2013 at 3:09 PM, Paul Berry <stereotype441 at gmail.com> wrote:

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


>
>> >
>> >[...]
>>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131212/71eafe14/attachment.html>