GNOME Bugzilla – Bug 770766
rtpssrcdemux: Fix race condition in clear-ssrc signal handler
Last modified: 2018-11-03 15:11:24 UTC
Created attachment 334666 [details] [review] rtpssrcdemux: Fix race condition in clear-ssrc signal handler I sometimes run into a critical error like this: GStreamer-CRITICAL **: Padname src_1119852695 is not unique in element rtpssrcdemux2, not adding This is because of a race condition with clear-ssrc and another rtp/rtcp packet with the same ssrc coming in, which could cause a new pad with the same name to be added before the one being cleared was actually removed.
*** Bug 667821 has been marked as a duplicate of this bug. ***
I had the same error in some of my rtp sessions with gstreamer 1.8.2 I can confirm that the patch fix the issue. I've been using this patch with more than 100 rtp sessions and I don't see the error any more. In each rtp session streams are added and removed dynamically during the session.
Review of attachment 334666 [details] [review]: ::: gst/rtpmanager/gstrtpssrcdemux.c @@ +530,3 @@ + gst_element_remove_pad (GST_ELEMENT_CAST (demux), dpad->rtp_pad); + gst_element_remove_pad (GST_ELEMENT_CAST (demux), dpad->rtcp_pad); + GST_PAD_UNLOCK (demux); The problem here is that remove_pad() will emit the pad-removed signal... which might cause the application (or other code) from the signal handler to do something else on ssrcdemux that will take the same mutex again, possibly from another thread. Do you see a way of doing this different without keeping this mutex locked? It's a recursive mutex, so problems are unlikely, it's still problematic though. Apart from that, the patch makes sense.
(In reply to Sebastian Dröge (slomo) from comment #3) > Do you see a way of doing this different without keeping this mutex locked? > It's a recursive mutex, so problems are unlikely, it's still problematic > though. The only way to do this would be to add a separate mutex around the pad adding/removing. I have a patch, but haven't had a chance to actually test it.
Created attachment 336281 [details] [review] rtpssrcdemux: Fix race condition in clear-ssrc signal handler (with a separate lock for pads) Untested patch that adds a new lock around adding/removing pads
(In reply to GstBlub from comment #5) > Untested patch that adds a new lock around adding/removing pads I've been running with this patch and haven't seen any issues so far. However, haven't tested it quite as much as the other patch.
Comment on attachment 336281 [details] [review] rtpssrcdemux: Fix race condition in clear-ssrc signal handler (with a separate lock for pads) Looks generally good, but please explain in the commit message also why you add a new lock and what the different uses of the two locks is (and their locking order).
GstBlub, any chance you could update your commit message? I have the same issue and will love to see this resolved.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/296.