Bug 784846 – audioaggregator: Deadlock for specific files playback since 20bf97f08912f0edb72bfdcdde4e5c40acb29823

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 784846 - audioaggregator: Deadlock for specific files playback since 20bf97f08912f0edb72bfdcdde4e5c40acb29823


Summary:	audioaggregator: Deadlock for specific files playback since 20bf97f08912f0edb...


Status:	RESOLVED FIXED

Product:	GStreamer
Classification:	Platform
Component:	gst-plugins-bad
Version:	git master
Hardware:	Other Linux

Importance:	Normal blocker
Target Milestone:	1.13.1
Assigned To:	GStreamer Maintainers
QA Contact:	GStreamer Maintainers

URL:
Whiteboard:

Depends on:	784911
Blocks:

Reported:	2017-07-12 13:33 UTC by Vivia Nikolaidou
Modified:	2017-10-23 10:11 UTC

See Also:	784911
GNOME target:	---
GNOME version:	---

Attachments
audioaggregator: Accept buffer with no data, but duration and gap flag (1.40 KB, patch) 2017-07-13 23:13 UTC, Olivier Crête	committed	Details \| Review

Description Vivia Nikolaidou 2017-07-12 13:33:49 UTC

Using this file:

https://ahiru.eu/~vivia/v.mov

and this pipeline:

gst-launch-1.0 -v filesrc location='./v.mov' !  decodebin name=d ! video/x-raw ! fakesink d. ! audio/x-raw ! tee ! queue ! audiomixer ! fakesink

the pipeline doesn't preroll. Removing the video path of the pipeline, or the audiomixer, makes it work.

I found it to work in 1.12.1 and a git bisect showed me this commit as the first bad one:

commit 20bf97f08912f0edb72bfdcdde4e5c40acb29823
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Sun May 21 14:34:13 2017 +0200

    aggregator: Check for the result of caps events
    
    https://bugzilla.gnome.org/show_bug.cgi?id=782918

A backtrace shows that it's a deadlock:

+ Trace 237642

Thread 11 (Thread 0x7fffd80b9700 (LWP 12277))

#0 syscall
at ../sysdeps/unix/sysv/linux/x86_64/syscall.S line 38
#1 g_mutex_lock_slowpath
at ././glib/gthread-posix.c line 1313
#2 g_mutex_lock
at ././glib/gthread-posix.c line 1337
#3 g_cond_wait
at ././glib/gthread-posix.c line 1396
#4 gst_aggregator_pad_query_func
at gstaggregator.c line 2659
#5 gst_pad_query
at gstpad.c line 3950
#6 gst_pad_peer_query
at gstpad.c line 4082
#7 gst_base_transform_default_propose_allocation
at gstbasetransform.c line 1367
#8 gst_base_transform_default_query
at gstbasetransform.c line 1471
#9 gst_pad_query
at gstpad.c line 3950
#10 gst_pad_peer_query
at gstpad.c line 4082
#11 query_forward_func
at gstpad.c line 3318
#12 gst_pad_forward
at gstpad.c line 2946
#13 gst_pad_query_default
at gstpad.c line 3385
#14 gst_decode_pad_query
at gstdecodebin2.c line 5067
#15 gst_pad_query
at gstpad.c line 3950
#16 gst_pad_peer_query
at gstpad.c line 4082
#17 gst_audio_decoder_negotiate_default
at gstaudiodecoder.c line 659
#18 gst_audio_decoder_negotiate_unlocked
at gstaudiodecoder.c line 709
#19 gst_audio_decoder_allocate_output_buffer
at gstaudiodecoder.c line 3533
#20 gst_ffmpegauddec_audio_frame
at gstavauddec.c line 505
#21 gst_ffmpegauddec_frame
at gstavauddec.c line 628
#22 gst_ffmpegauddec_handle_frame
at gstavauddec.c line 781
#23 gst_audio_decoder_push_buffers
at gstaudiodecoder.c line 1538
#24 gst_audio_decoder_chain_forward
at gstaudiodecoder.c line 1652
#25 gst_audio_decoder_chain
at gstaudiodecoder.c line 1912
#26 gst_pad_chain_data_unchecked
at gstpad.c line 4205
#27 gst_pad_push_data
at gstpad.c line 4457
#28 gst_pad_push
at gstpad.c line 4576
#29 gst_base_parse_push_frame
at gstbaseparse.c line 2520
#30 gst_base_parse_chain
at gstbaseparse.c line 3142
#31 gst_pad_chain_data_unchecked
at gstpad.c line 4205
#32 gst_pad_push_data
at gstpad.c line 4457
#33 gst_pad_push
at gstpad.c line 4576
#34 gst_single_queue_push_one
at gstmultiqueue.c line 1608
#35 gst_multi_queue_loop
at gstmultiqueue.c line 1920
#36 gst_task_func
at gsttask.c line 332
#37 g_thread_pool_thread_proxy
at ././glib/gthreadpool.c line 307
#38 g_thread_proxy
at ././glib/gthread.c line 784
#39 start_thread
at pthread_create.c line 333
#40 clone
at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 97

