[Mesa-dev] [PATCH V3 1/9] mesa: create _mesa_attach_renderbuffer_without_ref() helper

[Brian Paul]

On 04/10/2017 06:09 PM, Timothy Arceri wrote:
> On 11/04/17 03:11, Brian Paul wrote:
>> On 04/07/2017 09:21 PM, Timothy Arceri wrote:
>>> This will be used to take ownership of freashly created renderbuffers,
>>> avoiding the need to call the reference function which requires
>>> locking.
>>>
>>> V2: dereference any existing fb attachments and actually attach the
>>>      new rb.
>>>
>>> v3: split out validation and attachment type/complete setting into
>>>      a shared static function.
>>> ---
>>>   src/mesa/main/renderbuffer.c | 43
>>> +++++++++++++++++++++++++++++++++++--------
>>>   src/mesa/main/renderbuffer.h |  5 +++++
>>>   2 files changed, 40 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/mesa/main/renderbuffer.c b/src/mesa/main/renderbuffer.c
>>> index 4375b5b..627bdca 100644
>>> --- a/src/mesa/main/renderbuffer.c
>>> +++ b/src/mesa/main/renderbuffer.c
>>> @@ -99,28 +99,24 @@ _mesa_new_renderbuffer(struct gl_context *ctx,
>>> GLuint name)
>>>    * free the object in the end.
>>>    */
>>>   void
>>>   _mesa_delete_renderbuffer(struct gl_context *ctx, struct
>>> gl_renderbuffer *rb)
>>>   {
>>>      mtx_destroy(&rb->Mutex);
>>>      free(rb->Label);
>>>      free(rb);
>>>   }
>>>
>>> -
>>> -/**
>>> - * Attach a renderbuffer to a framebuffer.
>>> - * \param bufferName  one of the BUFFER_x tokens
>>> - */
>>> -void
>>> -_mesa_add_renderbuffer(struct gl_framebuffer *fb,
>>> -                       gl_buffer_index bufferName, struct
>>> gl_renderbuffer *rb)
>>> +static void
>>> +validate_and_init_renderbuffer_attachment(struct gl_framebuffer *fb,
>>> +                                          gl_buffer_index bufferName,
>>> +                                          struct gl_renderbuffer *rb)
>>>   {
>>>      assert(fb);
>>>      assert(rb);
>>>      assert(bufferName < BUFFER_COUNT);
>>>
>>>      /* There should be no previous renderbuffer on this attachment
>>> point,
>>>       * with the exception of depth/stencil since the same
>>> renderbuffer may
>>>       * be used for both.
>>>       */
>>>      assert(bufferName == BUFFER_DEPTH ||
>>> @@ -130,20 +126,51 @@ _mesa_add_renderbuffer(struct gl_framebuffer *fb,
>>>      /* winsys vs. user-created buffer cross check */
>>>      if (_mesa_is_user_fbo(fb)) {
>>>         assert(rb->Name);
>>>      }
>>>      else {
>>>         assert(!rb->Name);
>>>      }
>>>
>>>      fb->Attachment[bufferName].Type = GL_RENDERBUFFER_EXT;
>>>      fb->Attachment[bufferName].Complete = GL_TRUE;
>>> +}
>>> +
>>> +
>>> +/**
>>> + * Attach a renderbuffer to a framebuffer.
>>> + * \param bufferName  one of the BUFFER_x tokens
>>> + *
>>> + * This function avoids adding a reference and is therefore intended
>>> to be
>>> + * used with a freashly created renderbuffer.
>>
>> "freshly"
>>
>>
>>> + */
>>> +void
>>> +_mesa_add_renderbuffer_without_ref(struct gl_framebuffer *fb,
>>> +                                   gl_buffer_index bufferName,
>>> +                                   struct gl_renderbuffer *rb)
>>
>> I see you've already pushed this.
>
> Yes the previous leak was causing issue in Gnome so I wanted to push
> these in case they were causing problems also.
>
>> Still, I'd like to suggest a
>> different name such as _mesa_own_renderbuffer() that stresses the
>> transfer of ownership of the renderbuffer.
>
> I pushed the other two trivial suggestions, as for the name I struggled
> a bit with this because we still want it to be obvious that it is a
> variant of _mesa_add_renderbuffer().
>
> How about _mesa_add_and_own_renderbuffer() ??
>
>
> IMO these would make more sense being called _mesa_attach_* rather than
> add. I can change that also is you agree?
>
> Maybe:
>
> _mesa_attach_and_own_rb()
> _mesa_attach_and_reference_rb()
>
> ??

That sounds OK.

-Brian