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 711550 - appsrc: Deadlocking because holding mutex while setting caps
appsrc: Deadlocking because holding mutex while setting caps
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.2.0
Other Linux
: Normal major
: 1.2.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-11-06 14:23 UTC by tcdgreenwood
Modified: 2013-11-07 18:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch that does not lock priv->mutex when calling negotiate (2.25 KB, patch)
2013-11-07 15:17 UTC, tcdgreenwood
committed Details | Review

Description tcdgreenwood 2013-11-06 14:23:11 UTC
We have an application using rtpbin and appsrc which leaves us with a deadlock.  From investigations I have found an example of some threads that lock.  It seems to me that the deadlock is between the appsrc priv->mutex lock and the stream lock.  I have reproduced this on 1.2.0 and 1.0.10. The appsrc line is:
      g_mutex_lock (&priv->mutex);

The line of code from the pad is:
        GST_PAD_STREAM_LOCK (pad);

I think the source of the issue is that appsrc list locking priv->mutex before calling gst_app_src_do_negotiate.  As such I am developing a patch that avoids this locking, I think that the priv->mutex is just trying to protect variables such as the caps and new_caps flags.  In this way I think that I can call do_negotiate outside of the lock as long as I ensure that the new_caps flag is acted on after caps is changed.  Is there any reason why do_negotiate needs to be inside the priv->mutex lock for appsrc?  I understand that it should be thread safe so it's just ensuring that the appsrc logic is correct and caps changes are responded to on the next check.  This plan would also follow some of the other logic in appsrc where it unlocks before making callbacks to the application.

Here are the full back traces of the two threads that appear deadlocked and the relevant mutexes 

Thread 66 (Thread 0x7f185c4ca700 (LWP 15312))

  • #0 __lll_lock_wait
    from /lib64/libpthread.so.0
  • #1 _L_lock_892
    from /lib64/libpthread.so.0
  • #2 pthread_mutex_lock
    from /lib64/libpthread.so.0
  • #3 gst_pad_send_event_unchecked
    at gstpad.c line 4782
  • #4 gst_pad_push_event_unchecked
    at gstpad.c line 4515
  • #5 push_sticky
    at gstpad.c line 3286
  • #6 events_foreach
    at gstpad.c line 514
  • #7 check_sticky
    at gstpad.c line 3334
  • #8 gst_pad_push_event
    at gstpad.c line 4636
  • #9 gst_rtp_jitter_buffer_sink_event
    at gstrtpjitterbuffer.c line 1056
  • #10 gst_pad_send_event_unchecked
    at gstpad.c line 4822
  • #11 gst_pad_push_event_unchecked
    at gstpad.c line 4515
  • #12 push_sticky
    at gstpad.c line 3286
  • #13 events_foreach
    at gstpad.c line 514
  • #14 check_sticky
    at gstpad.c line 3334
  • #15 gst_pad_push_event
    at gstpad.c line 4636
  • #16 gst_rtp_ssrc_demux_sink_event
    at gstrtpssrcdemux.c line 570
  • #17 gst_pad_send_event_unchecked
    at gstpad.c line 4822
  • #18 gst_pad_push_event_unchecked
    at gstpad.c line 4515
  • #19 push_sticky
    at gstpad.c line 3286
  • #20 events_foreach
    at gstpad.c line 514
  • #21 check_sticky
    at gstpad.c line 3334
  • #22 gst_pad_push_event
    at gstpad.c line 4636
  • #23 gst_rtp_session_event_recv_rtp_sink
    at gstrtpsession.c line 1335
  • #24 gst_pad_send_event_unchecked
    at gstpad.c line 4822
  • #25 gst_pad_push_event_unchecked
    at gstpad.c line 4515
  • #26 push_sticky
    at gstpad.c line 3286
  • #27 events_foreach
    at gstpad.c line 514
  • #28 check_sticky
    at gstpad.c line 3334
  • #29 gst_pad_push_event
    at gstpad.c line 4636
  • #30 event_forward_func
    at gstpad.c line 2720
  • #31 gst_pad_forward
    at gstpad.c line 2674
  • #32 gst_pad_event_default
    at gstpad.c line 2771
  • #33 gst_pad_send_event_unchecked
    at gstpad.c line 4822
  • #34 gst_pad_push_event_unchecked
    at gstpad.c line 4515
  • #35 push_sticky
    at gstpad.c line 3286
  • #36 events_foreach
    at gstpad.c line 514
  • #37 check_sticky
    at gstpad.c line 3334
  • #38 gst_pad_push_event
    at gstpad.c line 4636
  • #39 gst_pad_set_caps
    at ../../../gst/gstcompat.h line 71
  • #40 gst_base_src_set_caps
    at gstbasesrc.c line 881
  • #41 gst_app_src_do_negotiate
    at gstappsrc.c line 937
  • #42 gst_app_src_negotiate
    at gstappsrc.c line 954
  • #43 gst_base_src_negotiate
    at gstbasesrc.c line 3101
  • #44 gst_base_src_loop
    at gstbasesrc.c line 2580
  • #45 gst_task_func
    at gsttask.c line 316
  • #46 default_func
    at gsttaskpool.c line 70
  • #47 g_thread_pool_thread_proxy
    at gthreadpool.c line 309
  • #48 g_thread_proxy
    at gthread.c line 797
  • #49 start_thread
    from /lib64/libpthread.so.0
  • #50 clone
    from /lib64/libc.so.6

