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 760559 - glvideomixer ! glimagesink deadlocks on resize
glvideomixer ! glimagesink deadlocks on resize
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal critical
: 1.7.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-01-13 02:26 UTC by Matthew Waters (ystreet00)
Modified: 2016-02-16 23:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
glimagesink: don't push a reconfigure event from the GL thread (3.30 KB, patch)
2016-01-13 02:29 UTC, Matthew Waters (ystreet00)
committed Details | Review
glmixer: don't hold the object lock while calling into GL (1.08 KB, patch)
2016-01-13 05:17 UTC, Matthew Waters (ystreet00)
none Details | Review
glmixer: don't hold the object lock while calling into GL (5.93 KB, patch)
2016-01-13 05:26 UTC, Matthew Waters (ystreet00)
committed Details | Review

Description Matthew Waters (ystreet00) 2016-01-13 02:26:58 UTC
glvideomixer ! glimagesink will deadlock rather quickly when attempting to resize the glimagesink window.

obligatory backtrace:

^C
Program received signal SIGINT, Interrupt.
0x00007ffff673918d in poll () from /usr/lib/libc.so.6
(gdb) t a a bt

Thread 6 (Thread 0x7fffdffff700 (LWP 23533))

  • #0 syscall
    from /usr/lib/libc.so.6
  • #1 g_cond_wait
    at gthread-posix.c line 1397
  • #2 gst_aggregator_pad_query_func
    at gstaggregator.c line 2290
  • #3 gst_pad_query
    at gstpad.c line 3898
  • #4 gst_pad_peer_query
    at gstpad.c line 4030
  • #5 gst_gl_base_filter_query
    at gstglbasefilter.c line 211
  • #6 gst_pad_query
    at gstpad.c line 3898
  • #7 gst_pad_peer_query
    at gstpad.c line 4030
  • #8 gst_base_transform_do_bufferpool
    at gstbasetransform.c line 1003
  • #9 gst_base_transform_setcaps
    at gstbasetransform.c line 1392
  • #10 gst_base_transform_reconfigure
    at gstbasetransform.c line 1472
  • #11 gst_base_transform_default_query
    at gstbasetransform.c line 1513
  • #12 gst_gl_base_filter_query
    at gstglbasefilter.c line 257
  • #13 gst_pad_query
    at gstpad.c line 3898
  • #14 gst_pad_peer_query
    at gstpad.c line 4030
  • #15 query_forward_func
    at gstpad.c line 3279
  • #16 gst_pad_forward
    at gstpad.c line 2912
  • #17 gst_pad_query_default
    at gstpad.c line 3346
  • #18 gst_pad_query
    at gstpad.c line 3898
  • #19 gst_pad_peer_query
    at gstpad.c line 4030
  • #20 gst_base_src_prepare_allocation
    at gstbasesrc.c line 3132
  • #21 gst_base_src_negotiate
    at gstbasesrc.c line 3276
  • #22 gst_base_src_loop
    at gstbasesrc.c line 2693
  • #23 gst_task_func
    at gsttask.c line 331
  • #24 g_thread_pool_thread_proxy
    at gthreadpool.c line 307
  • #25 g_thread_proxy
    at gthread.c line 780
  • #26 start_thread
    from /usr/lib/libpthread.so.0
  • #27 clone
    from /usr/lib/libc.so.6

Thread 5 (Thread 0x7fffec9c3700 (LWP 23532))

  • #0 syscall
    from /usr/lib/libc.so.6
  • #1 g_cond_wait
    at gthread-posix.c line 1397
  • #2 gst_aggregator_pad_query_func
    at gstaggregator.c line 2290
  • #3 gst_pad_query
    at gstpad.c line 3898
  • #4 gst_pad_peer_query
    at gstpad.c line 4030
  • #5 gst_gl_base_filter_query
    at gstglbasefilter.c line 211
  • #6 gst_pad_query
    at gstpad.c line 3898
  • #7 gst_pad_peer_query
    at gstpad.c line 4030
  • #8 gst_base_transform_do_bufferpool
    at gstbasetransform.c line 1003
  • #9 gst_base_transform_setcaps
    at gstbasetransform.c line 1392
  • #10 gst_base_transform_reconfigure
    at gstbasetransform.c line 1472
  • #11 gst_base_transform_default_query
    at gstbasetransform.c line 1513
  • #12 gst_gl_base_filter_query
    at gstglbasefilter.c line 257
  • #13 gst_pad_query
    at gstpad.c line 3898
  • #14 gst_pad_peer_query
    at gstpad.c line 4030
  • #15 query_forward_func
    at gstpad.c line 3279
  • #16 gst_pad_forward
    at gstpad.c line 2912
  • #17 gst_pad_query_default
    at gstpad.c line 3346
  • #18 gst_pad_query
    at gstpad.c line 3898
  • #19 gst_pad_peer_query
    at gstpad.c line 4030
  • #20 gst_base_src_prepare_allocation
    at gstbasesrc.c line 3132
  • #21 gst_base_src_negotiate
    at gstbasesrc.c line 3276
  • #22 gst_base_src_loop
    at gstbasesrc.c line 2693
  • #23 gst_task_func
    at gsttask.c line 331
  • #24 g_thread_pool_thread_proxy
    at gthreadpool.c line 307
  • #25 g_thread_proxy
    at gthread.c line 780
  • #26 start_thread
    from /usr/lib/libpthread.so.0
  • #27 clone
    from /usr/lib/libc.so.6

