GNOME Bugzilla – Bug 775445
adaptivedemux: Fix deadlock during transition from track disable to enable
Last modified: 2017-03-07 10:50:01 UTC
adaptivedemux: Fix deadlock during transition from track disable to enable When restarting download task due to track enable, taking lock when querying postion causes deadlock.
Created attachment 341111 [details] [review] adaptivedemux: Fix deadlock during transition from track disable to enable
Problem can be reproduced by gst-play-1.0 http://dash.akamaized.net/dash264/TestCases/10a/1/iis_forest_short_poem_multi_lang_480p_single_adapt_aaclc_sidx.mpd And, press 'a' for audio track change
Review of attachment 341111 [details] [review]: ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +3241,3 @@ } } } I don't think it's safe to iterate the streams list without holding the manifest lock - it might be modified by another thread.
Does it happen always? I Tried switching a few times but couldn't reproduce the issue.
Do you have a backtrace of the deadlock?
Created attachment 341142 [details] AdaptiveDemux log Frequency is not always... And it depends on log level, so seems to be timing issue.
Created attachment 341143 [details] backtrace
Created attachment 341298 [details] deadlock log
I attached new log which can show where deadlock happen. The problem is that, Seq 1. gst_input_selector_set_active_pad() taking [input-selector lock] and inside the function, "reconfigure event" are pushed to "old" pad and "new pad". https://cgit.freedesktop.org/gstreamer/gstreamer/tree/plugins/elements/gstinputselector.c#n1391 Seq 2. adaptivedemux got the first "reconfigure event". Then, download task will be start. then, return the event. https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/adaptivedemux/gstadaptivedemux.c#n1576 Seq 3. in the download loop, query_position was forwarded to inputselector. Now, we are in [manifest lock]. https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/adaptivedemux/gstadaptivedemux.c#n3217 Seq 4. to forwarding query_position in input-selector, gst_input_selector_get_linked_pad() should be called, but cannot, because it also taking [input-selector lock]. So, query_position cannot be returned, now. Seq 5. input-selector sent the last "reconfigure event" to adaptivedemux and it takes [manifest lock]. But, it was taken at Seq 3, so we cannot go further.
Created attachment 341304 [details] [review] inputselector: Always proxy position query During active-pad switch, upstream element might query the current position
Review of attachment 341304 [details] [review]: ::: plugins/elements/gstinputselector.c @@ +680,3 @@ + * upstream element might query the current position + * See https://bugzilla.gnome.org/show_bug.cgi?id=775445 */ + res = gst_pad_peer_query (self->srcpad, query); The same should also happen for DURATION then I assume? It seems to make sense in general to just forward these always to the srcpad
(In reply to Sebastian Dröge (slomo) from comment #11) > Review of attachment 341304 [details] [review] [review]: > I agree with you, but hard to imagine what it will cause, because I cannot find any situation that upstream element queries DURATION to downstream element.... Should I include code for DURATION in this patch?
Yes please, just for consistency really
Created attachment 347349 [details] [review] inputselector: Always proxy position/duration query active-pad switch causes reconfigure event with lock taken, and upstream element might query the current position or duration before returning the reconfigure event. Meanwhile, gst_input_selector_get_linked_pad() is used to get srcpad inside of default query handle, and it takes also lock. Since inputselector is still locked by active-pad switch, and so the query cannot be handled further.
Attachment 347349 [details] pushed as 8908227 - inputselector: Always proxy position/duration query