GNOME Bugzilla – Bug 720805
pad: stop running tasks before deactivation requests?
Last modified: 2014-01-03 04:49:02 UTC
Hi, bug #720584 exposed a bug that I believe could rather lay in GstPad. We have determined that when the user hits Ctrl+C on the terminal, we are changing states from PAUSED to READY. This causes the GstVideoDecoder's parent change_state hook to be called, before subclasses have a chance to stop() anything. This subsequently causes the pad to get deactivated. In the chain of events, this causes the pad deactivation logic to acquire the stream lock. If we have task running around, the stream lock is never released until that task is paused. Ways to fix this issue: 1. Fix all GstVideoDecoder-based elements that use a srcpad task to make sure that task gets paused when we change from PAUSED to READY. This either means (i) hook up GstElement::change_state() hook and stop running tasks in there, or (ii) track whether the pad is actually still alive in the task callback function and pause itself otherwise. 2. Find a way in GstVideoDecoder to notify subclasses earlier that we are going to change states and that stop() hook may need to be called before change_state() is called. 3. Make sure GstPad stops any running task itself when it is getting deactivated. I'd much prefer solution (3) as, in the general case, it doesn't seem completely right to me to still have running tasks around after a pad got deactivated anyway. Or am I failing to see the use-case for that? Thanks.
I'd prefer option 2 :-) Regarding to 1 and 3, sometimes a user task may blocked there waiting for a signal(from user). If GstVideoDeocder or GstPad force to gst_pad_stop_task, they need make a hook to notify user to wakeup the task first then stop the task. and if there's hook to notify, there's no need for GstVideoDecoder of GstPad stop the task. so I prefer option 2 to make GstVideoDocoder hook stop before parent->GstElement::change_stat().
1) is what you're supposed to do and what e.g. gst-omx and the androidmedia plugin are doing. Before chaining up to the parent class' state change function you must make sure that all pads are unlocked. 3) can't be implemented because pads can't stop their tasks themselves, the code running inside the tasks is the only one who can unlock and stop 2) is not a solution because it would break ABI. Current subclasses of GstVideoDecoder assume that all data flow has stopped already when stop() is called.
(In reply to comment #2) > 1) is what you're supposed to do and what e.g. gst-omx and the androidmedia > plugin are doing. Before chaining up to the parent class' state change function > you must make sure that all pads are unlocked. Agree, actually this was my first patch going to fix this issue. > 3) can't be implemented because pads can't stop their tasks themselves, the > code running inside the tasks is the only one who can unlock and stop > > 2) is not a solution because it would break ABI. Current subclasses of > GstVideoDecoder assume that all data flow has stopped already when stop() is > called. If any hook can be added in GstVideoDecoder before change_state should be better. else subclass has to hook change_state()
(In reply to comment #3) > If any hook can be added in GstVideoDecoder before change_state should be > better. else subclass has to hook change_state() Something like basesink's unlock/unlock_stop maybe. However that will require you to hook into that instead of change_state(), not sure if that's much easier ;)
(In reply to comment #4) > (In reply to comment #3) > > > If any hook can be added in GstVideoDecoder before change_state should be > > better. else subclass has to hook change_state() > > Something like basesink's unlock/unlock_stop maybe. However that will require > you to hook into that instead of change_state(), not sure if that's much easier > ;) Not sure. there's normally a base lock between unlock and unlock_stop in basesink(PREROLL_LOCK) or basesrc(LIVE_LOCK). unlock is used to break blocks of hook of render or fill/create. It's a little different from our case since our thread was created by subclass not by base, and the dead-lock of mutex is srcpad->stream_lock not videodecoder/encoder's lock.
unlock/unlock_stop in those are usually used to unblock anything created by the subclass, e.g. a blocking read on a socket.
(In reply to comment #6) > unlock/unlock_stop in those are usually used to unblock anything created by the > subclass, e.g. a blocking read on a socket. This also depends on where would GstVideoDecoder/Encoder call unlock/unlock_stop. Regarding to our case of subclass gst_pad_start_task(srcpad), we only hope EOS or PAUSE to READY to call the hook of unlock which would make subclass to gst_pad_stop_task(srcpad).
It would be called for FLUSH_START and PAUSED_TO_READY, not EOS.
if in FLUSH_START and subclass hook unlock to stop/pause the srcpad->task. where to start the task again?
When you got the next input buffer for example
Make sense. Could you comment here(or link to new bug) if any update of unlock/unlock_stop of GstVideoDecoder/Encoder? Thanks:)