<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#c27">Comment # 27</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=rstrode%40redhat.com" title="Ray Strode [halfline] <rstrode@redhat.com>"> Ray Strode [halfline]</a> <pre>(In reply to Jonas Ådahl from <a href="show_bug.cgi?id=761312#c22">comment #22</a>) > 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> [review]: > > ::: 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? I agree it's less than optimal but crsurface is hideous looking :-) How about we rename surface to wl_surface, rename subsurface to wl_subsurface? And, it's wordy but, we could call the cairo surface, cairo_staging_surface, and cairo_committed_surface. Alternatively, we stuff all the proxy objects in a server_objects substructure or something. > @@ +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. There may be more than one begin_paint before the .frame callback. We could do it in the next begin_paint after a frame callback. That would give the compositor a little bit longer to release the buffer before we resort to a copy, but doing it in every begin_paint would force a readback of the entire buffer in the case the begin_paint happened before the frame callback. Also, note we destroy the wl_buffer when we destroy the cairo surface, so if we did it in the wrong begin_paint then we could end up destroying the buffer before the frame callback. > 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. well, see above, at step 2 we destroy buffer A so step 5 won't ever happen. > 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. Again, 3 might happen before we receive the .frame callback and we'd destroy the buffer before the .frame callback. Of course, this code all makes the assumption that it's okay to destroy the buffer after the frame callback, which as pointed out in the side discussion in this bug, isn't necessarily true. > 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; you mean from the buffer_release_callback. That sounds fine. It does mean that if the buffer release never comes (because of a compositor bug) then we'll continue to accumulate buffers, but I guess that's unavoidable. We can't really second guess the compositor; if it's broken the user is hosed anyway. I'll update the patch. > @@ +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? We can't assume we're starting with a filled surface. begin_paint might just be painting a small part of the window, and the surface may have already been committed. (In reply to Jonas Ådahl from <a href="show_bug.cgi?id=761312#c23">comment #23</a>) > > At 5. impl->commited would also be NULL, and it wouldn't be an error. > Actually, it wouldn't since committed would only be NULL during paint, and > we wouldn't handle events during painting. committed is set from the frame clock, but not all painting is tied to the frame clock. Widgets can call gdk_window_process_updates or whatever, outside of the frame clock.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>