GNOME Bugzilla – Bug 749574
audioaggregator: Deadlock in gst_object_sync_values()
Last modified: 2015-08-16 13:41:03 UTC
Created attachment 303584 [details] [review] An incorrect fix for the issue audioaggregator calls gst_object_sync_values() inside gst_audio_aggregator_fill_buffer() with the pad and aggregator locks taken. This is incorrect since if there's any changes scheduled using GstController, the sync_values call will deadlock while trying to make those changes (since making those changes requires taking those locks). The attached patch is completely wrong, but serves to show the location of the issue. Ideally, fill_buffer() should be refactored to call sync_values() right before taking the locks the same way that videoaggregator does.
A unit test for the issue would be great :)
That crossed my mind too. Will write one. :)
Created attachment 307919 [details] [review] Sync pad values before taking the locks and aggregating audioaggregator: Sync pad values before aggregating We need to sync the pad values before taking the aggregator and pad locks otherwise the element will just deadlock if there's any property changes scheduled using GstController since that involves taking the aggregator and pad locks. Also add a test for this. --- I've run the test without the patch to verify that the test actually works. The application deadlocks as expected.
Review of attachment 307919 [details] [review]: I'm not too happy about how long the object lock is held in audio aggregator, but in the mean time, this patch looks good.
Created attachment 307925 [details] [review] Sync pad values before taking the locks and aggregating Patch wasn't applying due to your commit from yesterday. This fixes that. :)
Thanks, pushed: commit ad8cb458baf875c4bf77c8831fc7aeb926c41d82 Author: Nirbheek Chauhan <nirbheek@centricular.com> Date: Tue May 19 16:08:08 2015 +0530 audioaggregator: Sync pad values before aggregating We need to sync the pad values before taking the aggregator and pad locks otherwise the element will just deadlock if there's any property changes scheduled using GstController since that involves taking the aggregator and pad locks. Also add a test for this. https://bugzilla.gnome.org/show_bug.cgi?id=749574