After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 748564 - wayland: Fix some leaks
wayland: Fix some leaks
Status: RESOLVED FIXED
Product: gstreamer-vaapi
Classification: Other
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: gstreamer-vaapi maintainer(s)
gstreamer-vaapi maintainer(s)
Depends on:
Blocks: 748634
 
 
Reported: 2015-04-28 02:13 UTC by Olivier Crête
Modified: 2015-04-29 18:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vaapidisplay_wayland: Don't leak the registry proxy (961 bytes, patch)
2015-04-28 02:14 UTC, Olivier Crête
committed Details | Review
wayland: Free all frames on window destruction (3.04 KB, patch)
2015-04-28 02:14 UTC, Olivier Crête
rejected Details | Review

Description Olivier Crête 2015-04-28 02:13:42 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.
Comment 1 Olivier Crête 2015-04-28 02:14:02 UTC
Created attachment 302491 [details] [review]
vaapidisplay_wayland: Don't leak the registry proxy
Comment 2 Olivier Crête 2015-04-28 02:14:06 UTC
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.
Comment 3 Gwenole Beauchesne 2015-04-28 08:20:35 UTC
Review of attachment 302491 [details] [review]:

OK.
Comment 4 Gwenole Beauchesne 2015-04-28 08:22:38 UTC
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 5 Víctor Manuel Jáquez Leal 2015-04-28 08:56:20 UTC
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
Comment 6 Víctor Manuel Jáquez Leal 2015-04-28 09:43:55 UTC
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 :)
Comment 7 Gwenole Beauchesne 2015-04-28 09:53:32 UTC
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.
Comment 8 Víctor Manuel Jáquez Leal 2015-04-28 10:06:22 UTC
(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.
Comment 9 Víctor Manuel Jáquez Leal 2015-04-28 10:18:06 UTC
(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 10 Olivier Crête 2015-04-29 14:23:50 UTC
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.
Comment 11 Víctor Manuel Jáquez Leal 2015-04-29 18:25:32 UTC
Closing this bug as the first patch is already committed and the second rejected.