On Thursday, January 08, 2015 01:31:56 PM Jason Ekstrand wrote:
> On Thu, Jan 8, 2015 at 12:02 PM, Anuj Phogat <anuj.pho...@gmail.com> wrote:
> > On Wed, Jan 7, 2015 at 11:20 PM, Iago Toral Quiroga <ito...@igalia.com>
> >
> > wrote:
> > > From: Jason Ekstrand <jason.ekstr...@intel.com>
> > >
> > > Aparently, the packing/unpacking functions for these formats have
> >
> > differed
> >
> > > from the format description in formats.h. Instead of fixing this,
> > > people
> > > simply left a comment saying it was broken. Let's actually fix it for
> > > real.
> > >
> > > Signed-off-by: Jason Ekstrand <jason.ekstr...@intel.com>
> > >
> > > v2 by Samuel Iglesias <sigles...@igalia.com>:
> > > - Keep comment in unpack_R5G6B5_UNORM()
> > > ---
> > >
> > > src/mesa/main/format_pack.c | 10 ++--------
> > > src/mesa/main/format_unpack.c | 7 ++-----
> > > src/mesa/main/formats.c | 12 ++++++------
> > > src/mesa/main/texstore.c | 2 +-
> > > src/mesa/swrast/s_texfetch_tmp.h | 8 ++++----
> > > 5 files changed, 15 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/src/mesa/main/format_pack.c b/src/mesa/main/format_pack.c
> > > index 31c9f77..20d2b1a 100644
> > > --- a/src/mesa/main/format_pack.c
> > > +++ b/src/mesa/main/format_pack.c
> > > @@ -458,17 +458,11 @@ pack_row_float_B5G6R5_UNORM(GLuint n, const
> >
> > GLfloat src[][4], void *dst)
> >
> > > }
> > >
> > > -/*
> > > - * MESA_FORMAT_R5G6B5_UNORM
> > > - * Warning: these functions do not match the current Mesa definition
> > > - * of MESA_FORMAT_R5G6B5_UNORM.
> > > - */
> > > -
> > >
> > > static void
> > > pack_ubyte_R5G6B5_UNORM(const GLubyte src[4], void *dst)
> > > {
> > >
> > > GLushort *d = ((GLushort *) dst);
> > >
> > > - *d = PACK_COLOR_565_REV(src[RCOMP], src[GCOMP], src[BCOMP]);
> > > + *d = PACK_COLOR_565(src[BCOMP], src[GCOMP], src[RCOMP]);
> > >
> > > }
> > >
> > > static void
> > >
> > > @@ -479,7 +473,7 @@ pack_float_R5G6B5_UNORM(const GLfloat src[4], void
> >
> > *dst)
> >
> > > UNCLAMPED_FLOAT_TO_UBYTE(r, src[RCOMP]);
> > > UNCLAMPED_FLOAT_TO_UBYTE(g, src[GCOMP]);
> > > UNCLAMPED_FLOAT_TO_UBYTE(b, src[BCOMP]);
> > >
> > > - *d = PACK_COLOR_565_REV(r, g, b);
> > > + *d = PACK_COLOR_565(b, g, r);
> > >
> > > }
> > >
> > > static void
> > >
> > > diff --git a/src/mesa/main/format_unpack.c
> >
> > b/src/mesa/main/format_unpack.c
> >
> > > index d5628a9..69c76d3 100644
> > > --- a/src/mesa/main/format_unpack.c
> > > +++ b/src/mesa/main/format_unpack.c
> > > @@ -2764,16 +2764,13 @@ unpack_ubyte_B5G6R5_UNORM(const void *src,
> >
> > GLubyte dst[][4], GLuint n)
> >
> > > static void
> > > unpack_ubyte_R5G6B5_UNORM(const void *src, GLubyte dst[][4], GLuint n)
> > > {
> > >
> > > - /* Warning: this function does not match the current Mesa definition
> > > - * of MESA_FORMAT_R5G6B5_UNORM.
> > > - */
> > >
> > > const GLushort *s = ((const GLushort *) src);
> > > GLuint i;
> > > for (i = 0; i < n; i++) {
> > >
> > > GLuint t = (s[i] >> 8) | (s[i] << 8); /* byte swap */
> > >
> > > - dst[i][RCOMP] = EXPAND_5_8((t >> 11) & 0x1f);
> > > + dst[i][RCOMP] = EXPAND_5_8( t & 0x1f);
> > >
> > > dst[i][GCOMP] = EXPAND_6_8((t >> 5 ) & 0x3f);
> > >
> > > - dst[i][BCOMP] = EXPAND_5_8( t & 0x1f);
> > > + dst[i][BCOMP] = EXPAND_5_8((t >> 11) & 0x1f);
> > >
> > > dst[i][ACOMP] = 0xff;
> > >
> > > }
> > >
> > > }
> > >
> > > diff --git a/src/mesa/main/formats.c b/src/mesa/main/formats.c
> > > index 58c32e2..1315d36 100644
> > > --- a/src/mesa/main/formats.c
> > > +++ b/src/mesa/main/formats.c
> > > @@ -1462,14 +1462,14 @@ _mesa_format_matches_format_and_type(mesa_format
> >
> > mesa_format,
> >
> > > return format == GL_RGB && type == GL_UNSIGNED_BYTE &&
> >
> > littleEndian;
> >
> > > case MESA_FORMAT_B5G6R5_UNORM:
> > > - return format == GL_RGB && type == GL_UNSIGNED_SHORT_5_6_5 &&
> >
> > !swapBytes;
> >
> > > + return ((format == GL_RGB && type == GL_UNSIGNED_SHORT_5_6_5) ||
> > > + (format == GL_BGR && type ==
> >
> > GL_UNSIGNED_SHORT_5_6_5_REV)) &&
> >
> > > + !swapBytes;
> > >
> > > case MESA_FORMAT_R5G6B5_UNORM:
> > > - /* Some of the 16-bit MESA_FORMATs that would seem to correspond
> >
> > to
> >
> > > - * GL_UNSIGNED_SHORT_* are byte-swapped instead of
> >
> > channel-reversed,
> >
> > > - * according to formats.h, so they can't be matched.
> > > - */
> > > - return GL_FALSE;
> > > + return ((format == GL_BGR && type == GL_UNSIGNED_SHORT_5_6_5) ||
> > > + (format == GL_RGB && type ==
> >
> > GL_UNSIGNED_SHORT_5_6_5_REV)) &&
> >
> > > + !swapBytes;
> > >
> > > case MESA_FORMAT_B4G4R4A4_UNORM:
> > > return format == GL_BGRA && type == GL_UNSIGNED_SHORT_4_4_4_4_REV
> >
> > &&
> >
> > > diff --git a/src/mesa/main/texstore.c b/src/mesa/main/texstore.c
> > > index 50aa1fd..b9407d2 100644
> > > --- a/src/mesa/main/texstore.c
> > > +++ b/src/mesa/main/texstore.c
> > > @@ -923,7 +923,7 @@ _mesa_texstore_rgb565(TEXSTORE_PARAMS)
> > >
> > > }
> > > else {
> > >
> > > for (col = 0; col < srcWidth; col++) {
> > >
> > > - dstUS[col] = PACK_COLOR_565_REV( srcUB[0], srcUB[1],
> >
> > srcUB[2] );
> >
> > > + dstUS[col] = PACK_COLOR_565( srcUB[2], srcUB[1],
> >
> > srcUB[0] );
> >
> > > srcUB += 3;
> > >
> > > }
> > >
> > > }
> > >
> > > diff --git a/src/mesa/swrast/s_texfetch_tmp.h
> >
> > b/src/mesa/swrast/s_texfetch_tmp.h
> >
> > > index 7ff30f6..23db48d 100644
> > > --- a/src/mesa/swrast/s_texfetch_tmp.h
> > > +++ b/src/mesa/swrast/s_texfetch_tmp.h
> > > @@ -417,10 +417,10 @@ FETCH(R5G6B5_UNORM)(const struct
> >
> > swrast_texture_image *texImage,
> >
> > > GLint i, GLint j, GLint k, GLfloat *texel)
> > >
> > > {
> > >
> > > const GLushort *src = TEXEL_ADDR(GLushort, texImage, i, j, k, 1);
> > >
> > > - const GLushort s = (*src >> 8) | (*src << 8); /* byte swap */
> > > - texel[RCOMP] = UBYTE_TO_FLOAT( ((s >> 8) & 0xf8) | ((s >> 13) & 0x7)
> >
> > );
> >
> > > - texel[GCOMP] = UBYTE_TO_FLOAT( ((s >> 3) & 0xfc) | ((s >> 9) & 0x3)
> >
> > );
> >
> > > - texel[BCOMP] = UBYTE_TO_FLOAT( ((s << 3) & 0xf8) | ((s >> 2) & 0x7)
> >
> > );
> >
> > > + const GLushort s = *src;
> > > + texel[RCOMP] = ((s ) & 0x1f) * (1.0F / 31.0F);
> > > + texel[GCOMP] = ((s >> 5 ) & 0x3f) * (1.0F / 63.0F);
> > > + texel[BCOMP] = ((s >> 11) & 0x1f) * (1.0F / 31.0F);
> > >
> > > texel[ACOMP] = 1.0F;
> > >
> > > }
> > >
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> > Below comment in formats.h also needs a fix to show correct ordering of
> >
> > components:
> > * examples: msb <------ TEXEL BITS -----------> lsb
> > * MESA_FORMAT_A8B8G8R8_UNORM, AAAA AAAA BBBB BBBB GGGG GGGG RRRR RRRR
> > * MESA_FORMAT_R5G6B5_UNORM RRRR RGGG GGGB BBBB
> > * MESA_FORMAT_B4G4R4X4_UNORM BBBB GGGG RRRR XXXX
>
> I believe the comment was always correct, but the code to [un]pack them
> wasn't
>
> > With this comment fixed, this patch is:
> > Reviewed-by: Anuj Phogat <anuj.pho...@gmail.com>
> >
Good catch!
Taking MESA_FORMAT_A8B8G8R8_UNORM as an example in the comment pointed by
Anuj:
* examples: msb <------ TEXEL BITS -----------> lsb
* MESA_FORMAT_A8B8G8R8_UNORM, AAAA AAAA BBBB BBBB GGGG GGGG RRRR RRRR
And this is what we say when explaining the mesa format bit ordering (a few
lines below of previous comment):
/* Packed unorm formats */ /* msb <------ TEXEL BITS -----------> lsb */
/* ---- ---- ---- ---- ---- ---- ---- ---- */
MESA_FORMAT_A8B8G8R8_UNORM, /* RRRR RRRR GGGG GGGG BBBB BBBB AAAA AAAA */
Same for the other formats in the comment pointed by Anuj.
I am going to fix it.
Thanks!
Sam
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
