GNOME Bugzilla – Bug 339326
gstutils.c pad link helper functions don't activate pads properly
Last modified: 2010-05-21 15:26:05 UTC
newly created pads are not set to the FLUSHING state. This means that a newly created element has pads that can accept data, even in the NULL or READY state. Setting the pads to FLUSHING initially causes regressions in demuxers that add pads dynamically in PAUSED/PLAYING but forget to activate the pad, so they need to be fixed first.
Example where it causes issues: * An element creates a New pad (not activated, flushing) in READY. * Someone does a gst_pad_set_blocking(newpad, TRUE, callback) * The element's state is set to lock (gst_element_set_locked_state (TRUE)) * we switch the element's state to PAUSED, which puts it to PAUSED without calling the state_change function (and the pad activation). * We unblock the state (gst_element_set_locked_state (FALSE)) * We change the state of the element to READY ** The state_change function tries to deactivate the pad, but since it's already not-activated, it won't call the function that unsets the FLUSHING flag ** The element send an EOS ** The pad blocks *** LOCKS in state_change
Created attachment 74154 [details] [review] patch this patch sets the pad to flushing initially and gives a warning when a flushing pad is added to a running element. this should help diagnose offenders.
* gst/gstelement.c: (gst_element_add_pad): * gst/gstghostpad.c: (gst_ghost_pad_new_full): * gst/gstpad.c: (gst_pad_init): Set pads to FLUSHING when they are created. Check, warn and fix when a demuxer adds an inactive pad to itself when running. Fixes #339326.
You missed a case, that is now impossible to do correctly: * Running element adds a SINK pad to itself (e.g: a Muxer with request pads, or multiqueue). * By the new logic, an element must do gst_pad_set_active before adding the pad. * Activating the pad causes the _activate handlers to be called, for stuff like activating pull mode when it can, but the pad isn't linked to anything and has no parent yet because it hasn't been added to the element. The same thing applies to SRC pads with activate handlers, but those aren't common. I think that instead of warning when a flushing pad is added to a running element, the pad should just be activated, because both add-to-element-then-activate or activate-then-add-to-element are both wrong - the activation needs to happen atomically when adding the pad.
Its very annoying that you change the API/ABI sematics like that in the middle of a stable series. If I set the pads to active before adding them, it breaks some of my elements on 0.10.9, so I need to maintain two separate versions now. Please fix .
What exactly does the breakage look like with 0.10.9? So far we haven't found any situations where the plugins can't work properly with GStreamer both before and after the change - it's either been a bug in the plugin, or an obscure case that can be easily done another way. Either way, there's been several releases of GStreamer with the change already made, so changing it back won't help you.
I have one example with one of our plugins, rtpbin. Source is at: http://projects.collabora.co.uk/darcs/farsight/gst-plugins-farsight/ext/jrtp/ The patch that makes it work with recent gstreamer but breaks with 0.10.9 is http://projects.collabora.co.uk/~tester/gst-plugins-farsight-set-active.patch The second example is in farsight itself, which uses a regular bin and ghostpads. Source code is at http://projects.collabora.co.uk/darcs/farsight/farsight/plugins/rtp/rtpstream.c The patch is at: http://projects.collabora.co.uk/~tester/farsight-set-active.patch And obviously, without the patches, it doesn't work with the newer gst.
I think I found the problem... It has to do with ghostpads and bins. One case is, I have a bin (actually a rtpbin), which is either in the NULL or PAUSED state to which I add an element and then create a sink ghostpad from that element's sinkpad. If I set the ghostpad active before adding it to the bin, it breaks with 0.10.9, if I don't 0.10.12 complains. The second case is similar, I have a bin which may be in the PAUSED state, to which I add a new element (which is in the NULL state), then create a ghostpad, add it to the bin and use it to link it to an element outside the bin. If I set the ghostpad active before adding it to the bin, data doesn't flow in 0.10.9, if I set it active after adding to the bin, 0.10.12 outputs a warning (this is from farsight code). In both cases, I can aleviate the problem by setting the ghostpad active only if the pad it refers to is active, but that still outputs a warning on 0.10.12.
> In both cases, I can aleviate the problem by setting the ghostpad active only > if the pad it refers to is active, but that still outputs a warning on 0.10.12. As a work-around, you could also check the core version at runtime with gst_version() and skip the activation for earlier cores.
I would also like to add that the helper functions in gstutils.c do not activate ghost pads before adding them to the bin, therefore, it causes warnings when using functions such as gst_element_link_filtered().
However annoying the semantic change may have been, we've had g_warnings for this in core for ages now, so I think we should just go and fix this in whatever way is best. Is any of this still relevant?
Wim or someone else, what shall be done about this bug now?
Ping?
Maybe Olivier can tell whether this is still an issue. If not, I'm all for closing as OBSOLETE.
I guess this works for everyone now.. So lets close it