[Mesa-dev] [PATCH] mesa: fix renderbuffer leak

[Timothy Arceri]

On 08/04/17 02:03, Brian Paul wrote:
> On 04/06/2017 03:55 PM, Timothy Arceri wrote:
>> We don't need to call _mesa_reference_renderbuffer() for the first
>> assignment as refCount starts at 1. For swrast we work around the
>> fact we will indirectly call _mesa_reference_renderbuffer() by
>> resetting refCount to 0.
>>
>> Fixes: 32141e53d1520 (mesa: tidy up renderbuffer RefCount initialisation)
>> ---
>>   src/mesa/main/fbobject.c         | 2 +-
>>   src/mesa/swrast/s_renderbuffer.c | 5 +++++
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
>> index e52c1e3..e46a7ca 100644
>> --- a/src/mesa/main/fbobject.c
>> +++ b/src/mesa/main/fbobject.c
>> @@ -441,21 +441,21 @@ _mesa_update_texture_renderbuffer(struct
>> gl_context *ctx,
>>
>>      texImage = att->Texture->Image[att->CubeMapFace][att->TextureLevel];
>>
>>      rb = att->Renderbuffer;
>>      if (!rb) {
>>         rb = ctx->Driver.NewRenderbuffer(ctx, ~0);
>>         if (!rb) {
>>            _mesa_error(ctx, GL_OUT_OF_MEMORY, "glFramebufferTexture()");
>>            return;
>>         }
>> -      _mesa_reference_renderbuffer(&att->Renderbuffer, rb);
>> +      att->Renderbuffer = rb;
>>
>>         /* This can't get called on a texture renderbuffer, so set it
>> to NULL
>>          * for clarity compared to user renderbuffers.
>>          */
>>         rb->AllocStorage = NULL;
>>
>>         rb->NeedsFinishRenderTexture = ctx->Driver.FinishRenderTexture
>> != NULL;
>>      }
>>
>>      if (!texImage)
>> diff --git a/src/mesa/swrast/s_renderbuffer.c
>> b/src/mesa/swrast/s_renderbuffer.c
>> index d8c4ed3..af09955 100644
>> --- a/src/mesa/swrast/s_renderbuffer.c
>> +++ b/src/mesa/swrast/s_renderbuffer.c
>> @@ -264,20 +264,25 @@ add_color_renderbuffers(struct gl_context *ctx,
>> struct gl_framebuffer *fb,
>>            continue;
>>
>>         assert(fb->Attachment[b].Renderbuffer == NULL);
>>
>>         rb = ctx->Driver.NewRenderbuffer(ctx, 0);
>>         if (!rb) {
>>            _mesa_error(ctx, GL_OUT_OF_MEMORY, "Allocating color buffer");
>>            return GL_FALSE;
>>         }
>>
>> +      /* Set refcount to 0 to avoid a leak since the
>> _mesa_add_renderbuffer()
>> +       * call below will bump the initial refcount.
>> +       */
>> +      rb->RefCount = 0;
>> +
>>         rb->InternalFormat = GL_RGBA;
>>
>>         rb->AllocStorage = soft_renderbuffer_storage;
>>         _mesa_add_renderbuffer(fb, b, rb);
>
> Perhaps another way of doing this which wouldn't look quite so hacky
> would be to call _mesa_renderbuffer_reference(&rb, NULL) here, after the
> _mesa_add_renderbuffer() call.  The idea is we're transferring ownership
> of the rb and the rb var is going out of scope.

I'd already pushed before seeing this. However it seems there were 
additional leaks from internal renderbuffer creation that I hadn't 
noticed, so I've sent a new series that fixes that (including updating 
the changes from this patch) and in a less hacky looking way.

https://patchwork.freedesktop.org/series/22708/

In the series I avoid calling _mesa_renderbuffer_reference() when it's 
not required which should be better as it creates an unnecessary lock.

Thanks,
Tim

>
> Something similar could be done above.  A comment indicating that we're
> transferring ownership could be helpful too.
>
> -Brian
>
>>      }
>>
>>      return GL_TRUE;
>>   }
>>
>>
>>
>