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 748663 - wayland: use a thread for event dispatching
wayland: use a thread for event dispatching
Status: RESOLVED NOTABUG
Product: gstreamer-vaapi
Classification: Other
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: gstreamer-vaapi maintainer(s)
gstreamer-vaapi maintainer(s)
Depends on:
Blocks: 748634
 
 
Reported: 2015-04-29 18:36 UTC by Víctor Manuel Jáquez Leal
Modified: 2015-05-07 10:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wayland: free all frames on window destruction (2.99 KB, patch)
2015-04-29 18:36 UTC, Víctor Manuel Jáquez Leal
none Details | Review
wayland: move the display dispatch to a thread (5.84 KB, patch)
2015-04-29 18:36 UTC, Víctor Manuel Jáquez Leal
none Details | Review
wayland: decouple the wl_buffer from the frame (3.04 KB, patch)
2015-04-29 18:36 UTC, Víctor Manuel Jáquez Leal
none Details | Review

Description Víctor Manuel Jáquez Leal 2015-04-29 18:36:41 UTC
This patchset use one patch of bug #748564, to keep track of the created frames. 
Then a thread is added for event dispatching. The surface "done" callback is kept 
for  throttling purposes and the frame is freed when the buffer is released.

With these two first patches, the artifacts described in bug #748564 seems to 
be mitigated a lot, and parts of the pending patch in bug #747492 are included.

The last patch was added because of the last comment in bug #748564: we cannot 
destroy a buffer in the wayland's queue. Hence this patch decouples the frame
structure from the wl buffer. In this way we reduce the leaked memory to the
queued buffers.
Comment 1 Víctor Manuel Jáquez Leal 2015-04-29 18:36:45 UTC
Created attachment 302601 [details] [review]
wayland: free all frames on window destruction

There can be more than one frame in play now, track all of them.
Comment 2 Víctor Manuel Jáquez Leal 2015-04-29 18:36:51 UTC
Created attachment 302602 [details] [review]
wayland: move the display dispatch to a thread

This patch moves to a thread the the queue dispatching, such as in
waylandsink.

This patch eliminates some artifacts seen in VC1 streams and reduces the
leaking to a one last frame.
Comment 3 Víctor Manuel Jáquez Leal 2015-04-29 18:36:56 UTC
Created attachment 302603 [details] [review]
wayland: decouple the wl_buffer from the frame

This patch takes out the wayland's buffer from the the frame structure. The
buffer is queued to wayland and destroyed in the "release" callback. The
frame is freed in the surface's "done" callback, avoiding memory leaks.
Comment 4 Víctor Manuel Jáquez Leal 2015-04-29 21:08:04 UTC
The main problem with these patches is that it requires glib 2.38 and wayland 1.2. It meas that gstreamer-vaapi ought upgrade its requirements.
Comment 5 Olivier Crête 2015-04-29 21:20:05 UTC
Review of attachment 302602 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c
@@ -482,3 @@
-  /* Wait for the previous frame to complete redraw */
-  if (!gst_vaapi_window_wayland_sync (window))
-    return FALSE;

I think you have to wait here for the frame to be displayed before pushing the next one to do the throttling. Or am I missing something?
Comment 6 Víctor Manuel Jáquez Leal 2015-04-30 09:02:24 UTC
(In reply to Olivier Crête from comment #5)
> Review of attachment 302602 [details] [review] [review]:
> 
> ::: gst-libs/gst/vaapi/gstvaapiwindow_wayland.c
> @@ -482,3 @@
> -  /* Wait for the previous frame to complete redraw */
> -  if (!gst_vaapi_window_wayland_sync (window))
> -    return FALSE;
> 
> I think you have to wait here for the frame to be displayed before pushing
> the next one to do the throttling. Or am I missing something?

Perhaps I'm not doing it correctly, but the surface 'done' callback seems to do the throttling, with no need of calling the event dispatching.
Comment 7 Víctor Manuel Jáquez Leal 2015-05-07 10:57:49 UTC
Using an extra thread is a unneeded burden since the rendering is sync'ed. Hence I'm closing this issue as not-a-bug.