<html> <head> <base href="https://bugzilla.gnome.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - memory leak" href="https://bugzilla.gnome.org/show_bug.cgi?id=761312#c22">Comment # 22</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - memory leak" href="https://bugzilla.gnome.org/show_bug.cgi?id=761312">bug 761312</a> from <a href="page.cgi?id=describeuser.html&login=jadahl%40gmail.com" title="Jonas Ådahl <jadahl@gmail.com>"> Jonas Ådahl</a> <pre>Review of <a href="attachment.cgi?id=320235&action=diff" name="attach_320235" title="wayland: stage uncommited changes to dedicated buffer">attachment 320235</a> <a href="attachment.cgi?id=320235&action=edit" title="wayland: stage uncommited changes to dedicated buffer">[details]</a> <a href='review?bug=761312&attachment=320235'>[review]</a>: ::: 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.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>