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 758984 - gl/wayland: fix loop test hang in glimagesink
gl/wayland: fix loop test hang in glimagesink
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.6.0
Other Linux
: Normal normal
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 788754 792156 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-12-03 05:55 UTC by Haihua Hu
Modified: 2018-01-04 04:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gl/wayland: fix loop test hang in glimagesink (3.54 KB, patch)
2015-12-03 05:55 UTC, Haihua Hu
rejected Details | Review
gl/wayland: fix loop test hang in glimagesink use two event queue in wayland (5.59 KB, patch)
2015-12-06 06:35 UTC, Haihua Hu
rejected Details | Review

Description Haihua Hu 2015-12-03 05:55:08 UTC
Created attachment 316692 [details] [review]
gl/wayland: fix loop test hang in glimagesink

gl/wayland: fix loop test hang in glimagesink

Don't call gst_gl_wl_display_roundtrip_queue() in window_show(). In glimagesink, gl thread will dispatch event queue and window_show() is called from streaming thread. Gl thread will empty event queue and potentially cause gst_gl_wl_display_roundtrip_queue() blocking the streaming thread to wait for an event occur. Actually, no event can occur because the swap_buffer event is queued by streaming thread but it is blocked.

And also we should destroy event queue when destroy surfaces.

This patch remove gst_gl_window_wayland_egl_show() from GstGLWindowWaylandEGLClass and call create_surfaces() gst_gl_wl_display_roundtrip_queue() in gst_gl_window_wayland_egl_open(). When when destroy surfaces, destroy event queue also.

please help me review. Thanks.
Comment 1 Matthew Waters (ystreet00) 2015-12-03 06:34:48 UTC
Review of attachment 316692 [details] [review]:

Moving create_surfaces() from _show() to _open() is not an option as that will break the GstVideoOverlay use case.
Comment 2 Matthew Waters (ystreet00) 2015-12-03 06:37:37 UTC
These days everything may implement the multi-threaded wayland event API which may help.
Comment 3 Haihua Hu 2015-12-03 07:17:43 UTC
(In reply to Matthew Waters from comment #2)
> These days everything may implement the multi-threaded wayland event API
> which may help.

You mean this issue will be fix by implement multi-threaded wayland event API in the next few days?
Comment 4 Matthew Waters (ystreet00) 2015-12-03 12:28:52 UTC
I'm not aware of anyone working on making the libgstgl/wayland code use the multi-threaded event loop API.  The majority of the API already seems to exist in wayland though.
Comment 5 Haihua Hu 2015-12-04 09:49:19 UTC
(In reply to Matthew Waters from comment #4)
> I'm not aware of anyone working on making the libgstgl/wayland code use the
> multi-threaded event loop API.  The majority of the API already seems to
> exist in wayland though.

I have searched the wayland reference manual, can we create two event queues, one for gl thread and another for the window_show(). I think this will fix the dispatch conflict and also will not break the GstVideoOverlay use case.
Comment 6 Haihua Hu 2015-12-04 09:54:45 UTC
(In reply to Matthew Waters from comment #4)
> I'm not aware of anyone working on making the libgstgl/wayland code use the
> multi-threaded event loop API.  The majority of the API already seems to
> exist in wayland though.

And I searched the wayland master branch, it seems that the multi-threaded event loop API has not been included yet.
Comment 7 Haihua Hu 2015-12-06 06:35:40 UTC
Created attachment 316832 [details] [review]
gl/wayland: fix loop test hang in glimagesink use two event queue in wayland
Comment 8 Matthew Waters (ystreet00) 2015-12-06 09:39:20 UTC
Review of attachment 316832 [details] [review]:

If we're just adding event queues for multi-threading issues, we should just push everything through the window thread with gst_gl_window_send_message{_async} () instead.
Comment 9 Haihua Hu 2015-12-06 10:05:24 UTC
(In reply to Matthew Waters (ystreet00) from comment #8)
> Review of attachment 316832 [details] [review] [review]:
> 
> If we're just adding event queues for multi-threading issues, we should just
> push everything through the window thread with
> gst_gl_window_send_message{_async} () instead.

I got your point, you mean we don't need to create another event queue but just put it run in gl thread by calling gst_gl_window_send_message{_async}(). 

But I just notice the gl thread will dispatch event queue. I wonder if this, why should we to call gst_gl_wl_display_roundtrip_queue() in _show() again, since the event on the created queue will automatically be dispatched by glthread?
Comment 10 Matthew Waters (ystreet00) 2017-07-14 14:10:41 UTC
Is this still an issue for you and are you working on it?
Comment 11 Matthew Waters (ystreet00) 2018-01-04 04:38:56 UTC
*** Bug 792156 has been marked as a duplicate of this bug. ***
Comment 12 Matthew Waters (ystreet00) 2018-01-04 04:39:03 UTC
*** Bug 788754 has been marked as a duplicate of this bug. ***
Comment 13 Matthew Waters (ystreet00) 2018-01-04 04:41:21 UTC
commit b25413fb1c3e2e442bfe8d4627e9cf7a9d313735
Author: Matthew Waters <matthew@centricular.com>
Date:   Thu Jan 4 15:33:33 2018 +1100

    gl/wayland: move roundtrip on show to window thread
    
    This makes it thread safe and fixes a possible deadlock.
    
    Keeping the roundtrip off the window thread will result in two different
    threads call wl_display_dispatch_queue() for the same queue which
    violates the assumption for _dispatch_queue()'s thread-safety
    guarantees.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=788754
    https://bugzilla.gnome.org/show_bug.cgi?id=792156
    https://bugzilla.gnome.org/show_bug.cgi?id=758984