[Mesa-dev] [PATCH] egl/wayland: do not leak wl_buffer when it is locked

[Juan A. Suarez Romero]

On Wed, 2018-08-08 at 17:49 +0100, Daniel Stone wrote:
> Hi Juan,
> 
> On Wed, 8 Aug 2018 at 17:40, Juan A. Suarez Romero <jasuarez at igalia.com> wrote:
> > On Wed, 2018-08-08 at 17:21 +0100, Daniel Stone wrote:
> > > On Thu, 2 Aug 2018 at 10:02, Juan A. Suarez Romero <jasuarez at igalia.com> wrote:
> > > > If color buffer is locked, do not set its wayland buffer to NULL;
> > > > otherwise it can not be freed later.
> > > 
> > > It can: see the 'if (i == ARRAY_SIZE(...))' branch inside wl_buffer_release.
> > 
> > I think I didn't explain wrongly :)
> > 
> > If color buffer is locked, we set color_buffer.wl_buffer to NULL, and thus we
> > can't free wl_buffer later.
> 
> If a surface is resized, we will orphan all its wl_buffers by clearing
> their pointers to NULL, hence losing the ability to directly free
> them. But when a new buffer has been attached and displayed, a
> 'release' event will be delivered for the old buffer (handled by
> wl_buffer_release as an event listener), which will detect that the
> wl_buffer is not in the list and should be immediately destroyed.
> 
> In fact, I have to rescind my R-b since I'm pretty sure this breaks resizing:
>   - buffer 1 is allocated with original size, committed to server
> (locked == true)
>   - user resizes surface, release_buffers() is called but leaves
> wl_buffer intact in color_buffers list
>   - buffers 2..4 are allocated with new size and committed to server
> (locked == true)
>   - release event for buffer 1 is delivered, locked = false
>   - get_back_bo() finds buffer 1 has a wl_buffer and is not locked,
> but dri_image is NULL so a new image is created

I understand this situation happens because the release event just set
locked=false, but does not free the wl_buffer.

What about this? Within my proposed fix, add a flag to tell the release event if
the wl_buffer should be released or not. By default this is false, so the
release event only unlocks the buffer. In release_buffers() if the buffer is
locked, then we set the flag to true, so when the release event is invoked we
free the buffer and set to NULL, so get_back_bo() won't reuse the old buffer.


	J.A.



>   - swap_buffers_with_damage() finds buffer 1 (with new DRIimage)
> still has wl_buffer (old) and attaches that buffer
> 
> So we need to either find a way to still destroy the wl_buffer inside
> wl_buffer_release(), or at least do it later, e.g. in get_back_bo().
> 
> Cheers,
> Daniel
>