After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 777009 - urisourcebin: Never push actual EOS event to slot
urisourcebin: Never push actual EOS event to slot
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-01-08 13:13 UTC by Seungha Yang
Modified: 2017-01-11 11:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
urisourcebin: Never push actual EOS event to slot (6.68 KB, patch)
2017-01-08 13:14 UTC, Seungha Yang
none Details | Review
urisourcebin: Clear EOS state with stream-start/flush event (2.62 KB, patch)
2017-01-08 13:14 UTC, Seungha Yang
none Details | Review
urisourcebin: Never push actual EOS event to slot (6.67 KB, patch)
2017-01-08 23:34 UTC, Seungha Yang
accepted-commit_now Details | Review
urisourcebin: Clear EOS state with stream-start/flush-stop event (2.63 KB, patch)
2017-01-09 10:52 UTC, Seungha Yang
none Details | Review
urisourcebin: Never push actual EOS event to slot (6.79 KB, patch)
2017-01-09 10:58 UTC, Jan Schmidt
none Details | Review
urisourcebin: Clear EOS state with stream-start/flush-stop event (2.63 KB, patch)
2017-01-09 10:58 UTC, Jan Schmidt
none Details | Review
urisourcebin: Never push actual EOS event to slot (6.78 KB, patch)
2017-01-09 11:42 UTC, Jan Schmidt
committed Details | Review
urisourcebin: Clear EOS state with stream-start/flush-stop event (2.63 KB, patch)
2017-01-09 11:42 UTC, Jan Schmidt
committed Details | Review

Description Seungha Yang 2017-01-08 13:13:00 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.
Comment 1 Seungha Yang 2017-01-08 13:14:13 UTC
Created attachment 343118 [details] [review]
urisourcebin: Never push actual EOS event to slot
Comment 2 Seungha Yang 2017-01-08 13:14:41 UTC
Created attachment 343119 [details] [review]
urisourcebin: Clear EOS state with stream-start/flush event

EOS state should cleared with stream-start of flush
Comment 3 Seungha Yang 2017-01-08 23:34:17 UTC
Created attachment 343130 [details] [review]
urisourcebin: Never push actual EOS event to slot
Comment 4 Jan Schmidt 2017-01-09 10:15:13 UTC
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.
Comment 5 Jan Schmidt 2017-01-09 10:18:54 UTC
Review of attachment 343130 [details] [review]:

Looks good, thanks! I will commit this and fix the commit message in the other patch shortly.
Comment 6 Jan Schmidt 2017-01-09 10:47:35 UTC
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?
Comment 7 Seungha Yang 2017-01-09 10:52:13 UTC
Created attachment 343140 [details] [review]
urisourcebin: Clear EOS state with stream-start/flush-stop event
Comment 8 Seungha Yang 2017-01-09 10:53:01 UTC
(In reply to Jan Schmidt from comment #4)
> Review of attachment 343119 [details] [review] [review]:

Typo fix done :)
Comment 9 Jan Schmidt 2017-01-09 10:58:25 UTC
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.
Comment 10 Jan Schmidt 2017-01-09 10:58:31 UTC
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
Comment 11 Jan Schmidt 2017-01-09 10:59:27 UTC
I updated the commit messages. I'd still like to make the description in the first commit clearer.
Comment 12 Seungha Yang 2017-01-09 11:05:40 UTC
(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.
Comment 13 Seungha Yang 2017-01-09 11:13:29 UTC
(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.
Comment 14 Jan Schmidt 2017-01-09 11:26:52 UTC
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.
Comment 15 Tim-Philipp Müller 2017-01-09 11:32:43 UTC
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.
Comment 16 Jan Schmidt 2017-01-09 11:42:03 UTC
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.
Comment 17 Jan Schmidt 2017-01-09 11:42:10 UTC
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
Comment 18 Jan Schmidt 2017-01-09 11:43:16 UTC
OK, I updated the description. Does that match what's happening Seungha?
Comment 19 Seungha Yang 2017-01-09 11:47:52 UTC
(In reply to Jan Schmidt from comment #18)
> OK, I updated the description. Does that match what's happening Seungha?

It's perfect!! :)
Comment 20 Jan Schmidt 2017-01-09 11:54:49 UTC
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
Comment 21 Arun Raghavan 2017-01-11 11:36:37 UTC
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.
Comment 22 Jan Schmidt 2017-01-11 11:41:50 UTC
(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.