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

[Kenneth Graunke]

On 04/15/2013 05:53 PM, Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at 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.romanick at 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.

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 <kenneth at 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");
>   }
>
>
>