GNOME Bugzilla – Bug 777009
urisourcebin: Never push actual EOS event to slot
Last modified: 2017-01-11 11:41:50 UTC
Due to the special nature of adaptivedemux, reconfigure happen frequently with seek/track-change. In very exceptional case, following sequence is possible * EOS event is pushed to queue element and still buffers are queued * During draining remaining buffers, reconfigure event happen from downstream * Then, flow return from downstream might not be FLOW_OK Because non FLOW_OK return will cause element error in queue element, actual EOS event shouldn't be used.
Created attachment 343118 [details] [review] urisourcebin: Never push actual EOS event to slot
Created attachment 343119 [details] [review] urisourcebin: Clear EOS state with stream-start/flush event EOS state should cleared with stream-start of flush
Created attachment 343130 [details] [review] urisourcebin: Never push actual EOS event to slot
Review of attachment 343119 [details] [review]: In the commit message, "EOS state should cleared with stream-start of flush" - I think you mean "EOS state should cleared with stream-start or flush-stop" Otherwise, looks good. Please fix the commit message.
Review of attachment 343130 [details] [review]: Looks good, thanks! I will commit this and fix the commit message in the other patch shortly.
Actually, one more question - I don't understand how a failed reconfigure event triggers a failed flow-return from downstream. Elements generally send a reconfigure event, but don't care if it fails. Where is the failure generated?
Created attachment 343140 [details] [review] urisourcebin: Clear EOS state with stream-start/flush-stop event
(In reply to Jan Schmidt from comment #4) > Review of attachment 343119 [details] [review] [review]: Typo fix done :)
Created attachment 343142 [details] [review] urisourcebin: Never push actual EOS event to slot Due to the special nature of adaptivedemux, reconfigure happens frequently with seek/track-change. In very exceptional cases, the following sequence is possible: * EOS event is pushed to queue element and still buffers are queued * During draining remaining buffers, a reconfigure event arrives from downstream * Because the sinkpad is EOS, the reconfigure event fails and triggers a flow return from downstream that is not GST_FLOW_OK. Because non FLOW_OK return will cause element error in queue element, avoid the sinkpad getting marked EOS by using a custom event in place of EOS.
Created attachment 343143 [details] [review] urisourcebin: Clear EOS state with stream-start/flush-stop event The EOS state marker should cleared on stream-start or flush-stop
I updated the commit messages. I'd still like to make the description in the first commit clearer.
(In reply to Jan Schmidt from comment #6) > Actually, one more question - I don't understand how a failed reconfigure > event triggers a failed flow-return from downstream. > > Elements generally send a reconfigure event, but don't care if it fails. > Where is the failure generated? Error will be caused at https://cgit.freedesktop.org/gstreamer/gstreamer/tree/plugins/elements/gstqueue2.c#n3064 Assume that queue2/queue already got EOS event, and then track-change happend. So, at the srcpad loop of queue2/queue will get flow not-linked maybe. It causes element error.
(In reply to Jan Schmidt from comment #9) > Created attachment 343142 [details] [review] [review] > * Because the sinkpad is EOS, the reconfigure event fails and triggers > a flow return from downstream that is not GST_FLOW_OK. > I think my statement was not clear enough, sorry. My point is that, "srcpad's flow return is not GST_FLOW_OK" and it's maybe GST_FLOW_NOT_LINKED. And the non "GST_FLOW_OK" was caused by reconfigured downstream element.
Ah, OK I think I understand. The problem is not reconfigure events, it's that if downstream returns not-linked after queue2 has received an EOS event, it will trigger an element error on the bus. Actually, I don't know why queue and queue2 do that.
Because without it you might be in a situation where streaming stops and no EOS message is ever posted and no ERROR messages is ever posted (we can't return a flow error to the upstream element at this point). And the app is completely unaware of this. I agree it's not quite right though, but not sure what else to do either.
Created attachment 343145 [details] [review] urisourcebin: Never push actual EOS event to slot Due to the special nature of adaptivedemux, reconfigure happens frequently with seek/track-change. In very exceptional cases, the following sequence is possible: * EOS event is pushed to queue element and still buffers are queued * During draining remaining buffers, reconfiguration downstream happens due to track switch. * The queue gets a not-linked flow return from downstream * Because the sinkpad is EOS, the queue registers an error on the bus, causing the pipeline to fail. Avoid the sinkpad getting marked EOS in the first place, by using a custom event in place of EOS.
Created attachment 343146 [details] [review] urisourcebin: Clear EOS state with stream-start/flush-stop event The EOS state marker should cleared on stream-start or flush-stop
OK, I updated the description. Does that match what's happening Seungha?
(In reply to Jan Schmidt from comment #18) > OK, I updated the description. Does that match what's happening Seungha? It's perfect!! :)
Thanks for the re(In reply to Seungha Yang from comment #19) > (In reply to Jan Schmidt from comment #18) > > OK, I updated the description. Does that match what's happening Seungha? > > It's perfect!! :) Great, thanks! Pushed as: commit 87f21a925c5e404add906e7ddd134b52d4367698 Author: Seungha Yang <sh.yang@lge.com> Date: Sun Jan 8 21:53:27 2017 +0900 urisourcebin: Clear EOS state with stream-start/flush-stop event The EOS state marker should cleared on stream-start or flush-stop https://bugzilla.gnome.org/show_bug.cgi?id=777009 commit c10e7c5011e5149712bf3c7c9d9bb8ee765c4ac6 Author: Seungha Yang <sh.yang@lge.com> Date: Sun Jan 8 21:36:04 2017 +0900 urisourcebin: Never push actual EOS event to slot Due to the special nature of adaptivedemux, reconfigure happens frequently with seek/track-change. In very exceptional cases, the following sequence is possible: * EOS event is pushed to queue element and still buffers are queued * During draining remaining buffers, reconfiguration downstream happens due to track switch. * The queue gets a not-linked flow return from downstream * Because the sinkpad is EOS, the queue registers an error on the bus, causing the pipeline to fail. Avoid the sinkpad getting marked EOS in the first place, by using a custom event in place of EOS. https://bugzilla.gnome.org/show_bug.cgi?id=777009
Review of attachment 343145 [details] [review]: ::: gst/playback/gsturisourcebin.c @@ +1256,3 @@ slot = g_object_get_data (G_OBJECT (pad), "urisourcebin.slotinfo"); + + gst_pad_push_event (slot->srcpad, gst_event_new_eos ()); The code after this checks for whether slot is NULL or not, so either this should have the corresponding check, or if it can never be NULL, that check can be removed.
(In reply to Arun Raghavan from comment #21) > Review of attachment 343145 [details] [review] [review]: > > ::: gst/playback/gsturisourcebin.c > @@ +1256,3 @@ > slot = g_object_get_data (G_OBJECT (pad), "urisourcebin.slotinfo"); > + > + gst_pad_push_event (slot->srcpad, gst_event_new_eos ()); > > The code after this checks for whether slot is NULL or not, so either this > should have the corresponding check, or if it can never be NULL, that check > can be removed. Yes, I get coverity reports too ;) Have patch locally - pushing soon.