On 7 December 2013 07:42, Francisco Jerez <curroje...@riseup.net> wrote:
> Paul Berry <stereotype...@gmail.com> writes:
>
> > On 24 November 2013 21:00, Francisco Jerez <curroje...@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 <stereotype...@gmail.com>
>
> > Do you have a unit test that validates that the format is correctly
> > interpreted?
>
> Nope, sorry.
>
> >
> >[...]
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
