GNOME Bugzilla – Bug 748564
wayland: Fix some leaks
Last modified: 2015-04-29 18:25:32 UTC
The registry proxy was being leaked.. Be aware that when an external display connection, the registry object is leaked server-side, but that's a protocol design mistake, so there isn't much we can do. With Michael's new patches, there can be more than one wl_buffer outstanding, so we need to keep track of all of them so they can all be freed when the window is destroyed.
Created attachment 302491 [details] [review] vaapidisplay_wayland: Don't leak the registry proxy
Created attachment 302492 [details] [review] wayland: Free all frames on window destruction There can be more than one frame in play now, track all of them.
Review of attachment 302491 [details] [review]: OK.
Review of attachment 302492 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c @@ +332,1 @@ priv->frame_pending = FALSE; - Can the compositor choose to drop a frame, thus not raising the notification? - If we have two frames pending in the queue, we losing the fact that there are frames pending. Why not s/frame_pending/num_frames_pending/ and use that as a counter?
Comment on attachment 302491 [details] [review] vaapidisplay_wayland: Don't leak the registry proxy commit 4091e86ab6ec25deb8bfa38ddb716731e59131db Author: Olivier Crete <olivier.crete@collabora.com> Date: Mon Apr 27 20:31:50 2015 -0400 wayland: don't leak the registry proxy Release the registry proxy when closing the display. https://bugzilla.gnome.org/show_bug.cgi?id=748564
Review of attachment 302492 [details] [review]: This patch seems to fix, for real, the artifact that is shown in some videos, which bug 747902 tried to fix with that horrible hack :)
Review of attachment 302492 [details] [review]: This doesn't change the fact that if there is more than 1 frame in the queue, the _sync() is no longer serving its purposed role, and if this works, then I am inclined to think there is another issue.
(In reply to Víctor Manuel Jáquez Leal from comment #6) > Review of attachment 302492 [details] [review] [review]: > > This patch seems to fix, for real, the artifact that is shown in some > videos, which bug 747902 tried to fix with that horrible hack :) Also, I have changed the boolean flag with an integer accumulator (num_frames_pending) and found out that always two last frames are not rendered.
(In reply to Gwenole Beauchesne from comment #7) > Review of attachment 302492 [details] [review] [review]: > > This doesn't change the fact that if there is more than 1 frame in the > queue, the _sync() is no longer serving its purposed role, and if this > works, then I am inclined to think there is another issue. I spoke too fast. Sorry. There are still some artifacts, but they are not a flashy as before. Perhaps something changed in my last jhbuild.
Comment on attachment 302492 [details] [review] wayland: Free all frames on window destruction Seems like we can't do that, Pekka we have to wait until the wl_buffer_release event before releasing the frames to prevent bad things on the screen. So I think doing frame_state_free() in gst_vaapi_window_wayland_destroy() may be wrong, we're possibly stuck having to keep the object alive until the callback is received, which means sync stopping can't be done. Very annoying.
Closing this bug as the first patch is already committed and the second rejected.