gstpad: Make pad (de)activation atomic

The following could happen previously:
* T1: calls gst_pad_set_active()
* T2: currently (de)activating it
* T1: gst_pad_set_active() returns, caller assumes that the pad has
  completed the requested (de)activation ... whereas it is not
  the case since the actual (de)activation in T2 might still be
  going on.

To ensure atomicity of pad (de)activation, we use a internal
variable (and cond) to ensure only one thread at a time goes through
the actual (de)activation block

https://bugzilla.gnome.org/show_bug.cgi?id=790431
This commit is contained in:
Edward Hervey 2017-11-16 10:47:46 +01:00 committed by Edward Hervey
parent 80262013ca
commit d915dd4b20

View file

@ -149,6 +149,11 @@ struct _GstPadPrivate
* call. Used to block any data flowing in the pad while the idle callback * call. Used to block any data flowing in the pad while the idle callback
* Doesn't finish its work */ * Doesn't finish its work */
gint idle_running; gint idle_running;
/* conditional and variable used to ensure pads only get (de)activated
* by a single thread at a time. Protected by the object lock */
GCond activation_cond;
gboolean in_activation;
}; };
typedef struct typedef struct
@ -417,6 +422,8 @@ gst_pad_init (GstPad * pad)
pad->priv->events = g_array_sized_new (FALSE, TRUE, sizeof (PadEvent), 16); pad->priv->events = g_array_sized_new (FALSE, TRUE, sizeof (PadEvent), 16);
pad->priv->events_cookie = 0; pad->priv->events_cookie = 0;
pad->priv->last_cookie = -1; pad->priv->last_cookie = -1;
g_cond_init (&pad->priv->activation_cond);
pad->ABI.abi.last_flowret = GST_FLOW_FLUSHING; pad->ABI.abi.last_flowret = GST_FLOW_FLUSHING;
} }
@ -762,6 +769,7 @@ gst_pad_finalize (GObject * object)
g_rec_mutex_clear (&pad->stream_rec_lock); g_rec_mutex_clear (&pad->stream_rec_lock);
g_cond_clear (&pad->block_cond); g_cond_clear (&pad->block_cond);
g_cond_clear (&pad->priv->activation_cond);
g_array_free (pad->priv->events, TRUE); g_array_free (pad->priv->events, TRUE);
G_OBJECT_CLASS (parent_class)->finalize (object); G_OBJECT_CLASS (parent_class)->finalize (object);
@ -966,12 +974,15 @@ pre_activate (GstPad * pad, GstPadMode new_mode)
switch (new_mode) { switch (new_mode) {
case GST_PAD_MODE_NONE: case GST_PAD_MODE_NONE:
GST_OBJECT_LOCK (pad); GST_OBJECT_LOCK (pad);
while (G_UNLIKELY (pad->priv->in_activation))
g_cond_wait (&pad->priv->activation_cond, GST_OBJECT_GET_LOCK (pad));
if (new_mode == GST_PAD_MODE (pad)) { if (new_mode == GST_PAD_MODE (pad)) {
GST_WARNING_OBJECT (pad, GST_WARNING_OBJECT (pad,
"Pad is already in the process of being deactivated"); "Pad is already in the process of being deactivated");
GST_OBJECT_UNLOCK (pad); GST_OBJECT_UNLOCK (pad);
return FALSE; return FALSE;
} }
pad->priv->in_activation = TRUE;
GST_DEBUG_OBJECT (pad, "setting PAD_MODE NONE, set flushing"); GST_DEBUG_OBJECT (pad, "setting PAD_MODE NONE, set flushing");
GST_PAD_SET_FLUSHING (pad); GST_PAD_SET_FLUSHING (pad);
pad->ABI.abi.last_flowret = GST_FLOW_FLUSHING; pad->ABI.abi.last_flowret = GST_FLOW_FLUSHING;
@ -983,12 +994,15 @@ pre_activate (GstPad * pad, GstPadMode new_mode)
case GST_PAD_MODE_PUSH: case GST_PAD_MODE_PUSH:
case GST_PAD_MODE_PULL: case GST_PAD_MODE_PULL:
GST_OBJECT_LOCK (pad); GST_OBJECT_LOCK (pad);
while (G_UNLIKELY (pad->priv->in_activation))
g_cond_wait (&pad->priv->activation_cond, GST_OBJECT_GET_LOCK (pad));
if (new_mode == GST_PAD_MODE (pad)) { if (new_mode == GST_PAD_MODE (pad)) {
GST_WARNING_OBJECT (pad, GST_WARNING_OBJECT (pad,
"Pad is already in the process of being activated"); "Pad is already in the process of being activated");
GST_OBJECT_UNLOCK (pad); GST_OBJECT_UNLOCK (pad);
return FALSE; return FALSE;
} }
pad->priv->in_activation = TRUE;
GST_DEBUG_OBJECT (pad, "setting pad into %s mode, unset flushing", GST_DEBUG_OBJECT (pad, "setting pad into %s mode, unset flushing",
gst_pad_mode_get_name (new_mode)); gst_pad_mode_get_name (new_mode));
GST_PAD_UNSET_FLUSHING (pad); GST_PAD_UNSET_FLUSHING (pad);
@ -1029,12 +1043,18 @@ post_activate (GstPad * pad, GstPadMode new_mode)
GST_PAD_STREAM_LOCK (pad); GST_PAD_STREAM_LOCK (pad);
GST_DEBUG_OBJECT (pad, "stopped streaming"); GST_DEBUG_OBJECT (pad, "stopped streaming");
GST_OBJECT_LOCK (pad); GST_OBJECT_LOCK (pad);
pad->priv->in_activation = FALSE;
g_cond_broadcast (&pad->priv->activation_cond);
remove_events (pad); remove_events (pad);
GST_OBJECT_UNLOCK (pad); GST_OBJECT_UNLOCK (pad);
GST_PAD_STREAM_UNLOCK (pad); GST_PAD_STREAM_UNLOCK (pad);
break; break;
case GST_PAD_MODE_PUSH: case GST_PAD_MODE_PUSH:
case GST_PAD_MODE_PULL: case GST_PAD_MODE_PULL:
GST_OBJECT_LOCK (pad);
pad->priv->in_activation = FALSE;
g_cond_broadcast (&pad->priv->activation_cond);
GST_OBJECT_UNLOCK (pad);
/* NOP */ /* NOP */
break; break;
} }
@ -1256,6 +1276,8 @@ failure:
active ? "activate" : "deactivate", gst_pad_mode_get_name (mode)); active ? "activate" : "deactivate", gst_pad_mode_get_name (mode));
GST_PAD_SET_FLUSHING (pad); GST_PAD_SET_FLUSHING (pad);
GST_PAD_MODE (pad) = old; GST_PAD_MODE (pad) = old;
pad->priv->in_activation = FALSE;
g_cond_broadcast (&pad->priv->activation_cond);
GST_OBJECT_UNLOCK (pad); GST_OBJECT_UNLOCK (pad);
goto exit; goto exit;
} }