Thread 32 (Thread 0x7f1852261700 (LWP 15346))

  • #0 __lll_lock_wait
    from /lib64/libpthread.so.0
  • #1 _L_lock_995
    from /lib64/libpthread.so.0
  • #2 pthread_mutex_lock
    from /lib64/libpthread.so.0
  • #3 g_mutex_lock
    at gthread-posix.c line 210
  • #4 gst_app_src_query
    at gstappsrc.c line 799
  • #5 gst_base_src_query
    at gstbasesrc.c line 1259
  • #6 gst_pad_query
    at gstpad.c line 3419
  • #7 gst_pad_peer_query
    at gstpad.c line 3550
  • #8 query_forward_func
    at gstpad.c line 2910
  • #9 gst_pad_forward
    at gstpad.c line 2674
  • #10 gst_pad_query_default
    at gstpad.c line 2974
  • #11 gst_pad_query
    at gstpad.c line 3419
  • #12 gst_pad_peer_query
    at gstpad.c line 3550
  • #13 query_forward_func
    at gstpad.c line 2910
  • #14 gst_pad_forward
    at gstpad.c line 2674
  • #15 gst_pad_query_default
    at gstpad.c line 2974
  • #16 gst_pad_query
    at gstpad.c line 3419
  • #17 gst_pad_peer_query
    at gstpad.c line 3550
  • #18 gst_rtp_ssrc_demux_src_query
    at gstrtpssrcdemux.c line 878
  • #19 gst_pad_query
    at gstpad.c line 3419
  • #20 gst_pad_peer_query
    at gstpad.c line 3550
  • #21 gst_rtp_jitter_buffer_src_query
    at gstrtpjitterbuffer.c line 2144
  • #22 gst_pad_query
    at gstpad.c line 3419
  • #23 gst_pad_peer_query
    at gstpad.c line 3550
  • #24 query_forward_func
    at gstpad.c line 2910
  • #25 gst_pad_forward
    at gstpad.c line 2674
  • #26 gst_pad_query_default
    at gstpad.c line 2974
  • #27 gst_pad_query
    at gstpad.c line 3419
  • #28 gst_pad_peer_query
    at gstpad.c line 3550
  • #29 query_forward_func
    at gstpad.c line 2910
  • #30 gst_pad_forward
    at gstpad.c line 2674
  • #31 gst_pad_query_default
    at gstpad.c line 2974
  • #32 gst_pad_query
    at gstpad.c line 3419
  • #33 gst_pad_peer_query
    at gstpad.c line 3550
  • #34 query_forward_func
    at gstpad.c line 2910
  • #35 gst_pad_forward
    at gstpad.c line 2674
  • #36 gst_pad_query_default
    at gstpad.c line 2974
  • #37 gst_pad_query
    at gstpad.c line 3419
  • #38 gst_pad_peer_query
    at gstpad.c line 3550
  • #39 gst_ffmpegviddec_set_format
    from /opt/cafex/fusion_media_broker/native/lib/gstreamer-1.0/libgstlibav.so
  • #40 gst_video_decoder_setcaps
    at gstvideodecoder.c line 807
  • #41 gst_video_decoder_chain
    at gstvideodecoder.c line 1968
  • #42 gst_pad_chain_data_unchecked
    at gstpad.c line 3655
  • #43 gst_pad_push_data
    at gstpad.c line 3872
  • #44 gst_pad_push
    at gstpad.c line 3975
  • #45 gst_rtp_base_depayload_push
    at gstrtpbasedepayload.c line 608
  • #46 gst_rtp_base_depayload_chain
    at gstrtpbasedepayload.c line 356
  • #47 gst_pad_chain_data_unchecked
    at gstpad.c line 3655
  • #48 gst_pad_push_data
    at gstpad.c line 3872
  • #49 gst_pad_push
    at gstpad.c line 3975
  • #50 gst_proxy_pad_chain_default
    at gstghostpad.c line 128
  • #51 gst_pad_chain_data_unchecked
    at gstpad.c line 3655
  • #52 gst_pad_push_data
    at gstpad.c line 3872
  • #53 gst_pad_push
    at gstpad.c line 3975
  • #54 gst_rtp_pt_demux_chain
    at gstrtpptdemux.c line 439
  • #55 gst_pad_chain_data_unchecked
    at gstpad.c line 3655
  • #56 gst_pad_push_data
    at gstpad.c line 3872
  • #57 gst_pad_push
    at gstpad.c line 3975
  • #58 gst_rtp_jitter_buffer_loop
    at gstrtpjitterbuffer.c line 1903
  • #59 gst_task_func
    at gsttask.c line 316
  • #60 default_func
    at gsttaskpool.c line 70
  • #61 g_thread_pool_thread_proxy
    at gthreadpool.c line 309
  • #62 g_thread_proxy
    at gthread.c line 797
  • #63 start_thread
    from /lib64/libpthread.so.0
  • #64 clone
    from /lib64/libc.so.6
  • #2 pthread_mutex_lock
    from /lib64/libpthread.so.0
  • #2 pthread_mutex_lock
    from /lib64/libpthread.so.0
