[Mesa-dev] [PATCH 1/2] mesa: fix SwapBytes handling in numerous places
Brian Paul
brianp at vmware.com
Tue Sep 1 07:32:57 PDT 2015
Yeah, this looks better. Just minor nit-picks below.
Otherwise, Reviewed-by: Brian Paul <brianp at vmware.com>
On 08/31/2015 10:50 PM, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> In a number of places the SwapBytes handling
> didn't handle cases with GL_(UN)PACK_ALIGNMENT
> set and 7 byte width cases aligned to 8 bytes.
>
> This adds a common routine to swap bytes a 2D
> image and uses this code in the:
Could line-wrap closer to 75 chars.
> texture store
> texture getting
> readpixels
> and swrast drawpixels.
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
> src/mesa/main/image.c | 43 ++++++++++++++++++++++++++++++++++++++++---
> src/mesa/main/image.h | 20 +++++++-------------
> src/mesa/main/readpix.c | 11 ++---------
> src/mesa/main/texgetimage.c | 14 +++-----------
> src/mesa/main/texstore.c | 28 +++++++++++++++++-----------
> src/mesa/swrast/s_drawpix.c | 14 +++++++-------
> 6 files changed, 76 insertions(+), 54 deletions(-)
>
> diff --git a/src/mesa/main/image.c b/src/mesa/main/image.c
> index 711a190..770d89c 100644
> --- a/src/mesa/main/image.c
> +++ b/src/mesa/main/image.c
> @@ -49,7 +49,7 @@
> * \param src the array with the source data we want to byte-swap.
> * \param n number of words.
> */
> -void
> +static void
> _mesa_swap2_copy( GLushort *dst, GLushort *src, GLuint n )
Could remove _mesa_ prefix from static fn.
> {
> GLuint i;
> @@ -58,7 +58,11 @@ _mesa_swap2_copy( GLushort *dst, GLushort *src, GLuint n )
> }
> }
>
> -
> +void
> +_mesa_swap2(GLushort *p, GLuint n)
> +{
> + _mesa_swap2_copy(p, p, n);
> +}
>
> /*
> * Flip the order of the 4 bytes in each word in the given array (src) and
> @@ -69,7 +73,7 @@ _mesa_swap2_copy( GLushort *dst, GLushort *src, GLuint n )
> * \param src the array with the source data we want to byte-swap.
> * \param n number of words.
> */
> -void
> +static void
> _mesa_swap4_copy( GLuint *dst, GLuint *src, GLuint n )
> {
> GLuint i, a, b;
> @@ -83,6 +87,11 @@ _mesa_swap4_copy( GLuint *dst, GLuint *src, GLuint n )
> }
> }
>
> +void
> +_mesa_swap4(GLuint *p, GLuint n)
> +{
> + _mesa_swap4_copy(p, p, n);
> +}
>
> /**
> * Return the byte offset of a specific pixel in an image (1D, 2D or 3D).
> @@ -958,3 +967,31 @@ _mesa_clip_blit(struct gl_context *ctx,
>
> return GL_TRUE;
> }
> +
> +
> +void
> +_mesa_swap_bytes_2d_image(GLenum format, GLenum type,
How about putting a comment on the function. I presume the 'packing'
info applies to both the src and dest images?
> + const struct gl_pixelstore_attrib *packing,
> + GLsizei width, GLsizei height,
> + GLvoid *dst, const GLvoid *src)
> +{
> + GLint swapSize = _mesa_sizeof_packed_type(type);
Can we assert(packing->SwapBytes)?
> + if (swapSize == 2 || swapSize == 4) {
> + int swapsPerPixel = _mesa_bytes_per_pixel(format, type) / swapSize;
> + int stride = _mesa_image_row_stride(packing, width, format, type);
> + int row;
> + const uint8_t *dstrow;
Remove const qualifier.
> + const uint8_t *srcrow;
> + assert(_mesa_bytes_per_pixel(format, type) % swapSize == 0);
Could also assert(swapsPerPixel >= 1)??
> + dstrow = dst;
> + srcrow = src ? src : dst;
That seems a little funny. Could the caller just pass the same address
for both dst and src? And maybe mention in the function comments that
it's legal for dst==src to do in-place byte swapping.
> + for (row = 0; row < height; row++) {
> + if (swapSize == 2)
> + _mesa_swap2_copy((GLushort *) dstrow, (GLushort *)srcrow, width * swapsPerPixel);
> + else if (swapSize == 4)
> + _mesa_swap4_copy((GLuint *) dstrow, (GLuint *)srcrow, width * swapsPerPixel);
> + dstrow += stride;
> + srcrow += stride;
> + }
> + }
> +}
> diff --git a/src/mesa/main/image.h b/src/mesa/main/image.h
> index 501586b..b5075be 100644
> --- a/src/mesa/main/image.h
> +++ b/src/mesa/main/image.h
> @@ -35,22 +35,11 @@ struct gl_pixelstore_attrib;
> struct gl_framebuffer;
>
> extern void
> -_mesa_swap2_copy(GLushort *dst, GLushort *src, GLuint n);
> +_mesa_swap2(GLushort *p, GLuint n);
>
> extern void
> -_mesa_swap4_copy(GLuint *dst, GLuint *src, GLuint n);
> +_mesa_swap4(GLuint *p, GLuint n);
>
> -static inline void
> -_mesa_swap2(GLushort *p, GLuint n)
> -{
> - _mesa_swap2_copy(p, p, n);
> -}
> -
> -static inline void
> -_mesa_swap4(GLuint *p, GLuint n)
> -{
> - _mesa_swap4_copy(p, p, n);
> -}
>
> extern GLintptr
> _mesa_image_offset( GLuint dimensions,
> @@ -146,5 +135,10 @@ _mesa_clip_blit(struct gl_context *ctx,
> GLint *srcX0, GLint *srcY0, GLint *srcX1, GLint *srcY1,
> GLint *dstX0, GLint *dstY0, GLint *dstX1, GLint *dstY1);
>
> +void
> +_mesa_swap_bytes_2d_image(GLenum format, GLenum type,
> + const struct gl_pixelstore_attrib *packing,
> + GLsizei width, GLsizei height,
> + GLvoid *dst, const GLvoid *src);
>
> #endif
> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
> index 1277944..0ef07b5 100644
> --- a/src/mesa/main/readpix.c
> +++ b/src/mesa/main/readpix.c
> @@ -613,15 +613,8 @@ read_rgba_pixels( struct gl_context *ctx,
> done_swap:
> /* Handle byte swapping if required */
> if (packing->SwapBytes) {
> - GLint swapSize = _mesa_sizeof_packed_type(type);
> - 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);
> - }
> + _mesa_swap_bytes_2d_image(format, type, packing,
> + width, height, dst, NULL);
> }
>
> done_unmap:
> diff --git a/src/mesa/main/texgetimage.c b/src/mesa/main/texgetimage.c
> index f62553d..0c23687 100644
> --- a/src/mesa/main/texgetimage.c
> +++ b/src/mesa/main/texgetimage.c
> @@ -557,17 +557,9 @@ get_tex_rgba_uncompressed(struct gl_context *ctx, GLuint dimensions,
>
> do_swap:
> /* Handle byte swapping if required */
> - if (ctx->Pack.SwapBytes) {
> - GLint swapSize = _mesa_sizeof_packed_type(type);
> - 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);
> - }
> - }
> + if (ctx->Pack.SwapBytes)
> + _mesa_swap_bytes_2d_image(format, type, &ctx->Pack,
> + width, height, dest, NULL);
>
> /* Unmap the src texture buffer */
> ctx->Driver.UnmapTextureImage(ctx, texImage, zoffset + img);
> diff --git a/src/mesa/main/texstore.c b/src/mesa/main/texstore.c
> index 5394026..e50964e 100644
> --- a/src/mesa/main/texstore.c
> +++ b/src/mesa/main/texstore.c
> @@ -727,19 +727,25 @@ texstore_rgba(TEXSTORE_PARAMS)
> */
> GLint swapSize = _mesa_sizeof_packed_type(srcType);
> if (swapSize == 2 || swapSize == 4) {
> - int bytesPerPixel = _mesa_bytes_per_pixel(srcFormat, srcType);
> - int swapsPerPixel = bytesPerPixel / swapSize;
> - int elementCount = srcWidth * srcHeight * srcDepth;
> - assert(bytesPerPixel % swapSize == 0);
> - tempImage = malloc(elementCount * bytesPerPixel);
> + int imageStride = _mesa_image_image_stride(srcPacking, srcWidth, srcHeight, srcFormat, srcType);
> + int bufferSize = imageStride * srcDepth;
> + int layer;
> + const uint8_t *src;
> + uint8_t *dst;
> +
> + tempImage = malloc(bufferSize);
> if (!tempImage)
> return GL_FALSE;
> - if (swapSize == 2)
> - _mesa_swap2_copy(tempImage, (GLushort *) srcAddr,
> - elementCount * swapsPerPixel);
> - else
> - _mesa_swap4_copy(tempImage, (GLuint *) srcAddr,
> - elementCount * swapsPerPixel);
> + src = srcAddr;
> + dst = tempImage;
> + for (layer = 0; layer < srcDepth; layer++) {
> + _mesa_swap_bytes_2d_image(srcFormat, srcType,
> + srcPacking,
> + srcWidth, srcHeight,
> + dst, src);
> + src += imageStride;
> + dst += imageStride;
> + }
> srcAddr = tempImage;
> }
> }
> diff --git a/src/mesa/swrast/s_drawpix.c b/src/mesa/swrast/s_drawpix.c
> index 5393d50..f05528d 100644
> --- a/src/mesa/swrast/s_drawpix.c
> +++ b/src/mesa/swrast/s_drawpix.c
> @@ -481,17 +481,17 @@ draw_rgba_pixels( struct gl_context *ctx, GLint x, GLint y,
> */
> GLint swapSize = _mesa_sizeof_packed_type(type);
> if (swapSize == 2 || swapSize == 4) {
> - int components = _mesa_components_in_format(format);
> - int elementCount = width * height * components;
> - tempImage = malloc(elementCount * swapSize);
> + int imageStride = _mesa_image_image_stride(unpack, width, height, format, type);
> +
> + tempImage = malloc(imageStride);
> if (!tempImage) {
> _mesa_error(ctx, GL_OUT_OF_MEMORY, "glDrawPixels");
> return;
> }
> - if (swapSize == 2)
> - _mesa_swap2_copy(tempImage, (GLushort *) pixels, elementCount);
> - else
> - _mesa_swap4_copy(tempImage, (GLuint *) pixels, elementCount);
> +
> + _mesa_swap_bytes_2d_image(format, type, unpack,
> + width, height, tempImage, pixels);
> +
> pixels = tempImage;
> }
> }
>
More information about the mesa-dev
mailing list