Created attachment 288151 [details] [review]
[PATCH] Work around ABBA deadlock between vaapisink and vaapipostproc


vaapisink takes the display lock, then does a gst_buffer_replace which can
take the lock on the gst_vaapi_video_pool.

vaapipostproc asks the gst_vaapi_video_pool for a new surface. This takes
the lock on the gst_vaapi_video_pool; if you're unlucky, there are no free
surfaces, which means that gst_vaapi_surface_create is
called. gst_vaapi_surface_create takes the display lock.

If vaapisink and vaapipostproc are in different threads, and this happens,
you get a deadlock. vaapisink holds the display lock, and wants the
gst_vaapi_video_pool lock. vaapipostproc holds the gst_vaapi_video_pool lock
and wants the display lock.

Work around this by releasing the display lock in vaapisink around the
gst_buffer_replace.

Thread 7 (Thread 0x7f1db2171700 (LWP 8580)):
%0  __lll_lock_wait () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
%1  0x00007f1de90d6151 in _L_lock_1022 () from /lib64/libpthread.so.0
%2  0x00007f1de90d60f2 in __GI___pthread_mutex_lock (mutex=0x7f1da4005920) at ../nptl/pthread_mutex_lock.c:134
%3  0x00007f1dde0845a1 in g_mutex_lock (mutex=mutex@entry=0x7f1da40031c8) at gthread-posix.c:213
%4  0x00007f1db2b4cb8a in gst_vaapi_video_pool_put_object (pool=0x7f1da4003180, object=0x7f1da4005480) at gstvaapivideopool.c:235
%5  0x00007f1db2b4af53 in gst_vaapi_surface_proxy_finalize (proxy=0x7f1da40034a0) at gstvaapisurfaceproxy.c:43
%6  0x00007f1db2b4845f in gst_vaapi_mini_object_free (object=0x7f1da40034a0) at gstvaapiminiobject.c:34
%7  gst_vaapi_mini_object_unref (object=0x7f1da40034a0) at gstvaapiminiobject.c:137
%8  0x00007f1db2b484d2 in gst_vaapi_mini_object_replace (old_object_ptr=old_object_ptr@entry=0x7f1dc00d6ea0, new_object=new_object@entry=0x0) at gstvaapiminiobject.c:170
%9  0x00007f1db2b4aeca in gst_vaapi_surface_proxy_replace (old_proxy_ptr=old_proxy_ptr@entry=0x7f1dc00d6ea0, new_proxy=new_proxy@entry=0x0) at gstvaapisurfaceproxy.c:195
%10 0x00007f1db39d7fa7 in gst_vaapi_video_meta_destroy_proxy (meta=0x7f1dc00d6e80) at gstvaapivideometa.c:124
%11 gst_vaapi_video_meta_finalize (meta=0x7f1dc00d6e80) at gstvaapivideometa.c:149
%12 _gst_vaapi_video_meta_free (meta=0x7f1dc00d6e80) at gstvaapivideometa.c:195
%13 gst_vaapi_video_meta_unref (meta=0x7f1dc00d6e80) at gstvaapivideometa.c:400
%14 0x00007f1ddc3a5c11 in _gst_buffer_free (buffer=0x7f1da400b0c0) at gstbuffer.c:563
%15 0x00007f1ddc3cf50c in gst_mini_object_replace (olddata=olddata@entry=0x7f1dc006e6a8, newdata=0x7f1da400b1d0) at gstminiobject.c:513
%16 0x00007f1db39d533b in gst_buffer_replace (nbuf=<optimized out>, obuf=0x7f1dc006e6a8) at /usr/include/gstreamer-1.0/gst/gstbuffer.h:475
%17 gst_vaapisink_show_frame_unlocked (src_buffer=0x7f1da400b1d0, sink=0x7f1dc006e210) at gstvaapisink.c:1283
%18 gst_vaapisink_show_frame (base_sink=0x7f1dc006e210, src_buffer=0x7f1da400b1d0) at gstvaapisink.c:1302
%19 0x00007f1dd153c1f2 in gst_base_sink_chain_unlocked (basesink=basesink@entry=0x7f1dc006e210, obj=obj@entry=0x7f1da400b1d0, is_list=is_list@entry=0, pad=<optimized out>) at gstbasesink.c:3378
%20 0x00007f1dd153d754 in gst_base_sink_chain_main (basesink=0x7f1dc006e210, pad=<optimized out>, obj=0x7f1da400b1d0, is_list=0) at gstbasesink.c:3486
%21 0x00007f1ddc3d3138 in gst_pad_chain_data_unchecked (data=0x7f1da400b1d0, type=4112, pad=0x7f1dc004e0a0) at gstpad.c:3760
%22 gst_pad_push_data (pad=0x7f1dc004edc0, type=type@entry=4112, data=<optimized out>, data@entry=0x7f1da400b1d0) at gstpad.c:3990
%23 0x00007f1ddc3d9f86 in gst_pad_push (pad=<optimized out>, buffer=buffer@entry=0x7f1da400b1d0) at gstpad.c:4093
%24 0x00007f1dd178e5d0 in gst_queue_push_one (queue=0x7f1dc007e400) at gstqueue.c:1115
%25 gst_queue_loop (pad=<optimized out>) at gstqueue.c:1244
%26 0x00007f1ddc4014a9 in gst_task_func (task=0x7f1dbc00a170) at gsttask.c:316
%27 0x00007f1dde069406 in g_thread_pool_thread_proxy (data=<optimized out>) at gthreadpool.c:309
%28 0x00007f1dde068a45 in g_thread_proxy (data=0x7f1dc00a6cf0) at gthread.c:798
%29 0x00007f1de90d3ee5 in start_thread (arg=0x7f1db2171700) at pthread_create.c:309
%30 0x00007f1de84deb8d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

