GNOME Bugzilla – Bug 724571
In collectpads, the collected function is sometimes called incorrectly.
Last modified: 2014-03-16 17:24:15 UTC
Created attachment 269454 [details] [review] This test highlights the issue. Have two sinks on an element implementing the collected function, push a seek on its src pad, push flush_start + flush_stop + eos on each of the sinks, collected gets called, that's normal. Push a new seek on its src pad, flush_start / flush_stop and a buffer on *only one* of the sinks, collected gets called again, that doesn't seem right to me. The new proposed patch to the collectpads tests is attached.
Created attachment 269649 [details] [review] This patch corrects the observed bug. This patch corrects the observed bug. The previously attached test case "succeeds" in that instead of failing it hangs on the thread joining as logically gst_pad_push now blocks instead of running collected. There still are theoretically races with that patch, but it makes the situation better, Sebastian's rework of collectpads should hopefully make the situation better and the code clearer, I'm available to discuss all this on IRC. I'll attach an updated test case after that.
Created attachment 269650 [details] [review] This test highlights the issue correctly.
Review of attachment 269650 [details] [review]: ::: tests/check/libs/collectpads.c @@ +1040,3 @@ + collected = FALSE; + + gst_pad_push_event (srcpad1, gst_event_new_flush_stop (TRUE)); If these parts are independent, please split it into two tests. @@ +1056,3 @@ + + /* Give a little time to make sure collected is not called */ + meh. Can we do this differently?
The first part is necessary to trigger the bug, the two pads must have had an EOS for priv->eospads to be incremented. As for the sleep, I haven't found an other solution, I am aware that it makes the test theoretically racy, but as the call to gst_pad_push blocks in the thread, one needs to make sure it actually started waiting. I am open to suggestions, I racked my brain but couldn't find. Even if that test can't be merged, at least it helps showing the problem. It would be useful to merge it though, as that's a case that will need testing when reimplementing collectpads.
As you send a flushing seek, do you need to send the flush_start/stop before sending the EOS? Also I wonder why you are using both gst_pad_push_event and gst_pad_send_event? Also how does the test actually use collect pads? I don't see a call to gst_collect_pads_add_pad(). Regarding the sleep, can you use a condition var triggered by the chain function on the peer-pad you are pushing the buffer to?
Comment on attachment 269649 [details] [review] This patch corrects the observed bug. commit d784d59262ebe93d0f7109a5bc2bc59d0f185067 Author: Mathieu Duponchelle <mduponchelle1@gmail.com> Date: Wed Feb 19 02:27:36 2014 +0100 collectpads: When seek flushed, immediately set eospads to 0 This prevents situations where a first branch would get seeked and receive a buffer before all branches got seeked, and thus collected would get called based on EOS from the previous segment. As a consequence, during the process of seeking, don't decrease the eospads number when a FLUSH_STOP is received. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=724571
We send a flushing seek, but there's no one to handle it upstream, so we have to send the flush start / stop ourselves. The usage of collectpads is implemented by the GstAggregator object, defined at the beginning of the file. I don't know how I could use the condition var in collectpads, as it's triggered when the code *stops* waiting. Only solution would be to have a special condition for debugging purposes, but I think we don't want that, and Sebastian seems to agree there's no other way but the sleep.
Yeah, the problem is that you need to check that something is *not* called. And you can only do that by putting an assertion into that and wait for a while if it doesn't happen.