GNOME Bugzilla – Bug 757821
New GstPadEventFunction which returns a GstFlowReturn
Last modified: 2015-12-02 15:07:39 UTC
Currently pushing events on a pad returns a gboolean value. While on its own this is not problematic, it causes issues with sticky event propagation. The use-case is the following: * Two elements (element1 and element2) are linked * There are sticky events that need propagating from element1:src to element2:sink * element1 pushes a buffer (gst_pad_push()) * gst_pad_push calls check_sticky() before doing the actual push. Note that if check_sticky() returns something else than GST_FLOW_OK, that flowret is returned straight away. * check_sticky() calls push_sticky for all events ** push_sticky() calls gst_pad_push_event_unchecked () (which returns a GstFlowReturn) *** gst_pad_push_event_unchecked() will eventually call gst_pad_send_event_unchecked () (which returns a GstFlowReturn) on the peer pad. **** gst_pad_send_event_unchecked() will call the GstPad GstEventFunction (which returns a ==>gboolean<==). Based on that boolean return value, and the type of event, that function will return a certain GstFlowReturn. And this is where the problem lies ... ==> Some asynchronous elements, such as the various *queue elements, will return FALSE if downstream is flushing (i.e. the last buffer push returned GST_FLOW_FLUSHING). ==> core cannot see that it is returning FALSE because downstream is flushing ==> Depending on the type of event, it will transform that FALSE into GST_FLOW_NOT_NEGOTIATED or GST_FLOW_ERROR ==> That flow return trickles all the way upstream and we get a failure And in a more generic fashion, this is breaking the rationale that GstFlowReturn should be properly propagated upstream. In this case it is broken (GstFlowReturn from downstream of an element is not propagated to the upstream push, due to the sticky event step). I'm open to other proposals on how to fix this, but I'd say a simple way forward would be to create: * a new GstFlowReturn GstPadEventFullFunc(GstPad*, GstObject *, GstEvent*); * a new GstPad eventfullfunc * If the eventfullfunc is present, call that in gst_pad_send_event_unchecked() and use that GstFlowReturn directly, else do the standard code path Comments/Feedback welcome
In many cases we manage to workaround this recurrent issue by checking the FLUSHING flag on the pad after receiving FALSE return value. Here is some similar cases. Because gst_pad_has_current_caps() return boolean http://cgit.freedesktop.org/gstreamer/gstreamer/tree/libs/gst/base/gstbaseparse.c#n2461 Because gst_pad_check_reconfigure() return a boolean http://cgit.freedesktop.org/gstreamer/gstreamer/tree/libs/gst/base/gstbasesrc.c#n2695 Because gst_base_transform_reconfigure() return a boolean http://cgit.freedesktop.org/gstreamer/gstreamer/tree/libs/gst/base/gstbasetransform.c#n2119 Because gst_audio_decoder_negotiate_unlocked() returns a boolean (same in encoder, same for video) http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/audio/gstaudiodecoder.c#n1130 Those are not strictly the same, in the sense that it's not send_event() but mostly negotiation function. But they are the same in the sense that there is a boolean function that return FALSE due to a FLUSH happening at the same time. I'm just documenting how we survived those API errors so far. But this approach have no solution as simple as this if the flow you need to handle is NOT_LINKED.
While I think that in general it's a good idea, doing it like this will probably cause lots of weird problems... adding an element that does not use the new API could potentially break the complete pipeline around it in surprising ways without any direct indication of the cause. You should also talk with Wim, he looked into that before 1.6 and there was more about it than it being too much work. There were cases where it wasn't clear how the flow returns would be handled.
Sebastian, those concerns are still present. Which is why I am *not* proposing to make a gst_pad_{send|push}_event_full() public function. This would require a lot more thinking regarding how elements should handle that (i.e. potentially wait for 2.x to tackle that). I am just proposing to: * allow some elements to implement an event function that returns a GstFlowReturn * have core use it in gst_pad_send_event_unchecked() if present That will not modify the current behaviour. Nicolas, Yes, this is exactly what I'm proposing to solve ... but in core. All the examples you have mentioned are how we adapt the flow return (in the buffer push system) based on an event push and surrounding information (flushing state, etc..).
(In reply to Edward Hervey from comment #3) > I am just proposing to: > * allow some elements to implement an event function that returns a > GstFlowReturn > * have core use it in gst_pad_send_event_unchecked() if present > > That will not modify the current behaviour. That makes sense, yes
Created attachment 315657 [details] [review] WIP: Implement GstPadEventFullFunction Returns a GstFlowReturn, allows asynchronous elements to properly propagate flow returns
The proposed patch adds the GstPadEventFullFunction. Elements can decide to set that event function instead of the regular one. If present it will be used by gst_pad_send_event_unchecked() It also sets the "regular" eventfunc, which is a wrapper that will convert the GstFlowReturn to a gboolean. I've been using it for a few days, there are no regression in make check, and some racyness in gst-validate has disappeared on some files.
commit 56d4650789e113c2184ed955ed19b9457b41769a Author: Edward Hervey <bilboed@bilboed.com> Date: Thu Nov 12 17:15:37 2015 +0100 pad: Implement GstPadEventFullFunction API: GstPadEventFullFunction Returns a GstFlowReturn, allows asynchronous elements to properly propagate flow returns https://bugzilla.gnome.org/show_bug.cgi?id=757821