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 749078 - [wayland] finer frame control
[wayland] finer frame control
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: 743569 748634
 
 
Reported: 2015-05-07 16:56 UTC by Víctor Manuel Jáquez Leal
Modified: 2015-06-19 09:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wayland: decouple wl_buffer from frame (3.97 KB, patch)
2015-05-07 18:31 UTC, Víctor Manuel Jáquez Leal
none Details | Review
wayland: use a counter as sync flag (2.30 KB, patch)
2015-05-07 18:31 UTC, Víctor Manuel Jáquez Leal
none Details | Review
wayland: rename frame for last_frame (2.26 KB, patch)
2015-05-07 18:31 UTC, Víctor Manuel Jáquez Leal
none Details | Review
wayland: sync() when destroy() (1.25 KB, patch)
2015-05-07 18:31 UTC, Víctor Manuel Jáquez Leal
none Details | Review
wayland: wl_display_dispatch_queue() can block forever. (2.73 KB, patch)
2015-05-07 18:31 UTC, Víctor Manuel Jáquez Leal
none Details | Review
wayland: decouple wl_buffer from frame (3.97 KB, patch)
2015-05-14 16:39 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
wayland: use a counter as sync flag (2.30 KB, patch)
2015-05-14 16:39 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
wayland: rename frame for last_frame (2.26 KB, patch)
2015-05-14 16:39 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
wayland: wl_display_dispatch_queue() can block forever. (2.78 KB, patch)
2015-05-14 16:39 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapisink: implement unlock/unlock_stop for wayland (8.87 KB, patch)
2015-05-14 16:40 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
wayland: sync() when destroy() (2.89 KB, patch)
2015-05-14 16:40 UTC, Víctor Manuel Jáquez Leal
committed Details | Review

Description Víctor Manuel Jáquez Leal 2015-05-07 16:56:36 UTC
The following patches are meant to fix what bug #747492 and bug #746501 aim.
Comment 1 Víctor Manuel Jáquez Leal 2015-05-07 18:31:06 UTC
Created attachment 303043 [details] [review]
wayland: decouple wl_buffer from 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.

In this way a buffer may be leaked but not the whole frame structure.

- surface 'done' callback is used to throttle the rendering operation and to
  unallocate the frame, but not the buffer.
- buffer 'release' callback is used to destroy wl_buffer.

Original-patch-by: Zhao Halley <halley.zhao@intel.com>
* code rebase
* kept the the event_queue for buffer's proxy

Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Comment 2 Víctor Manuel Jáquez Leal 2015-05-07 18:31:10 UTC
Created attachment 303044 [details] [review]
wayland: use a counter as sync flag

Wayland window has a pointer to the last pushed frame and use it to set the
flag for stopping the queue dispatch loop. This may lead to memory leaks,
since we are not keeping track of all the queued frames structures.

This patch removes the last pushed frame pointer and change the binary flag
for an atomic counter, keeping track of number of queued frames and use it for
the queue dispatch loop.
Comment 3 Víctor Manuel Jáquez Leal 2015-05-07 18:31:15 UTC
Created attachment 303045 [details] [review]
wayland: rename frame for last_frame

Since frame in the private data means the last frame sent, it would
semantically better use last_frame.

Also, this patch makes use of g_atomic_pointer_{compare_and_exchange, set}()
functions.
Comment 4 Víctor Manuel Jáquez Leal 2015-05-07 18:31:20 UTC
Created attachment 303046 [details] [review]
wayland: sync() when destroy()

Before pushing a the new frame, the render() method calls sync() to flush the
pending frames. Nonetheless, the last pushed frames is never sync() leading to
a memory leak, also.

This patch calls sync() in the destroy() to flush the pending frames before
destroying the window.
Comment 5 Víctor Manuel Jáquez Leal 2015-05-07 18:31:25 UTC
Created attachment 303047 [details] [review]
wayland: wl_display_dispatch_queue() can block forever.

wl_display_dispatch_queue() might prevent the pipeline from shutting
down. This can happen e.g. if the wayland compositor exits while the
pipeline is running.

This patch replaces it with these steps:

- With wl_display_prepare_read() all threads announce their intention
  to read.
- wl_display_read_events() is thread save. On threads reads, the other
  wait for it to finish.
- With wl_display_dispatch_queue_pending() each thread dispatches its
  own events.

wl_display_dispatch_queue_pending() was defined since wayland 1.0.2

