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 774029 - vaapiwindow: wayland: fix surface leaks.
vaapiwindow: wayland: fix surface leaks.
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
git master
Other Linux
: Normal normal
: 1.11.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 748634 780442
 
 
Reported: 2016-11-07 06:48 UTC by Hyunjun Ko
Modified: 2017-04-24 11:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
libs: window: wayland: fix leaks of surfaces during destory (1.57 KB, patch)
2016-11-07 06:50 UTC, Hyunjun Ko
rejected Details | Review

Description Hyunjun Ko 2016-11-07 06:48:39 UTC
Reproduce step:
1. running GST_DEBUG="GST_TRACER:7,2" GST_TRACERS="leaks" gst-play-1.0 sample.mp4
on *Wayland*.
2. Just quit.

We can see the following log.

0:00:01.269680384   465      0x1c0aac0 TRACE             GST_TRACER :0:: object-alive, type-name=(string)GstVaapiDisplayWayland, address=(gpointer)0x7fc964064230, description=(string)<vaapidisplaywayland1>, ref-count=(uint)4, trace=(string);

Note that this leak happens only whe the tested media is rendering with crop.


In addition, this leak leads to crash when playing videos in playlist by gst-play.

Reproduce step:
1. Running gst-play-1.0 sample1.mp4 sample2.mp4 on *Wayland*
2. Press 'n' to skip to next video.
3. Crashed after about 1 sec.

This is because VaapiDisplayWayland is not destroyed properly(because of leak) when skipping to the next one.
Comment 1 Hyunjun Ko 2016-11-07 06:50:39 UTC
Created attachment 339219 [details] [review]
libs: window: wayland: fix leaks of surfaces during destory

When destroying window, it should release surfaces,
which was created to filter out.
Comment 2 Víctor Manuel Jáquez Leal 2016-11-07 12:52:29 UTC
Review of attachment 339219 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c
@@ +384,2 @@
 {
   wl_buffer_destroy (wl_buffer);

I don't understand this change.

Please check the commit e4ed64b7, when this was done because the wayland protocol is heavily async.

@@ +528,3 @@
+    if (need_vpp && priv->use_vpp) {
+      gst_vaapi_object_unref (surface);
+    }

Here you are hiding the problem. The surface should be unref when the callback is called from the following wl_buffer_destroy()

Perhaps we have to ensure that the buffer queue is destroyed at error.
Comment 3 Hyunjun Ko 2016-11-08 03:04:20 UTC
Review of attachment 339219 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c
@@ +383,3 @@
 frame_release_callback (void *data, struct wl_buffer *wl_buffer)
 {
   wl_buffer_destroy (wl_buffer);

That's what I was wondering. 
The problem in this test case (which is simply using vaapisink on gst-launch) is,
The frame_release_callback for the last wl_buffer is always not called.
What I understand is that this callback is usually called later, probably when the next buffer is shown.
I need to investigate more for proper fix.

@@ +528,3 @@
+    if (need_vpp && priv->use_vpp) {
+      gst_vaapi_object_unref (surface);
+    }

I don't understand.
There's no callback to unref this surface at this moment, since it is before even creating frame and adding listener to the wl buffer.
Comment 4 Hyunjun Ko 2016-11-08 07:01:57 UTC
(In reply to Hyunjun Ko from comment #3)

> 
> @@ +528,3 @@
> +    if (need_vpp && priv->use_vpp) {
> +      gst_vaapi_object_unref (surface);
> +    }
> 
> I don't understand.
> There's no callback to unref this surface at this moment, since it is before
> even creating frame and adding listener to the wl buffer.

And it should be gst_vaapi_video_pool_put_object instead of gst_vaapi_object_unref to return it to the surface pool.
Comment 5 Víctor Manuel Jáquez Leal 2016-11-14 11:46:48 UTC
check the patch in bug 773689

I just closed it, but I'm having second thoughts. What do you think?
Comment 6 Víctor Manuel Jáquez Leal 2017-04-24 11:35:02 UTC
commit ca314a25ccf81c9740d147c6b20de4c24b354301
Author: Hyunjun Ko <zzoon@igalia.com>
Date:   Wed Apr 19 10:37:19 2017 +0900

    libs: window: wayland: null buffer at destroy()

    Fix leakage of the last wl buffer.

    VAAPI wayland sink needs to send a null buffer while destruction,
    it assures that all the wl buffers are released. Otherwise, the last
    buffer's callback might be not called, which leads to leak of
    GstVaapiDisplay.

    This was inspired by gstwaylandsink.

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

    Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>