GNOME Bugzilla – Bug 708636
collectpads: Should set *all* its pads to flushing when set_flushing is called, not only the ones in the public list
Last modified: 2013-09-24 08:44:56 UTC
Created attachment 255583 [details] [review] fixes the reported issue pads->data is the public list. It is dynamically rebuilt at each call to check_collected, in check_pads to be specific. When you add a pad and collectpads have been started, it is not added to the public list. Thus there exists a possible race where : 1) You would add a pad to collectpads while running. 2) You set collectpads to flushing before check_collected has been called again -> the pad is not set to flushing 3) the pad starts pushing data as downstream might not be prepared, in the case of adder it then returns FLOW_FLUSHING. 4) elements like demuxers, when they get a FLOW_FLUSHING, stop their tasks, never to be seen again. 5) I wonder what the hell is happening and spend two days writing a one-liner.
Review of attachment 255583 [details] [review]: From gst_collect_pads_set_flushing(), gst_collect_pads_set_flushing_unlocked() is called without holding the pads GstObject lock, which is documented to protect pad_list. I think the solution is to just take the object lock in gst_collect_pads_set_flushing() ?
Ah right that could be a problem indeed. I don't know why it's called unlocked, and why we couldn't take the object lock while calling it.
Ah OK it's called unlocked because the *STREAM* lock is not taken inside, I don't think it would be a problem to take the OBJECT_LOCK
The if (GST_IS_PAD (cdata->pad)) in gst_collect_pads_set_flushing_unlocked() also sounds very fishy? When can the cdata->pad not be a pad? Probably that check should be removed or replaced with g_assert(GST_IS_PAD(cdata->pad)))
You need to take the GstObject lock there. It is already taken from every caller of set_flushing_unlocked() except for gst_collect_pads_set_flushing(). Taking the object lock there should be fine. I'll update the patch and push.
commit 20f1c96c89e9b5d57a83f81eee4191938c952764 Author: Sebastian Dröge <slomo@circular-chaos.org> Date: Tue Sep 24 10:42:06 2013 +0200 collectpads: Make sure that the object lock is always taken when accessing the private pad list https://bugzilla.gnome.org/show_bug.cgi?id=708636 commit c79e5bbcad3f73fd231d4571d95506fd516f7a98 Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Tue Sep 17 23:23:34 2013 +0200 collectpads: Use private pad list in set_flushing_unlocked pads->data is the public list. It is dynamically rebuilt at each call to check_collected, in check_pads to be specific. When you add a pad and collectpads have been started, it is not added to the public list. Thus there exists a possible race where : 1) You would add a pad to collectpads while running. 2) You set collectpads to flushing before check_collected has been called again -> the pad is not set to flushing 3) the pad starts pushing data as downstream might not be prepared, in the case of adder it then returns FLOW_FLUSHING. 4) elements like demuxers, when they get a FLOW_FLUSHING, stop their tasks, never to be seen again. https://bugzilla.gnome.org/show_bug.cgi?id=708636