[Mesa-dev] [PATCH] mesa: Fix element count for byte-swaps in texstore, readpix and texgetimage

[Iago Toral]

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;
> >        }
> >     }
> > 
> 
>