Thread 6 (Thread 0x7ffff0b46700 (LWP 12205))

#0 syscall
at ../sysdeps/unix/sysv/linux/x86_64/syscall.S line 38
#1 g_cond_wait
at ././glib/gthread-posix.c line 1395
#2 gst_base_sink_wait_preroll
at gstbasesink.c line 2267
#3 gst_base_sink_do_preroll
at gstbasesink.c line 2361
#4 gst_base_sink_do_sync
at gstbasesink.c line 2563
#5 gst_base_sink_chain_unlocked
at gstbasesink.c line 3519
#6 gst_base_sink_chain_main
at gstbasesink.c line 3675
#7 gst_pad_chain_data_unchecked
at gstpad.c line 4205
#8 gst_pad_push_data
at gstpad.c line 4457
#9 gst_pad_push
at gstpad.c line 4576
#10 gst_base_transform_chain
at gstbasetransform.c line 2312
#11 gst_pad_chain_data_unchecked
at gstpad.c line 4205
#12 gst_pad_push_data
at gstpad.c line 4457
#13 gst_pad_push
at gstpad.c line 4576
#14 gst_proxy_pad_chain_default
at gstghostpad.c line 127
#15 gst_pad_chain_data_unchecked
at gstpad.c line 4205
#16 gst_pad_push_data
at gstpad.c line 4457
#17 gst_pad_push
at gstpad.c line 4576
#18 gst_video_decoder_clip_and_push_buf
at gstvideodecoder.c line 3205
#19 gst_video_decoder_finish_frame
at gstvideodecoder.c line 3060
#20 gst_ffmpegviddec_video_frame
at gstavviddec.c line 1679
#21 gst_ffmpegviddec_frame
at gstavviddec.c line 1740
#22 gst_ffmpegviddec_handle_frame
at gstavviddec.c line 1853
#23 gst_video_decoder_decode_frame
at gstvideodecoder.c line 3415
#24 gst_video_decoder_chain_forward
at gstvideodecoder.c line 2141
#25 gst_video_decoder_chain
at gstvideodecoder.c line 2455
#26 gst_pad_chain_data_unchecked
at gstpad.c line 4205
#27 gst_pad_push_data
at gstpad.c line 4457
#28 gst_pad_push
at gstpad.c line 4576
#29 gst_base_transform_chain
at gstbasetransform.c line 2312
#30 gst_pad_chain_data_unchecked
at gstpad.c line 4205
#31 gst_pad_push_data
at gstpad.c line 4457
#32 gst_pad_push
at gstpad.c line 4576
#33 gst_base_parse_push_frame
at gstbaseparse.c line 2520
#34 gst_base_parse_handle_and_push_frame
at gstbaseparse.c line 2337
#35 gst_base_parse_finish_frame
at gstbaseparse.c line 2678
#36 gst_h264_parse_handle_frame_packetized
at gsth264parse.c line 1025
#37 gst_h264_parse_handle_frame
at gsth264parse.c line 1078
#38 gst_base_parse_handle_buffer
at gstbaseparse.c line 2145
#39 gst_base_parse_chain
at gstbaseparse.c line 3227
#40 gst_pad_chain_data_unchecked
at gstpad.c line 4205
#41 gst_pad_push_data
at gstpad.c line 4457
#42 gst_pad_push
at gstpad.c line 4576
#43 gst_single_queue_push_one
at gstmultiqueue.c line 1608
#44 gst_multi_queue_loop
at gstmultiqueue.c line 1920
#45 gst_task_func
at gsttask.c line 332
#46 g_thread_pool_thread_proxy
at ././glib/gthreadpool.c line 307
#47 g_thread_proxy
at ././glib/gthread.c line 784
#48 start_thread
at pthread_create.c line 333
#49 clone
at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 97

Thread 5 (Thread 0x7ffff1347700 (LWP 12204))

