GNOME Bugzilla – Bug 732750
videoaggregator: broken locking in setcaps function
Last modified: 2014-07-06 21:53:03 UTC
I don't really understand how the locking is supposed to work there - at least four different locks at the same time! Respect! (PAD_STREAM_LOCK, AGGREGATOR_SETCAPS_LOCK, AGGREGATOR_LOCK, OBJECT_LOCK) All I can say is that it's buggy: Line 558 does GST_OBJECT_UNLOCK (vagg); and then line 613 and 633 call GST_OBJECT_UNLOCK again.
This is gst_videoaggregator_update_src_caps() btw.
This at least un-breaks it: commit 341dc8aecf460bd28868035a8c35d6a7c8bdeec3 Author: Tim-Philipp Müller <tim@centricular.com> Date: Sun Jul 6 22:16:48 2014 +0100 videoaggregator: fix broken locking in update_src_caps function We would unlock an already-unlocked mutex that we never re-locked. https://bugzilla.gnome.org/show_bug.cgi?id=732750
Created attachment 280012 [details] [review] videoaggregator: Fix some more the locking logic in update_src_caps We need the GST_OBJECT_LOCK only to iterate the sinkpads, nothing else.
(In reply to comment #0) > I don't really understand how the locking is supposed to work there - at least > four different locks at the same time! Respect! (PAD_STREAM_LOCK, > AGGREGATOR_SETCAPS_LOCK, AGGREGATOR_LOCK, OBJECT_LOCK) > > All I can say is that it's buggy: > > Line 558 does GST_OBJECT_UNLOCK (vagg); and then line 613 and 633 call > GST_OBJECT_UNLOCK again. Not sure why you talk about the pad stream lock. Then the locking logic is the exact same one as with videomixer, at that time we need the src cpas lock avoid concurrent srcpad caps negotiation, then we need to avoid the aggregate function to be called at that same time. And we need the object lock to iterate the sinkpads, I agree it can look weird but I think it is correct. I attach here a patch that should be more correct as the object lock should be taken only for the iteration part.
Attachment 280012 [details] pushed as e4b07ee - videoaggregator: Fix some more the locking logic in update_src_caps