GNOME Bugzilla – Bug 750098
New element to convert GST_FLOW_ERROR into GST_FLOW_OK
Last modified: 2015-06-23 08:28:41 UTC
Created attachment 304240 [details] [review] The patch. Can be used to fix misbehaving sinks. It will pass through all buffers until it encounters GST_FLOW_ERROR. At that point it will unref the buffers and return GST_FLOW_OK - until the next READY_TO_PAUSED or FLUSH_STOP.
If there are two sinks and one is misbehaving and needs to be removed, the whole pipeline will stall. This element can be placed before suspicious sinks to keep the pipeline up and running until they can be safely removed.
Thanks for the patch, this would've been useful for me already a few times and I worked around it with pad probes. It feels like this should be more configurable though, like - a property to enable/disable error ignorign - ignore NOT_LINKED, NOT_NEGOTIATED or others too It should probably also just go into the debugutils plugin in gst-plugins-bad for now, no need for another micro plugin :)
Review of attachment 304240 [details] [review]: ::: gst/errorignore/gsterrorignore.c @@ +41,3 @@ +#define GST_PACKAGE_ORIGIN "http://toolsonair.com" +#define GST_LICENSE "LGPL" +#endif These #defines should go away, They are properly #defined by the gst-plugins-bad build system
Wouldn't it be useful to convert GST_FLOW_ERROR to GST_FLOW_NOT_LINKED? If you have multiple sinks and one fails it could be ok but if all fails is there any reason to keep the pipeline going? Treating the misbehaving sinks as 'not-linked' seems to make this possible.
Created attachment 305151 [details] [review] Reviewed patch.
Thanks for the feedback, moved to debugutils, also now I'm converting ERROR and NOT_NEGOTIATED into NOT_LINKED by default (configurable) :) Please see the new patch attached!
Comment on attachment 305151 [details] [review] Reviewed patch. Looks good to me, I don't like the property names but I also don't know better ones :) Thiago?
Review of attachment 305151 [details] [review]: Looks good. One thing I noticed is that it likely needs to reset keep_pushing when a reconfigure event is received. Maybe check with gst_pad_check_reconfigure() or add a source event handler to catch it. Just some minor suggestions below, otherwise looks good. ::: gst/debugutils/gsterrorignore.c @@ +23,3 @@ + * + * Passes through all packets, but converts GST_FLOW_ERROR into GST_FLOW_OK + * (e.g. to allow other sinks to keep playing if one is misbehaving) I think the documentation got outdated after the latest changes. Might be worth mentioning the default values that are ignored. @@ +120,3 @@ + + g_object_class_install_property (object_class, PROP_CONVERT_TO, + g_param_spec_enum ("convert-to", "GstFlowReturn to convert to", convert-to looks not optimal to me but I don't have good ideas as well. 'converted-return'? 'ignored-return'? But don't let this block you from pushing. I'd ask around in IRC for suggestions, if no better options arise then I'd merge it as is.
Created attachment 305541 [details] [review] Reviewed(2) patch. Thanks for the review! Is this what you meant with reconfigure? Does it look OK now?
Review of attachment 305541 [details] [review]: ::: gst/debugutils/gsterrorignore.c @@ +206,3 @@ + +static gboolean +gst_error_ignore_sink_event (GstPad * pad, GstObject * parent, GstEvent * event) RECONFIGURE is an upstream event, so it should be in a src_event handler. It will be useful here because it can be sent, for example, when a new element is linked downstream so errorignore can try pushing data again.
Instead of catching the event, just check gst_pad_check_reconfigure() in the chain function before handling any buffer. And if it's returning TRUE, reset keep_pushing. (But also call gst_pad_check_reconfigure() if keep_pushing is TRUE, always call it)
Created attachment 305813 [details] [review] Reviewed(3) patch Thanks, added another version of the patch :)
Review of attachment 305813 [details] [review]: Looks good to me, no more comments.
commit 9664d1a6b1796491048eb579e919ab592c1ec2f6 Author: Vivia Nikolaidou <vivia@toolsonair.com> Date: Fri May 29 14:27:24 2015 +0300 error-ignore: New element to convert some GstFlowReturn types into others Can be used to fix misbehaving sinks. It will pass through all buffers until it encounters GST_FLOW_ERROR or GST_FLOW_NOT_NEGOTIATED (configurable). At that point it will unref the buffers and return GST_FLOW_NOT_LINKED (configurable) - until the next READY_TO_PAUSED or FLUSH_STOP. https://bugzilla.gnome.org/show_bug.cgi?id=750098