Thread 4 (Thread 0x7fffed1c4700 (LWP 23531))

  • #0 syscall
    from /usr/lib/libc.so.6
  • #1 g_cond_wait
    at gthread-posix.c line 1397
  • #2 gst_gl_window_default_send_message
    at gstglwindow.c line 630
  • #3 gst_gl_context_thread_add
    at gstglcontext.c line 1504
  • #4 _mem_unmap_full
    at gstglbasememory.c line 337
  • #5 gst_memory_unmap
    at gstmemory.c line 346
  • #6 gst_buffer_unmap
    at gstbuffer.c line 1712
  • #7 default_unmap
    at gstvideometa.c line 226
  • #8 gst_video_frame_unmap
    at video-frame.c line 211
  • #9 gst_gl_mixer_process_textures
    at gstglmixer.c line 652
  • #10 gst_gl_mixer_aggregate_frames
    at gstglmixer.c line 725
  • #11 gst_videoaggregator_do_aggregate
    at gstvideoaggregator.c line 1333
  • #12 gst_videoaggregator_aggregate
    at gstvideoaggregator.c line 1539
  • #13 gst_aggregator_aggregate_func
    at gstaggregator.c line 816
  • #14 gst_task_func
    at gsttask.c line 331
  • #15 g_thread_pool_thread_proxy
    at gthreadpool.c line 307
  • #16 g_thread_proxy
    at gthread.c line 780
  • #17 start_thread
    from /usr/lib/libpthread.so.0
  • #18 clone
    from /usr/lib/libc.so.6

Thread 3 (Thread 0x7fffefb05700 (LWP 23526))

  • #0 syscall
    from /usr/lib/libc.so.6
  • #1 g_mutex_lock_slowpath
    at gthread-posix.c line 1315
  • #2 g_mutex_lock
    at gthread-posix.c line 1339
  • #3 gst_iterator_next
    at gstiterator.c line 346
  • #4 gst_pad_forward
    at gstpad.c line 2897
  • #5 gst_aggregator_forward_event_to_all_sinkpads
    at gstaggregator.c line 1648
  • #6 gst_aggregator_default_src_event
    at gstaggregator.c line 1735
  • #7 gst_videoaggregator_src_event
    at gstvideoaggregator.c line 1746
  • #8 gst_pad_send_event_unchecked
    at gstpad.c line 5552
  • #9 gst_pad_push_event_unchecked
    at gstpad.c line 5210
  • #10 gst_pad_push_event
    at gstpad.c line 5347
  • #11 gst_base_transform_src_eventfunc
    at gstbasetransform.c line 2010
  • #12 gst_pad_send_event_unchecked
    at gstpad.c line 5552
  • #13 gst_pad_push_event_unchecked
    at gstpad.c line 5210
  • #14 gst_pad_push_event
    at gstpad.c line 5347
  • #15 gst_base_transform_src_eventfunc
    at gstbasetransform.c line 2010
  • #16 gst_pad_send_event_unchecked
    at gstpad.c line 5552
  • #17 gst_pad_push_event_unchecked
    at gstpad.c line 5210
  • #18 gst_pad_push_event
    at gstpad.c line 5347
  • #19 event_forward_func
    at gstpad.c line 2958
  • #20 gst_pad_forward
  • #21 gst_pad_event_default
    at gstpad.c line 3009
  • #22 gst_pad_send_event_unchecked
    at gstpad.c line 5552
  • #23 gst_pad_push_event_unchecked
    at gstpad.c line 5210
  • #24 gst_pad_push_event
    at gstpad.c line 5347
  • #25 gst_glimage_sink_on_resize
    at gstglimagesink.c line 1900
  • #26 gst_gl_window_resize
    at gstglwindow.c line 1167
  • #27 gst_gl_window_x11_handle_event
    at gstglwindow_x11.c line 556
  • #28 x11_event_source_dispatch
    at x11_event_source.c line 70
  • #29 g_main_dispatch
    at gmain.c line 3154
  • #30 g_main_context_dispatch
    at gmain.c line 3769
  • #31 g_main_context_iterate
    at gmain.c line 3840
  • #32 g_main_loop_run
    at gmain.c line 4034
  • #33 gst_gl_context_create_thread
    at gstglcontext.c line 1234
  • #34 g_thread_proxy
    at gthread.c line 780
  • #35 start_thread
    from /usr/lib/libpthread.so.0
  • #36 clone
    from /usr/lib/libc.so.6