rax            0xfffffffffffffe00	-512
rbx            0x7f18e1eaae06	139744846196230
rcx            0xffffffffffffffff	-1
rdx            0x7f18a4001f40	139743807414080
rsi            0x80	128
rdi            0x7f18a4001a40	139743807412800
rbp            0x7f185c4c89e0	0x7f185c4c89e0
rsp            0x7f185c4c8928	0x7f185c4c8928
r8             0x7f18a4001a40	139743807412800
r9             0x3bd0	15312
r10            0xfffffffffffff9f8	-1544
r11            0x202	514
r12            0x7f18e132b430	139744834139184
r13            0x0	0
r14            0x4	4
r15            0x7	7
rip            0x7f18f3666287	0x7f18f3666287 <pthread_mutex_lock+103>
eflags         0x202	[ IF ]
cs             0x33	51
ss             0x2b	43
ds             0x0	0
es             0x0	0
fs             0x0	0
gs             0x0	0
(gdb) p *(pthread_mutex_t*)0x7f18a4001a40
$2 = {__data = {__lock = 2, __count = 1, __owner = 15346, __nusers = 1, __kind = 1, __spins = 0, __list = {__prev = 0x0, __next = 0x0}}, 
  __size = "\002\000\000\000\001\000\000\000\362;\000\000\001\000\000\000\001", '\000' <repeats 22 times>, __align = 4294967298}
Comment 1 tcdgreenwood 2013-11-06 14:36:37 UTC
Sorry bugzilla corrupted the last part of that - I think when it was processing the backtraces.  Here is the register/mutex info:

(gdb) thread 32
[Switching to thread 32 (Thread 0x7f1852261700 (LWP 15346))]#0  0x00007f18f366b054 in __lll_lock_wait () from /lib64/libpthread.so.0
(gdb) frame 2
  • #2 pthread_mutex_lock
    from /lib64/libpthread.so.0
  • #2 pthread_mutex_lock
    from /lib64/libpthread.so.0
rax            0xfffffffffffffe00	-512
rbx            0x7f18e1eaae06	139744846196230
rcx            0xffffffffffffffff	-1
rdx            0x7f18a4001f40	139743807414080
rsi            0x80	128
rdi            0x7f18a4001a40	139743807412800
rbp            0x7f185c4c89e0	0x7f185c4c89e0
rsp            0x7f185c4c8928	0x7f185c4c8928
r8             0x7f18a4001a40	139743807412800
r9             0x3bd0	15312
r10            0xfffffffffffff9f8	-1544
r11            0x202	514
r12            0x7f18e132b430	139744834139184
r13            0x0	0
r14            0x4	4
r15            0x7	7
rip            0x7f18f3666287	0x7f18f3666287 <pthread_mutex_lock+103>
eflags         0x202	[ IF ]
cs             0x33	51
ss             0x2b	43
ds             0x0	0
es             0x0	0
fs             0x0	0
gs             0x0	0
(gdb) p *(pthread_mutex_t*)0x7f18a4001a40
$2 = {__data = {__lock = 2, __count = 1, __owner = 15346, __nusers = 1, __kind = 1, __spins = 0, __list = {__prev = 0x0, __next = 0x0}}, 
  __size = "\002\000\000\000\001\000\000\000\362;\000\000\001\000\000\000\001", '\000' <repeats 22 times>, __align = 4294967298}
Comment 2 tcdgreenwood 2013-11-07 15:17:52 UTC
Created attachment 259196 [details] [review]
Patch that does not lock priv->mutex when calling negotiate

This patch does a similar thing around do_negotiate as done with the callbacks.  Lock is released and then after the lock we recheck flushing status, caps have changed again and if any data is available in the queue.

Tested on Mac and briefly on Centos.
Comment 3 Sebastian Dröge (slomo) 2013-11-07 17:28:42 UTC
commit 360ac34425539eaabfa6f6106ea1a820460f7101
Author: Tom Greenwood <tcdgreenwood@hotmail.com>
Date:   Thu Nov 7 15:03:34 2013 +0000

    appsrc: Fix deadlock that may occur when multiple threads access appsrc at once
    
    https://bugzilla.gnome.org/show_bug.cgi?id=711550