[Wayland-bugs] [Bug 761312] memory leak

[gtk+ (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=761312

--- Comment #22 from Jonas Ådahl <jadahl at gmail.com> ---
Review of attachment 320235:

::: gdk/wayland/gdkwindow-wayland.c
@@ +124,3 @@

+  cairo_surface_t *staging_surface;
+  cairo_surface_t *commited_surface;

s/commited/committed/.

The naming is a bit unfortunate. We now have "surface" which is a wl_surface,
and staging_surface, and commited_surface which are cairo_surface_t. Maybe name
these *_crsurface or something to make it more clear?

@@ +404,3 @@
+      cairo_surface_destroy (impl->commited_surface);
+      impl->commited_surface = NULL;
+    }

Shouldn't we do this on begin_paint instead? We might receive a .release after
.frame but before we enter the paint loop.

@@ +483,3 @@
+      g_warning ("Received buffer release notification for untracked buffer");
+      return;
+    }

Is this true branch block really an error? If we

1. commit a buffer A
2. receive .frame
3. commit a buffer B
4. receive .frame
5. receive A.release

At 5. impl->commited will be NULL, and it wouldn't be an error.

The same would happen if we create and read back on .begin_paint.

1. draw frame on A
2. commit a buffer A
3. draw frame on B
4. commit a buffer B
5. receive A.release

At 5. impl->commited would also be NULL, and it wouldn't be an error.

I think what we need to do is something like:

  cairo_surface_t *cairo_surface = wl_buffer_get_user_data (wl_buffer);

  /* Destroy any old released cairo surface we no longer will re-use. */
  if (impl->commited_surface != cairo_surface)
    {
      cairo_surface_destroy (cairo_surface);
      return;
    }

  if (impl->staging_surface == NULL)
    impl->staging_surface = cairo_surface;
  else
    cairo_surface_destroy (cairo_surface);

  impl->commited_surface = NULL;

@@ +759,3 @@
+       * that we didn't draw over
+       */
+      read_back_commited_surface (window);

Is this needed? If we ensure we start with a filled surface on begin_paint,
can't we assume it'll stay filled here as well?

@@ +1951,1 @@
     }

Should we also not do impl->commited_surface = NULL? When, in the future, the
wl_buffer.release is received, it can destroy the cairo surface if it knows its
not to be reused.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-bugs/attachments/20160202/4a19e628/attachment.html>