Comment 1 Matthew Waters (ystreet00) 2016-01-13 02:29:03 UTC
Created attachment 318929 [details] [review]
glimagesink: don't push a reconfigure event from the GL thread

This removes the gst_pad_push_event() from the GL thread which prevents the deadlock.
Comment 2 Nicolas Dufresne (ndufresne) 2016-01-13 02:56:19 UTC
Review of attachment 318929 [details] [review]:

Seems fair. Note that glvideomixer is probably wrong too as normally one should not handle the reconfigure event, but instead let the default handler mark the pad. It then should read the reconfigure flag from the streaming thread. There would be not way to link those context together, hence it would not be important which thread sends that upstream event. (it would deadlock all over gstreamer if it was not done this way everywhere else)
Comment 3 Nicolas Dufresne (ndufresne) 2016-01-13 02:57:52 UTC
Btw, make sure high resolution composition meta still work afterward please (even if I believe this change should be armless).
Comment 4 Matthew Waters (ystreet00) 2016-01-13 05:14:00 UTC
glmixer seems to have a deficiency in this area.  The deadlock is between glmixer's object lock and the GL thread submission.  The object lock is needed to be able to iterate over the sink pads and retrieve the texture id's.
Comment 5 Matthew Waters (ystreet00) 2016-01-13 05:17:23 UTC
Created attachment 318937 [details] [review]
glmixer: don't hold the object lock while calling into GL

This unfortunately is not enough as videoaggregator holds the object lock while it attempts to unref some buffers in fill_queues() which will deadlock in the same way.
Comment 6 Matthew Waters (ystreet00) 2016-01-13 05:26:03 UTC
Created attachment 318938 [details] [review]
glmixer: don't hold the object lock while calling into GL

This still isn't enough.
Comment 7 Sebastian Dröge (slomo) 2016-01-18 09:29:41 UTC
What's the remaining problem after these two patches?
Comment 8 Matthew Waters (ystreet00) 2016-01-19 00:28:50 UTC
The two patches are two options.

1. Don't send the reconfigure event from the GL thread.
2. Change glvideomixer et al to not gst_buffer_unref with the object lock held.

Ideally 1 shouldn't need to be performed however currently there are still some occurrences of 2 in videoaggregator et al that would need to be changed to make it work correctly.
Comment 9 Sebastian Dröge (slomo) 2016-01-19 07:49:45 UTC
Well, 2) I'd say then. glvideomixer should not do anything meaningful when receiving the RECONFIGURE event, it should just remember it and then do somethnig next time from its streaming thread.
Comment 10 Matthew Waters (ystreet00) 2016-01-19 17:08:43 UTC
It doesn't do anything special on reconfigure.  It's the pad event forwarding the reconfigure event (which takes a GstIterator and attempts to lock the object lock for access to the sink pads) deadlocking with the GL thread submission as in the backtrace above.
Comment 11 Matthew Waters (ystreet00) 2016-02-16 23:35:11 UTC
This is some of the second solution.

commit 0d94c9ae7f7dbb25285f80852dab0524dc50fa87
Author: Matthew Waters <matthew@centricular.com>
Date:   Wed Jan 13 14:41:22 2016 +1100

    glmixer: don't hold the object lock while calling into GL
    
    Doing so can deadlock between the GL thread and the object lock e.g.
    when performing reconfigure events in glimagesink on a resize event.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=760559
Comment 12 Matthew Waters (ystreet00) 2016-02-16 23:37:07 UTC
And this is the first solution that is complete.

commit ccc17ebe104d009ca9fe66bb59609ec1afa372d5
Author: Matthew Waters <matthew@centricular.com>
Date:   Wed Jan 13 13:17:56 2016 +1100

    glimagesink: don't push a reconfigure event from the GL thread
    
    Doing so may cause deadlocks when other elements attempt destroy or created
    GL resources.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=760559