Thread 6 (Thread 0x7f1db1970700 (LWP 8581)):
%0  __lll_lock_wait () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
%1  0x00007f1de90d6136 in _L_lock_870 () from /lib64/libpthread.so.0
%2  0x00007f1de90d602f in __GI___pthread_mutex_lock (mutex=0x7f1dc00460d0) at ../nptl/pthread_mutex_lock.c:114
%3  0x00007f1db2b49b6c in gst_vaapi_surface_create (height=1088, width=1920, chroma_type=GST_VAAPI_CHROMA_TYPE_YUV420, surface=0x7f1dac003740) at gstvaapisurface.c:115
%4  gst_vaapi_surface_new (display=<optimized out>, chroma_type=GST_VAAPI_CHROMA_TYPE_YUV420, width=1920, height=1088) at gstvaapisurface.c:229
%5  0x00007f1db2b4cb49 in gst_vaapi_video_pool_alloc_object (pool=0x7f1da4003180) at gstvaapivideopool.c:55
%6  gst_vaapi_video_pool_get_object_unlocked (pool=0x7f1da4003180) at gstvaapivideopool.c:180
%7  gst_vaapi_video_pool_get_object (pool=0x7f1da4003180) at gstvaapivideopool.c:198
%8  0x00007f1db2b4ae56 in gst_vaapi_surface_proxy_new_from_pool (pool=pool@entry=0x7f1da4003180) at gstvaapisurfaceproxy.c:91
%9  0x00007f1db39d8073 in set_surface_proxy_from_pool (pool=0x7f1da4003180, meta=0x7f1da40026d0) at gstvaapivideometa.c:100
%10 gst_vaapi_video_meta_new_from_pool (pool=0x7f1da4003180) at gstvaapivideometa.c:300
%11 0x00007f1db39d29c7 in gst_vaapipostproc_process_vpp (outbuf=0x7f1da400b500, inbuf=0x7f1dc4071150, trans=0x7f1dc00790d0) at gstvaapipostproc.c:598
%12 gst_vaapipostproc_transform (trans=0x7f1dc00790d0, inbuf=<optimized out>, outbuf=0x7f1da400b500) at gstvaapipostproc.c:1126
%13 0x00007f1dd1549c47 in gst_base_transform_handle_buffer (trans=trans@entry=0x7f1dc00790d0, inbuf=inbuf@entry=0x7f1dc4071150, outbuf=outbuf@entry=0x7f1db196fc60) at gstbasetransform.c:2094
%14 0x00007f1dd154a4d2 in gst_base_transform_chain (pad=<optimized out>, parent=0x7f1dc00790d0, buffer=0x7f1dc4071150) at gstbasetransform.c:2201
%15 0x00007f1ddc3d3138 in gst_pad_chain_data_unchecked (data=0x7f1dc4071150, type=4112, pad=0x7f1dc004e2d0) at gstpad.c:3760
%16 gst_pad_push_data (pad=0x7f1dc004e960, type=type@entry=4112, data=<optimized out>, data@entry=0x7f1dc4071150) at gstpad.c:3990
%17 0x00007f1ddc3d9f86 in gst_pad_push (pad=<optimized out>, buffer=buffer@entry=0x7f1dc4071150) at gstpad.c:4093
%18 0x00007f1dd178e5d0 in gst_queue_push_one (queue=0x7f1dc007e110) at gstqueue.c:1115
%19 gst_queue_loop (pad=<optimized out>) at gstqueue.c:1244
%20 0x00007f1ddc4014a9 in gst_task_func (task=0x7f1dbc00a290) at gsttask.c:316
%21 0x00007f1dde069406 in g_thread_pool_thread_proxy (data=<optimized out>) at gthreadpool.c:309
%22 0x00007f1dde068a45 in g_thread_proxy (data=0x7f1dc00a6d40) at gthread.c:798
%23 0x00007f1de90d3ee5 in start_thread (arg=0x7f1db1970700) at pthread_create.c:309
%24 0x00007f1de84deb8d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
---
 gst/vaapi/gstvaapisink.c | 3 +++
 1 file changed, 3 insertions(+)

