GNOME Bugzilla – Bug 719437
ghostpad: Clearing ghostpad sticky events when switching targets causes regressions
Last modified: 2013-11-29 18:06:50 UTC
Reproducing is easy: make gnl/simple.forever show a warning quite fast, namely : Unexpected critical/warning: gstpad.c:3918:gst_pad_push_data:<test_composition:src> Got data flow before stream-start event Reverting 8162a583a4dd68582bf186e2e47a8f0d68fa1980 makes forever pass again. Related bug : https://bugzilla.gnome.org/show_bug.cgi?id=707621
More precisely : GST_CHECKS=test_one_after_other make gnl/simple.check is enough for reproducing.
Problem here is that gnlcomposition drops all stream-start events that it doesn't expect on it's src ghostpads... and when the target of that ghostpad is changed by gnlcomposition, the previously accepted stream-start event is cleared now. A new one is resent before the next data flow but gnlcomposition does not expect it, so just drops it. So there's 3 problems here: a) gnlcomposition only forwards the stream-start event of the first stream (?). I think instead of doing such weird things it should just create its own stream-start event and drop all coming from upstream b) gnlcomposition's stream-start dropping logic is racy, because most of the time it actually passes on the stream-start event after it is cleared c) clearing the ghostpads sticky events when changing targets, and copying them from the new target is maybe not a good idea. The owner of the ghostpad could've caught sticky events coming from upstream and modified them or dropped some of them, and by now just copying them without giving the owner of the ghostpad to do anything about that could break code. So... what to do? Revert that change, reopen bug #707621 and make Christian and others unhappy? Independent of that the logic inside gnlcomposition needs to be fixed though.
> > So... what to do? Revert that change, reopen bug #707621 and make Christian and > others unhappy? I would revert the change and make decodebin2 copy the sticky event explicitly after setting the target. This would give control back to the one managing the ghostpads.
commit fe36be1c03d71008bccadf1ff73ae336631b430b Author: Wim Taymans <wtaymans@redhat.com> Date: Fri Nov 29 17:02:41 2013 +0100 Revert "ghostpad: copy sticky events to SRC ghostpads" This reverts commit 8162a583a4dd68582bf186e2e47a8f0d68fa1980. Automatically copying the sticky events makes it impossible for apps and elements to filter the events with event probes. This causes regressions (See #719437). The best option is to let the app/element copy and filter the events themselves after the ghostpad target is set.
Maybe it would be better to make a gst_ghost_pad_set_target_full() with an option to copy the sticky events or maybe with a callback for each event where you can filter things.
Why not copy them using gst_pad_push_event() instead of gst_pad_store_event()? That would trigger pad probes.
Because it would send serialized events downstream (and call pad probes and everything) from a random thread (the one calling gst_ghost_pad_set_target()) instead of the actual streaming thread. Could easily cause deadlocks in the application, and nobody would expect gst_ghost_pad_set_target() to block