<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#c29">Comment # 29</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>(In reply to Ray Strode [halfline] from <a href="show_bug.cgi?id=761312#c27">comment #27</a>) > (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] [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. > Either is fine by me. The only request I have is to make the difference obvious. > > @@ +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. I suppose begin_paint() would only create the staging buffer and read back if there was no staging buffer already created. And yea, we can only ever destroy any buffer after the .release (or if it is replaced by a second attach before a .commit). > > > 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. I didn't realize that. We can't destroy the surface on .frame, since it may be for whatever reason still be in use. > > > 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. Yea, if the compositor behaves that bad, we're screwed anyway :P > > > @@ +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. But won't there be scenarios where we make a almost-full frame memcopy here every frame even though the buffer already contained the correct pixels? If we ensure we have a fully drawn buffer when creating the staging buffer on-demand (possibly in begin_frame), we'd never need to complement after painting. My point being, the regular case for shm buffers is (assuming the compositor copies the shm buffer to the gpu and releases it immediately): 1. draw 2. commit 3. release 4. frame 5. goto 1 If this happens, we should never need to read back at all. If we make the default case handle this, and create the staging buffer on demand and doing a full read back when doing so, we would avoid most read backs except in rare cases. > > (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. (In reply to Ray Strode [halfline] from <a href="show_bug.cgi?id=761312#c28">comment #28</a>) > (In reply to Ray Strode [halfline] from <a href="show_bug.cgi?id=761312#c27">comment #27</a>) > > 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. > To clarify this, a little: in practice the assumption is okay, since we're > only talking about shm buffers and there's no legitimate reason for a > compositor to need to access to a shm buffer after processing the frame, but > conceptually, I guess, it's uncouth to destroy a buffer before it's > explicitly released. Sure, mutter will now release them immediately, but that is not something we can rely on. We must simply never destroy or re-use the buffer before it is explicitly released, because that is how the protocol is specified.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>