GNOME Bugzilla – Bug 722767
element: add function to sync element state with target state of the parent
Last modified: 2018-11-03 12:19:50 UTC
I suggest adding a function to set the element state to the target state of its parent. This is useful for avoiding race condition problems in dynamic pipelines. gboolean gst_element_sync_target_state_with_parent (GstElement * element) { GstElement *parent; GstState target; GstStateChangeReturn ret; g_return_val_if_fail (GST_IS_ELEMENT (element), FALSE); parent = GST_ELEMENT_CAST (gst_element_get_parent (element)); if (element == NULL) { GST_DEBUG_OBJECT (element, "element has no parent"); return FALSE; } GST_OBJECT_LOCK (parent); target = GST_STATE_TARGET (parent); GST_OBJECT_UNLOCK (parent); GST_DEBUG_OBJECT (element, "setting parent (%s) target state %s", GST_ELEMENT_NAME (parent), gst_element_state_get_name (target)); gst_object_unref (parent); ret = gst_element_set_state (element, target); if (ret == GST_STATE_CHANGE_FAILURE) { GST_DEBUG_OBJECT (element, "setting target state failed (%s)", gst_element_state_change_return_get_name (ret)); return FALSE; } return TRUE; }
parent = GST_ELEMENT_CAST (gst_element_get_parent (element)); if (element == NULL) { GST_DEBUG_OBJECT (element, "element has no parent"); return FALSE; } This should be: if (parent == NULL) { ... In any way, please post this as a patch, so that we can properly review it :)
Created attachment 267402 [details] [review] gstelement: add function to synchronize with parent target state OK, I attach the patch in this comment. Also, I think that we can do a common code for this function and for the already existing gst_element_sync_state_with_parent function.
Review of attachment 267402 [details] [review]: ::: gst/gstelement.c @@ +2016,3 @@ + * Returns: TRUE, if the element's state could be synced to the parent's state. + * + * MT safe. Please add a "Since: 1.4" line here @@ +2044,3 @@ + gst_object_unref (parent); + + ret = gst_element_set_state (element, target); Shouldn't we sync the state with the parent's state, but at the same time set the target to the parent's target state instead? Would be another option... but I guess what you do here right now makes most sense actually.
Created attachment 268655 [details] [review] Sync state of a children with its parent target state. I have been thinking that a better solution is change the connotation of gst_element_sync_state_with_parent to avoid the users have concurrence problems. I think that the users want use this function to sync the state of a element with the target state of its parent, not the current or the pending one, because these are variable.
The problem I see with this is that it can cause the child element to run ahead of the parent bin, e.g. 1) set_state(bin, PLAYING) 2) bin goes to READY 3) child is added, sync_state_with_parent() => set_state(child, PLAYING) 4) child goes to PLAYING directly ... 5) bin goes to PAUSED Not sure if that is a problem. It seems weird though. And we need to check the code in GstBin to see if it could cause the child element in the above example to go from PLAYING to PAUSED, and then later back to PLAYING.
In other hand, with the current implementation we have the next problem with a sinkelement if it does not receive the preroll: 1) set_state(bin, PLAYING) 2) bin goes to PLAYING 3) sinkelement (child_0) is added, sync_state_with_parent() => set_state(child_0, PLAYING) 4) child_0 goes to READY (blocked waiting for preroll), so parent syncs with its child and goes to READY. 5) another element (child_1) is added, sync_state_with_parent() => set_state(child_1, READY) 6) child_1 goes to READY ... If the sink element does not receive a preroll the other elements in the bin are blocked, and if the generation of buffers (so the preroll) depends on a expected state never reached, the pipe has a "deadlock".
I just had a case where this also caused a pipeline to never go to PLAYING.
Hello Sebastian, could you explain your case, please?
This would be the simplified version: Live pipeline in PLAYING with a tee. Two sinks (async=true) are added, synced state with parent. First sink gets set to PLAYING by sync_state(), pipeline goes to PAUSED (pending=PAUSED) because it loses state because the sink is going async to PAUSED. Second sink gets set to PAUSED by sync_state() (because pipeline is pending=PAUSED). Both sinks get a buffer, nothing ever sets the second sink to PLAYING (it still has target state PAUSED).
Hi, sorry for the delay. Yes, this is another case that would be solve with my proposal because it use the target state of the parent to set the state of the children, isn't it? Moreover, I have to say that I am using this function in some cases and by the moment I have not got any problem: https://github.com/Kurento/kms-core/blob/develop/src/gst-plugins/commons/kmsutils.c#L70
Yeah, it's probably not the worst idea. But then people are probably already expecting the existing function to do exactly that :) OTOH it might be better to just deprecate the function and let people call gst_element_set_state() directly. That's more explicit then and no hidden surprises.
Any other opinions here? Should we just change sync_state() to use the target state?
Some related discussion on IRC <wtay_> slomo, target state seems weird, you could set elements to playing already when the pipeline is still prerolling <slomo> wtay_: yes, but is that a problem? without you have the same problem the other way around, just that it there can cause things to never go to PLAYING ;) <wtay_> I don't know <slomo> so maybe a new function to set the target state instead? but otoh that will probably confuse people as they'll never know which one to use <slomo> wtay_: a problem could possibly be if the child element expects to go to PLAYING only from the surrounding pipeline and expects to have a clock and proper base time set at that point <slomo> wtay_: but then this is going to break in other situations too currently... when people explicitly set the state to PLAYING instead of syncing with the parent state for example. which would be the recommended way of doing things considering the never-go-to-PLAYING bug with sync_state_with_parent()
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/50.