[Wayland-bugs] [Bug 761312] memory leak

[gtk+ (GNOME Bugzilla)]

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

--- Comment #27 from Ray Strode [halfline] <rstrode at redhat.com> ---
(In reply to Jonas Ådahl from comment #22)
> Review of attachment 320235 [details] [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 comment #23)
> > 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.

-- 
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/425305d7/attachment.html>