On Fri, 2015-02-13 at 10:18 -0800, Ian Romanick wrote:
> On 02/13/2015 03:56 AM, Iago Toral Quiroga wrote:
> > Some old format conversion code in pack.c implemented byte-swapping like
> > this:
> >
> > GLint comps = _mesa_components_in_format(dstFormat);
> > GLint swapSize = _mesa_sizeof_packed_type(dstType);
> > if (swapSize == 2)
> > _mesa_swap2((GLushort *) dstAddr, n * comps);
> > else if (swapSize == 4)
> > _mesa_swap4((GLuint *) dstAddr, n * comps);
> >
> > where n is the pixel count. But this is incorrect for packed formats,
> > where _mesa_sizeof_packed_type is already returning the size of a pixel
> > instead of the size of a single component, so multiplying this by the
> > number of components in the format results in a larger element count
> > for _mesa_swap than we want.
> >
> > Unfortunately, we followed the same implementation for byte-swapping
> > in the rewrite of the format conversion code for texstore, readpixels
> > and texgetimage.
> >
> > This patch computes the correct element counts for _mesa_swap calls
> > by computing the bytes per pixel in the image and dividing that by the
> > swap size to obtain the number of swaps required per pixel. Then multiplies
> > that by the number of pixels in the image to obtain the swap count that
> > we need to use.
> >
> > Also, when handling byte-swapping in texstore_rgba, we were ignoring
> > the image's depth. This patch fixes this too.
>
> Are there any paths that need to handle
> GL_FLOAT_32_UNSIGNED_INT_24_8_REV? In those paths _mesa_sizeof_packed_type
I think this got cut... anyway, this patch handles swapping on color
data uploads and downloads, so I think none of these need to handle
GL_FLOAT_32_UNSIGNED_INT_24_8_REV.
Looking at get_tex_depth_stencil() in texgetimage.c it looks like we
always call _mesa_swap4 . For the readpixels path we call
_mesa_pack_depth_stencil_span() which seems to do the same. We don't use
_mesa_sizeof_packed_type in these cases, so I guess this is fine.
Iago
> > ---
> > src/mesa/main/readpix.c | 13 ++++++++-----
> > src/mesa/main/texgetimage.c | 13 ++++++++-----
> > src/mesa/main/texstore.c | 14 +++++++++-----
> > 3 files changed, 25 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
> > index 85f900d..ca4b943 100644
> > --- a/src/mesa/main/readpix.c
> > +++ b/src/mesa/main/readpix.c
> > @@ -605,12 +605,15 @@ read_rgba_pixels( struct gl_context *ctx,
> > done_swap:
> > /* Handle byte swapping if required */
> > if (packing->SwapBytes) {
> > - int components = _mesa_components_in_format(format);
> > GLint swapSize = _mesa_sizeof_packed_type(type);
>
> It's challenging to understand why this code is correct because
> _mesa_sizeof_packed_type is a misleading name. It determines the size
> of packed types (e.g., GL_UNSIGNED_BYTE_3_3_2) and non-packed types
> (e.g., GL_UNSIGNED_BYTE)... and thus the original bug.
>
> You don't need to change anything here. This is just commentary that
> naming matters.
>
> > - if (swapSize == 2)
> > - _mesa_swap2((GLushort *) dst, width * height * components);
> > - else if (swapSize == 4)
> > - _mesa_swap4((GLuint *) dst, width * height * components);
> > + if (swapSize == 2 || swapSize == 4) {
> > + int swapsPerPixel = _mesa_bytes_per_pixel(format, type) /
> > swapSize;
> > + assert(_mesa_bytes_per_pixel(format, type) % swapSize == 0);
> > + if (swapSize == 2)
> > + _mesa_swap2((GLushort *) dst, width * height * swapsPerPixel);
> > + else if (swapSize == 4)
> > + _mesa_swap4((GLuint *) dst, width * height * swapsPerPixel);
> > + }
> > }
> >
> > done_unmap:
> > diff --git a/src/mesa/main/texgetimage.c b/src/mesa/main/texgetimage.c
> > index ee465e6..405f085 100644
> > --- a/src/mesa/main/texgetimage.c
> > +++ b/src/mesa/main/texgetimage.c
> > @@ -511,12 +511,15 @@ get_tex_rgba_uncompressed(struct gl_context *ctx,
> > GLuint dimensions,
> > do_swap:
> > /* Handle byte swapping if required */
> > if (ctx->Pack.SwapBytes) {
> > - int components = _mesa_components_in_format(format);
> > GLint swapSize = _mesa_sizeof_packed_type(type);
> > - if (swapSize == 2)
> > - _mesa_swap2((GLushort *) dest, width * height * components);
> > - else if (swapSize == 4)
> > - _mesa_swap4((GLuint *) dest, width * height * components);
> > + if (swapSize == 2 || swapSize == 4) {
> > + int swapsPerPixel = _mesa_bytes_per_pixel(format, type) /
> > swapSize;
> > + assert(_mesa_bytes_per_pixel(format, type) % swapSize == 0);
> > + if (swapSize == 2)
> > + _mesa_swap2((GLushort *) dest, width * height *
> > swapsPerPixel);
> > + else if (swapSize == 4)
> > + _mesa_swap4((GLuint *) dest, width * height *
> > swapsPerPixel);
> > + }
> > }
> >
> > /* Unmap the src texture buffer */
> > diff --git a/src/mesa/main/texstore.c b/src/mesa/main/texstore.c
> > index 4d32659..227693d 100644
> > --- a/src/mesa/main/texstore.c
> > +++ b/src/mesa/main/texstore.c
> > @@ -721,15 +721,19 @@ texstore_rgba(TEXSTORE_PARAMS)
> > */
> > GLint swapSize = _mesa_sizeof_packed_type(srcType);
> > if (swapSize == 2 || swapSize == 4) {
> > - int components = _mesa_components_in_format(srcFormat);
> > - int elementCount = srcWidth * srcHeight * components;
> > - tempImage = malloc(elementCount * swapSize);
> > + int bytesPerPixel = _mesa_bytes_per_pixel(srcFormat, srcType);
> > + assert(bytesPerPixel % swapSize == 0);
> > + int swapsPerPixel = bytesPerPixel / swapSize;
> > + int elementCount = srcWidth * srcHeight * srcDepth;
> > + tempImage = malloc(elementCount * bytesPerPixel);
> > if (!tempImage)
> > return GL_FALSE;
> > if (swapSize == 2)
> > - _mesa_swap2_copy(tempImage, (GLushort *) srcAddr,
> > elementCount);
> > + _mesa_swap2_copy(tempImage, (GLushort *) srcAddr,
> > + elementCount * swapsPerPixel);
> > else
> > - _mesa_swap4_copy(tempImage, (GLuint *) srcAddr, elementCount);
> > + _mesa_swap4_copy(tempImage, (GLuint *) srcAddr,
> > + elementCount * swapsPerPixel);
> > srcAddr = tempImage;
> > }
> > }
> >
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
