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 719437 - ghostpad: Clearing ghostpad sticky events when switching targets causes regressions
ghostpad: Clearing ghostpad sticky events when switching targets causes regre...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: High blocker
: 1.2.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-11-27 17:23 UTC by Mathieu Duponchelle
Modified: 2013-11-29 18:06 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Mathieu Duponchelle 2013-11-27 17:23:45 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
Comment 1 Mathieu Duponchelle 2013-11-27 17:29:54 UTC
More precisely :

GST_CHECKS=test_one_after_other make gnl/simple.check is enough for reproducing.
Comment 2 Sebastian Dröge (slomo) 2013-11-29 11:46:24 UTC
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.
Comment 3 Wim Taymans 2013-11-29 15:17:43 UTC
> 
> 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.
Comment 4 Wim Taymans 2013-11-29 16:30:49 UTC
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.
Comment 5 Wim Taymans 2013-11-29 16:32:08 UTC
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.
Comment 6 Olivier Crête 2013-11-29 16:37:01 UTC
Why not copy them using gst_pad_push_event() instead of gst_pad_store_event()? That would trigger pad probes.
Comment 7 Sebastian Dröge (slomo) 2013-11-29 18:06:50 UTC
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