#0 syscall
at ../sysdeps/unix/sysv/linux/x86_64/syscall.S line 38
#1 g_cond_wait
at ././glib/gthread-posix.c line 1395
#2 gst_data_queue_push
at gstdataqueue.c line 521
#3 gst_multi_queue_chain
at gstmultiqueue.c line 2109
#4 gst_pad_chain_data_unchecked
at gstpad.c line 4205
#5 gst_pad_push_data
at gstpad.c line 4457
#6 gst_pad_push
at gstpad.c line 4576
#7 gst_qtdemux_decorate_and_push_buffer
at qtdemux.c line 5561
#8 gst_qtdemux_loop_state_movie
at qtdemux.c line 5858
#9 gst_qtdemux_loop
at qtdemux.c line 5935
#10 gst_task_func
at gsttask.c line 332
#11 g_thread_pool_thread_proxy
at ././glib/gthreadpool.c line 307
#12 g_thread_proxy
at ././glib/gthread.c line 784
#13 start_thread
at pthread_create.c line 333
#14 clone
at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 97

Thread 3 (Thread 0x7ffff3a7f700 (LWP 12185))

#0 syscall
at ../sysdeps/unix/sysv/linux/x86_64/syscall.S line 38
#1 g_cond_wait
at ././glib/gthread-posix.c line 1395
#2 gst_task_func
at gsttask.c line 317
#3 g_thread_pool_thread_proxy
at ././glib/gthreadpool.c line 307
#4 g_thread_proxy
at ././glib/gthread.c line 784
#5 start_thread
at pthread_create.c line 333
#6 clone
at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 97

Thread 2 (Thread 0x7ffff4280700 (LWP 12184))

#0 syscall
at ../sysdeps/unix/sysv/linux/x86_64/syscall.S line 38
#1 g_cond_broadcast
at ././glib/gthread-posix.c line 1412
#2 check_events
at gstaggregator.c line 811
#3 gst_aggregator_iterate_sinkpads
at gstaggregator.c line 392
#4 gst_aggregator_aggregate_func
at gstaggregator.c line 1104
#5 gst_task_func
at gsttask.c line 332
#6 g_thread_pool_thread_proxy
at ././glib/gthreadpool.c line 307
#7 g_thread_proxy
at ././glib/gthread.c line 784
#8 start_thread
at pthread_create.c line 333
#9 clone
at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 97



Probably something pre-existing though, because the offending commit isn't really forgetting to unlock anything...?

Comment 1 Olivier Crête 2017-07-13 23:06:55 UTC

First you need the patches from #784911 then this one

Comment 2 Olivier Crête 2017-07-13 23:13:08 UTC

Created attachment 355560 [details] [review]
audioaggregator: Accept buffer with no data, but duration and gap flag

These are produced from GAP events by the base class.

Comment 3 Olivier Crête 2017-07-13 23:13:39 UTC

We need to write a unit test for the GAP flag handling.

Comment 4 Olivier Crête 2017-07-13 23:14:31 UTC

GAP event handling I mean, but buffers with GAP flag too..

Comment 5 Vivia Nikolaidou 2017-07-14 10:46:35 UTC

This patch also works. Thanks! :)

Comment 6 Nicolas Dufresne (ndufresne) 2017-07-14 13:56:35 UTC

Review of attachment 355560 [details] [review]:

Looks good, my comments are just nitpick, no worries if you don't have time.

::: gst-libs/gst/audio/gstaudioaggregator.c
@@ +789,3 @@
+        !GST_BUFFER_FLAG_IS_SET (inbuf, GST_BUFFER_FLAG_GAP)) {
+      GST_WARNING_OBJECT (pad, "Dropping 0-sized buffer missing either a"
+          " duration or a GAP flag: %" GST_PTR_FORMAT, inbuf);

Nitpick, but something simpler like "Dropping non-GAP empty buffers" seems better to me.

@@ +794,3 @@
+
+    pad->priv->size = gst_util_uint64_scale (GST_BUFFER_DURATION (inbuf), rate,
+        GST_SECOND);

How hard would it be to rename size in something like "num_frames" or similar ?

Comment 7 Olivier Crête 2017-10-23 10:10:42 UTC

Merged:

commit fd81f27bd8e5e5b3e37ad4811597748eaedb7bc2 (HEAD -> master, upstream/master, origin/master)
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Thu Jul 13 19:09:43 2017 -0400

    audioaggregator: Accept buffer with no data, but duration and gap flag
    
    These are produced from GAP events by the base class.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=784846