Original-patch-by: Michael Olbrich <m.olbrich@pengutronix.de>
* stripped out the unlock() unlock_stop() logic
* stripped out the poll handling

Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Comment 6 Víctor Manuel Jáquez Leal 2015-05-14 16:39:43 UTC
Created attachment 303383 [details] [review]
wayland: decouple wl_buffer from 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.

In this way a buffer may be leaked but not the whole frame structure.

- surface 'done' callback is used to throttle the rendering operation and to
  unallocate the frame, but not the buffer.
- buffer 'release' callback is used to destroy wl_buffer.

Original-patch-by: Zhao Halley <halley.zhao@intel.com>
* code rebase
* kept the the event_queue for buffer's proxy

Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Comment 7 Víctor Manuel Jáquez Leal 2015-05-14 16:39:49 UTC
Created attachment 303384 [details] [review]
wayland: use a counter as sync flag

Wayland window has a pointer to the last pushed frame and use it to set the
flag for stopping the queue dispatch loop. This may lead to memory leaks,
since we are not keeping track of all the queued frames structures.

This patch removes the last pushed frame pointer and change the binary flag
for an atomic counter, keeping track of number of queued frames and use it for
the queue dispatch loop.
Comment 8 Víctor Manuel Jáquez Leal 2015-05-14 16:39:54 UTC
Created attachment 303385 [details] [review]
wayland: rename frame for last_frame

Since frame in the private data means the last frame sent, it would
semantically better use last_frame.

Also, this patch makes use of g_atomic_pointer_{compare_and_exchange, set}()
functions.
Comment 9 Víctor Manuel Jáquez Leal 2015-05-14 16:39:59 UTC
Created attachment 303386 [details] [review]
wayland: wl_display_dispatch_queue() can block forever.

wl_display_dispatch_queue() might prevent the pipeline from shutting
down. This can happen e.g. if the wayland compositor exits while the
pipeline is running.

This patch replaces it with these steps:

- With wl_display_prepare_read() all threads announce their intention
  to read.
- wl_display_read_events() is thread save. On threads reads, the other
  wait for it to finish.
- With wl_display_dispatch_queue_pending() each thread dispatches its
  own events.

wl_display_dispatch_queue_pending() was defined since wayland 1.0.2

Original-patch-by: Michael Olbrich <m.olbrich@pengutronix.de>
* stripped out the unlock() unlock_stop() logic
* stripped out the poll handling

Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Comment 10 Víctor Manuel Jáquez Leal 2015-05-14 16:40:05 UTC
Created attachment 303387 [details] [review]
vaapisink: implement unlock/unlock_stop for wayland

Otherwise wl_display_dispatch_queue() might prevent the pipeline from
shutting down. This can happen e.g. if the wayland compositor exits while
the pipeline is running.

Changes:
* renamed unlock()/unlock_stop() to unblock()/unblock_cancel() in gstvaapiwindow
* splitted the patch removing wl_display_dispatch_queue()

Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>

https://bugzilla.gnome.org/show_bug.cgi?id=747492
Comment 11 Víctor Manuel Jáquez Leal 2015-05-14 16:40:10 UTC
Created attachment 303388 [details] [review]
wayland: sync() when destroy()

Before pushing a the new frame, the render() method calls sync() to flush the
pending frames. Nonetheless, the last pushed frame never gets rendered, leading
to a memory leak too.

This patch calls sync() in the destroy() to flush the pending frames before
destroying the window.

Also a is_cancelled flag is added. This flag tells to not flush the event
queue again since the method failed previously or were cancelled by the user.
Comment 12 Víctor Manuel Jáquez Leal 2015-05-18 13:04:02 UTC
Attachment 303383 [details] pushed as 62c3888 - wayland: decouple wl_buffer from frame
Attachment 303384 [details] pushed as c80951a - wayland: use a counter as sync flag
Attachment 303385 [details] pushed as 882387d - wayland: rename frame for last_frame
Comment 13 Víctor Manuel Jáquez Leal 2015-05-18 13:19:57 UTC
Attachment 303386 [details] pushed as 522ec79 - wayland: wl_display_dispatch_queue() can block forever.
Attachment 303387 [details] pushed as 11b9260 - vaapisink: implement unlock/unlock_stop for wayland
Comment 14 Víctor Manuel Jáquez Leal 2015-05-18 13:33:39 UTC
Attachment 303388 [details] pushed as 70eff01 - wayland: sync() when destroy()