GNOME Bugzilla – Bug 700806
pad: Sticky events are dropped before stored on a pad and nothing is resending them
Last modified: 2013-05-27 10:48:35 UTC
The gstbasesrc uses gst_base_src_send_stream_start() to send STREAM_START events if needed. The function checks the src->priv->stream_start_pending variable to determine if it needs to send the event. If so, the event is pushed and the variable is un-conditionally set to false, which means that no more attempt to send the event shall be made. No check is done to see if the event was successfully pushed or not. But pushing the event may fail, For example, when a pipeline changes state to PAUSED the element will be started, and during the start sequence a task for the gst_base_src_loop() is created. The very first thing done from gst_base_src_loop() is to call gst_base_src_send_stream_start(). Depending on how threads are scheduled the application may issue a seek before the START_STREAM event reaches the src'c src pad, and the pad may be flushing, which means that it will drop the event. Later, when data is pushed on the src pad, gst_pad_push_data() checks whether a STREAM_START event has been received, and if it has not received it a g_warning message is printed. And this make the unit tests for the adder element in gst-plugins-base fail. I think that the solution is to modify gst_base_src_send_stream_start() to check the return value of gst_pad_push_event(), and not to set src->priv->stream_start_pending to FALSE unless the returnvalue is TRUE. This is what the attached patch does.
Created attachment 244991 [details] [review] gstbasesrc: retry pushing STREAM_START event if needed
<slomo> she-man: that patch is not correct, but it shows a general problem with sticky events currently <slomo> she-man: flushing drops sticky events that are being send, but that wouldn't be dropped from the pad by a flush (only EOS and SEGMENT are dropped from a pad by flush)
Also related is bug #688824 , which seems to be caused by dropping the CAPS event when flushing at unfortunate times.
<she-man> slomo: maybe so, but the problem is that the STREAM_START event is not dropped in the sense that it's removed, it is not attached to the pad in the first place. store_sticky_event() will not attach it while flushing <slomo> she-man: yes, that's what i mean too :) the same happens in the queues for example when an event is in the queue between sinkpad and srcpad, then dropped but was never attached to the srcpad <she-man> slomo: aha, ok <slomo> she-man: and it's not just in basesrc but a general problem with all pads :( <she-man> slomo: would it be possible, or desirable, to modify store_sticky_event() to attach sticke events even thoug the pad is flushing? <slomo> she-man: yes, something like that seems to be the solution <slomo> she-man: but of course only for non-segment, non-eos sticky events. and then there's the potential of getting the even order mixed up because of not storing the segment events but later events
I'm not sure why flush has to remove segments, has you always have to have a segment with valid timestamps anyway (and having one doesn't hurt if you buffers don't have valid timestamps/offsets). So only dropping EOS is probably safe?
I think that's possible now after this commit: commit e0f59d22eb414175c2471458c598681210a210cb Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Thu May 9 10:29:11 2013 +0200 pad: If we push sticky events because of another sticky event, only push those that come before the new event https://bugzilla.gnome.org/show_bug.cgi?id=699937 Before that it was possible that the old segment event was sent downstream after a flush when GstPad was sending sticky events downstream (e.g. because it got a new stream-start event). However we *require* a segment event to arrive after a flush before any data, if we don't remove the old one we could end up using the old segment without getting any warnings or anything. So maybe the old segment should just be "invalidated" instead of being removed, to work as a placeholder there for a new segment event.
Created attachment 245060 [details] [review] 0001-pad-Store-sticky-events-even-if-the-pad-is-flushing.patch This should fix it in theory, can you please check? What I mentioned earlier about event reordering due to removing/dropping the segment event but not others was not true.
I tested your patch, and it looks as it might solve the problem I initially described. At least I haven't been able to make the adder unit tests fail so far. Without your patch they fail at least every second run. With your patch they haven't failed after being run in a loop for more than half an hour. I also ran 'make check' in gstreamer, gst-plugins-base, gst-plugins-good, gst-plugin-bad, and gst-rtsp-server to see if something else was fixed, or if something was broken. The good thing is that nothing seems to be broken by the patch. The bad is that it does not fix all cases. I still get the problem in the unit tests for gst-rtsp-server. However, in that case it's the <rtpsession:send_rtcp_src> pads that get data before receiving a stream-start event. Unfortunately I haven't had time to look into it yet, so I do not yet know what's causing it.
Can you check if this is fixed with the patch I just pushed (slightly different to the attached). Also please file a new bug about the gst-rtsp-server problem, it's a separate problem :) commit 73895c05b116bf0fcfc7c2b12d3f1b04688c8525 Author: Sebastian Dröge <slomo@circular-chaos.org> Date: Mon May 27 12:40:50 2013 +0200 pad: Store sticky events even if the pad is flushing But do this only for events that are not dropped by flushing, i.e. do it only for everything except SEGMENT and EOS. Without this we might drop a CAPS event if flushing happens at an unfortunate time and nobody is resending the CAPS event. https://bugzilla.gnome.org/show_bug.cgi?id=700806