[Bug 785951] New: urisourcebin/decodebin3: Don't use custom EOS events

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785951

Bug ID: 785951
Summary: urisourcebin/decodebin3: Don't use custom EOS events
Classification: Platform
Product: GStreamer
Version: git master
OS: Linux
Status: NEW
Severity: normal
Priority: Normal
Component: gst-plugins-base
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@bilboed.com
QA Contact: gstreamer-***@lists.freedesktop.org
GNOME version: ---

(Note: reproducible with seek_with_stop validate scenarios and playbin3)

urisourcebin and decodebin3 make usage of GST_EVENT_CUSTOM_DOWNSTREAM events
replacing the actual GST_EVENT_EOS to decide later on whether the EOS event
should be forwarded or not.

The problem with this is that queue/multiqueue/queue2 will drop any
non-{EOS|SEGMENT} events if the downstream flow return is GST_FLOW_EOS.

This results in hangs because:
1) upstream (say adaptivedemux) pushes GST_EVENT_EOS once it's done pushing
buffers (of which there is slightly too much, causing a downstream decoder/sink
to return GST_FLOW_EOS).
2) urisourcebin/decodebin3 converts that EOS to a GST_EVENT_CUSTOM_DOWNSTREAM
and pushes it through queue2 and multiqueue elements
3) => Those custom events get dropped because GST_FLOW_EOS was previously
returned
4) => Actual EOS events never reaches sinks
5) => hang

I am failing to remember the reason why we used custom downstream events
instead of actual EOS.

The ideal way forward would be to just use regular EOS events to which we add a
custom field in the event gststructure (gst_structure_set(eos_event_structure,
"urisourcebin", G_TYPE_BOOLEAN, TRUE, NULL).

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785951

--- Comment #1 from Edward Hervey <***@bilboed.com> ---
The reason we don't use a regular EOS event but instead a custom one was
introduced partly by the following commit:

commit c10e7c5011e5149712bf3c7c9d9bb8ee765c4ac6
Author: Seungha 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

I did some testing to try to replace it with actual EOS, but then the problem
becomes dealing with EOS propagation being delayed (it gets very messy).

The bottom line question is:
* Why do we need custom EOS events in the first place and ss there any reason
we would want to delay EOS propagation ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785951

Edward Hervey <***@bilboed.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@bilboed.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785951

--- Comment #2 from Edward Hervey <***@bilboed.com> ---
Created attachment 357276
--> https://bugzilla.gnome.org/attachment.cgi?id=357276&action=edit
decodebin3/urisourcebin: Switch to actual EOS events internally

Use the intended sequence for re-using elements:
* EOS
* STREAM_START if element is to be re-used

This avoids having elements (such as queue/multiqueue/queue2) not
properly resetting themselves.

When delaying EOS propagation (because we want to wait until all
streams of a group are done for example), we re-trigger them by
first sending the cached STREAM_START and then EOS (which will
cause elements to re-set themselves if needed and accept new
buffers/events).

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785951

Edward Hervey <***@bilboed.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Depends on| |786056

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785951
Bug 785951 depends on bug 786056, which changed state.

Bug 786056 Summary: queue/queue2:: Allow re-usability after EOS
https://bugzilla.gnome.org/show_bug.cgi?id=786056

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785951

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #357276|none |needs-work
status| |

--- Comment #3 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 357276:
--> (https://bugzilla.gnome.org/review?bug=785951&attachment=357276)

Looks good to me except for two minor problems

::: gst/playback/gstdecodebin3-parse.c
@@ +272,3 @@
+ s = gst_event_writable_structure (event);
+ gst_structure_set (s, "decodebin3-custom-eos", G_TYPE_BOOLEAN,
TRUE,
+ NULL);

You probably want to take over the seqnum of the input event

::: gst/playback/gsturisourcebin.c
@@ +1444,3 @@
+ s = gst_event_writable_structure (event);
+ gst_structure_set (s, "urisourcebin-custom-eos", G_TYPE_BOOLEAN, TRUE,
+ NULL);

And here

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785951

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #357276|needs-work |committed
status| |

--- Comment #4 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 357276:
--> (https://bugzilla.gnome.org/review?bug=785951&attachment=357276)

Fixed up and merged, thanks!

::: gst/playback/gsturisourcebin.c
@@ +1444,3 @@
+ s = gst_event_writable_structure (event);
+ gst_structure_set (s, "urisourcebin-custom-eos", G_TYPE_BOOLEAN, TRUE,
+ NULL);

Nevermind for this one

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785951

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
CC| |***@coaxion.net
Resolution|--- |FIXED

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785951

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|git master |1.13.1

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785951

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|1.13.1 |1.12.3