Re: [Mesa-dev] [PATCH 3/3] mesa/swrast: Move memory allocation outside the blit loop

[Ian Romanick]

On 04/15/2013 11:05 PM, Kenneth Graunke wrote:
On 04/15/2013 05:53 PM, Ian Romanick wrote:
From: Ian Romanick <ian.d.roman...@intel.com>
Assume the maximum pixel size (16 bytes per pixel).  In addition to
moving redundant malloc and free calls outside the loop, this fixes a
potential resource leak when a surface is mapped and the malloc fails.
This also makes blit_nearest look a bit more like blit_linear.

Signed-off-by: Ian Romanick <ian.d.roman...@intel.com>
---
  src/mesa/swrast/s_blit.c | 45
+++++++++++++++++++++------------------------
  1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/src/mesa/swrast/s_blit.c b/src/mesa/swrast/s_blit.c
index ecec734..3824162 100644
--- a/src/mesa/swrast/s_blit.c
+++ b/src/mesa/swrast/s_blit.c
@@ -136,7 +136,7 @@ blit_nearest(struct gl_context *ctx,
        UNPACK_Z_INT,
        UNPACK_S,
     } mode = DIRECT;
-   GLubyte *srcMap, *dstMap;
+   GLubyte *srcMap = NULL, *dstMap = NULL;
     GLint srcRowStride, dstRowStride;
     GLint dstRow;

@@ -188,6 +188,12 @@ blit_nearest(struct gl_context *ctx,
        return;
     }

+   /* allocate the src/dst row buffers */
+   srcBuffer = malloc(16 * srcWidth);
+   dstBuffer = malloc(16 * dstWidth);

Perhaps use MAX_PIXEL_BYTES (from formats.h) here?  That might clarify
that you're using the maximum value (16 looks like a magic number).
Also, the maximum is actually whatever can be returned from
_mesa_get_format_bytes(), which has an assertion relating to that #define.

Good call!  I didn't realize there was such a define.  I thought about 
adding one, but laziness won...

If not, please at least add a comment above these mallocs saying that 16
is the maximum size.

With one of those changes, this series is:
Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>

+   if (!srcBuffer || !dstBuffer)
+      goto fail_no_memory;
+
     /* Blit to all the draw buffers */
     for (i = 0; i < numDrawBuffers; i++) {
        if (buffer == GL_COLOR_BUFFER_BIT) {
@@ -229,7 +235,7 @@ blit_nearest(struct gl_context *ctx,
        default:
           _mesa_problem(ctx, "unexpected pixel size (%d) in
blit_nearest",
                         pixelSize);
-         return;
+         goto fail;
        }

        if ((readRb == drawRb) ||
@@ -248,8 +254,7 @@ blit_nearest(struct gl_context *ctx,
                                       GL_MAP_READ_BIT |
GL_MAP_WRITE_BIT,
                                       &map, &rowStride);
           if (!map) {
-            _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBlitFramebuffer");
-            return;
+            goto fail_no_memory;
           }

           srcMap = map + srcYpos * rowStride + srcXpos * formatSize;
@@ -276,8 +281,7 @@ blit_nearest(struct gl_context *ctx,
                                       srcWidth, srcHeight,
                                       GL_MAP_READ_BIT, &srcMap,
&srcRowStride);
           if (!srcMap) {
-            _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBlitFramebuffer");
-            return;
+            goto fail_no_memory;
           }
           ctx->Driver.MapRenderbuffer(ctx, drawRb,
                                       dstXpos, dstYpos,
@@ -285,24 +289,10 @@ blit_nearest(struct gl_context *ctx,
                                       GL_MAP_WRITE_BIT, &dstMap,
&dstRowStride);
           if (!dstMap) {
              ctx->Driver.UnmapRenderbuffer(ctx, readRb);
-            _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBlitFramebuffer");
-            return;
+            goto fail_no_memory;
           }
        }

-      /* allocate the src/dst row buffers */
-      srcBuffer = malloc(pixelSize * srcWidth);
-      if (!srcBuffer) {
-         _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBlitFrameBufferEXT");
-         return;
-      }
-      dstBuffer = malloc(pixelSize * dstWidth);
-      if (!dstBuffer) {
-         free(srcBuffer);
-         _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBlitFrameBufferEXT");
-         return;
-      }
-
        for (dstRow = 0; dstRow < dstHeight; dstRow++) {
           GLfloat srcRowF = (dstRow + 0.5F) / dstHeight * srcHeight -
0.5F;
           GLint srcRow = IROUND(srcRowF);
@@ -369,14 +359,21 @@ blit_nearest(struct gl_context *ctx,
           }
        }

-      free(srcBuffer);
-      free(dstBuffer);
-
        ctx->Driver.UnmapRenderbuffer(ctx, readRb);
        if (drawRb != readRb) {
           ctx->Driver.UnmapRenderbuffer(ctx, drawRb);
        }
     }
+
+fail:
+   free(srcBuffer);
+   free(dstBuffer);
+   return;
+
+fail_no_memory:
+   free(srcBuffer);
+   free(dstBuffer);
+   _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBlitFrameBuffer");
  }

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev