GNOME Bugzilla – Bug 626784
element: link_many might assert elements are in paused or playing
Last modified: 2010-08-19 12:58:41 UTC
When using gst_element_link_many on elements that are on different bins (they are inside the same pipeline, but one of them is in a bin), the ghostpad automatic creation might cause a warning when the elements are in PAUSED/PLAYING state. The newly created ghostpad must be set to active before being added. The other solution is to mention on the docs that this function shouldn't be used after the pipeline is running ;)
Created attachment 167776 [details] [review] element: link_many should activate pads if needed gst_element_link_many does some magic and creates ghostpads if needed, but it didn't set the newly created ghostpad to active if needed. This patch fixes it.
Makes sense but the state might've changed already between the get_state() and the comparision and pad activation. But that's probably not really important and unlikely to cause problems in reality.
Can/should we use the state lock in this part?
Created attachment 168227 [details] [review] element: link_many should activate pads if needed Updated patch using locking.
Holding the state lock while adding pads, calling gst_element_get_state() and activating pads feels a bit dangerous IMHO but I might be wrong. Better ask Wim :) Might be better to hold the object lock and duplicate the timeout==0 part of gst_element_get_state() and releasing the lock before activating/adding pads.
The state lock is used in 3 functions in gstelement: 1) send_event 2) set_state 3) finalize Don't see exactly how adding a pad can conflict with those. But I might be wrong too :) Looking on the other way, the add_pad from gstelement doesn't hold the lock while testing if the pad is flushing, comparing with the element state and then activating it if needed (with a proper warning). I guess we're assuming that the user is not supposed to be adding pads and changing states at the same time. So we might leave this patch without the lock at all. In any case, let's have Wim's opinion.
The only purpose of the STATE_LOCK is to make sure that no threads can execute _set_state() at the same time. The most important use is so that the async state changes in gstbin execute only when the _set_state() is done. As a side effect, it can also be used to make sure that no state changes can be performed by the application (surround some code with the STATE_LOCK). The lock is recursive too and is supposed to be taken before any other lock. Without the STATE_LOCK, it is possible that a set_state() is executed right between getting the element state (old state) and adding the pad, leaving the pad in the wrong state. Another possible solution is to check if the state cookie changed between getting the state and after adding the pad. If it did, we need to redo the pad activation (although core might have complained already about adding inactive pads to running elements). So it looks like this patch (with the STATE_LOCK) would not cause any problems that I can think of. I would leave the lock. On a side note, I'm not entirely sure that we don't have other situations where we should but didn't take the STATE_LOCK..
Ok, then let's get this in as is :)
Thanks for the reviews. Pushed the fix. commit 706f0f657b0ee85c5ca603b1300234cb980fa619 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Thu Aug 12 20:23:45 2010 -0300 element: link_many should activate pads if needed gst_element_link_many does some magic and creates ghostpads if needed, but it didn't set the newly created ghostpad to active if needed. This patch fixes it. https://bugzilla.gnome.org/show_bug.cgi?id=626784