I should note that while the patch is against git master, the backtraces are from:

commit 8edcc8f81744246bdedb5fbe7ba2ee05e5e9966f
Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
Date:   Tue Aug 12 18:33:25 2014 +0200

    vaapipostproc: disable discontinuity detection code.
    
    The "discontinuity" tracking code, whereby lost frames are tentatively
    detected, is inoperant if the sink pad buffer timestamps are not right
    to begin with.
    
    This is a temporary workaround until the following bug is fixed:
    https://bugzilla.gnome.org/show_bug.cgi?id=734386

diff --git a/gst/vaapi/gstvaapipostproc.c b/gst/vaapi/gstvaapipostproc.c
index 56646b0..664cd0e 100644
--- a/gst/vaapi/gstvaapipostproc.c
+++ b/gst/vaapi/gstvaapipostproc.c
@@ -518,7 +518,7 @@ gst_vaapipostproc_process_vpp(GstBaseTransform *trans, GstBuffer *inbuf,
 
     deint_method = postproc->deinterlace_method;
     deint_refs = deint_method_is_advanced(deint_method);
-    if (deint_refs) {
+    if (deint_refs && 0) {
         GstBuffer * const prev_buf = ds_get_buffer(ds, 0);
         GstClockTime prev_pts, pts = GST_BUFFER_TIMESTAMP(inbuf);
         /* Reset deinterlacing state when there is a discontinuity */

Thanks for catching this up. I think fixing the FIXME in the caller is in order. tbh, I never really analyzed why this was needed, as I didn't try to run into the suggested issue.

Review of attachment 288151 [details] [review]:

Good catch ! 
Patch lgtm, except it requires rebasing which I can do locally..
Thanks.

I would like to get this patch in for 0.6...Please let me know if anyone have objection :)

(In reply to sreerenj from comment #5)
> I would like to get this patch in for 0.6...Please let me know if anyone
> have objection :)

LGTM

Pushed, Thanks for the patch

commit d946e7972e0d361f2060349440f0b879c458f59b
Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
Date:   Mon Jun 29 13:06:30 2015 +0300