[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.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.

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