GNOME Bugzilla – Bug 772115
tee: adding inactive pad to running element
Last modified: 2018-11-03 12:36:31 UTC
Created attachment 336431 [details] Test that reproduces the problem I have found the next problem in the tee running the attached program: GStreamer-WARNING **: adding inactive pad 'src_46' to running element 'tee0', you need to use gst_pad_set_active(pad,TRUE) before adding it.
Created attachment 336432 [details] [review] tee: activate src pad before adding it
Comment on attachment 336432 [details] [review] tee: activate src pad before adding it It should only be activated if the tee is running
@slomo, what happens if it is activated and the tee is not running? Anyway, this is done by gstelement [1] but with a previous warning. Refs [1] https://github.com/Kurento/gstreamer/blob/0fb3a083ce04551fbdba7a94f0ac5612515bda67/gst/gstelement.c#L680
Then you have activated the pad while the tee is not running :) That's not good. The pads should only be active in >= PAUSED
Created attachment 336436 [details] [review] element: do not consider a warning adding an inactive pad If I am not wrong, this should be the solution because the state of the element and if the pad is active or not must be checked in the same critical section to avoid race condition problems.
Comment on attachment 336436 [details] [review] element: do not consider a warning adding an inactive pad No, it's wrong to do that and the element that is adding inactive pads while in >= PAUSED is just wrong
I do not know another solution without having race condition problems. @slomo, could you please propose a solution?
I don't have time to think about that right now, but basically tee should know what state it is in and act accordingly.
@slomo, don't worry I understand it ;) I suppose what you mean but there are problems. Anyway, I uploading a patch to discuss over it.
Created attachment 336455 [details] [review] tee: activate src pads if element is running
Review of attachment 336455 [details] [review]: ::: plugins/elements/gsttee.c @@ -414,0 +414,7 @@ + GST_OBJECT_LOCK (tee); + if ((GST_STATE (tee) > GST_STATE_READY || + GST_STATE_NEXT (tee) == GST_STATE_PAUSED)) { ... 4 more ... Here there is a race condition problem when the lock is unlocked: If the element changes of state between - setting the proper active flag to the src pad - and adding the pad to the element. we will have the same problem that doing nothing about the active flag. This cannot be fixed here because the gst_element_add_pad also locks the element and we will have a deadlock.
I believe there is a reason why we have a recursive state lock in GstElement::state_lock.
(In reply to Nicolas Dufresne (stormer) from comment #12) > I believe there is a reason why we have a recursive state lock in > GstElement::state_lock. Just that it isn't used consistently (or even according to the comments in the code) right now. In theory using the state lock here would solve it, but in practice it doesn't (and making the state lock used like that causes deadlocks all over the place due to other issues).
We might have made it worst lately indeed.
(In reply to Nicolas Dufresne (stormer) from comment #14) > We might have made it worst lately indeed. How do you mean? It's like this at least since all of 1.0, should've been like this for quite a while in 0.10 too
From my point of view in this specific case we are dealing with an encapsulation problem. I mean that the state (active/not active) of the pad (in relation with the state of the element) must be handled by GstElement class as it already does but without consider a warning if the element is ready and the pad is not active. That is don done in attachment 336436 [details] [review]. I think that this is the best and the easy solution and matches with the idea that the state management is properly encapsulated in GstElement.
-- 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/190.