Do not set the internal pad as a parent anymore so we can avoid hierarchy linking...
authorWim Taymans <wim.taymans@gmail.com>
Tue, 20 Feb 2007 10:16:27 +0000 (10:16 +0000)
committerWim Taymans <wim.taymans@gmail.com>
Tue, 20 Feb 2007 10:16:27 +0000 (10:16 +0000)
Original commit message from CVS:
* docs/design/part-gstghostpad.txt:
* gst/gstghostpad.c: (gst_ghost_pad_class_init),
(gst_ghost_pad_internal_do_activate_push),
(gst_ghost_pad_internal_do_activate_pull),
(gst_ghost_pad_do_activate_push), (gst_ghost_pad_do_activate_pull),
(gst_ghost_pad_do_link), (gst_ghost_pad_dispose),
(gst_ghost_pad_new_full), (gst_ghost_pad_set_target):
Do not set the internal pad as a parent anymore so we can avoid
hierarchy linking errors when the ghostpad has no parent yet. This also
fixes failed activation because of unlinked internal pads, which in
turn fixes the impossible case where you have to activate a pad before
you can add it to a running element.
Also fix the docs.
* gst/gstpad.c: (pre_activate), (post_activate),
(gst_pad_set_active), (gst_pad_activate_pull),
(gst_pad_activate_push), (gst_pad_check_pull_range):
Add some more debug info.
Mark activation mode in pre_activate so that we don't try to activate in
endless loops. Fixes #385084.

ChangeLog
docs/design/part-gstghostpad.txt
gst/gstghostpad.c
gst/gstpad.c

index ea589a4..1fbdd6c 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,26 @@
+2007-02-20  Wim Taymans  <wim@fluendo.com>
+
+       * docs/design/part-gstghostpad.txt:
+       * gst/gstghostpad.c: (gst_ghost_pad_class_init),
+       (gst_ghost_pad_internal_do_activate_push),
+       (gst_ghost_pad_internal_do_activate_pull),
+       (gst_ghost_pad_do_activate_push), (gst_ghost_pad_do_activate_pull),
+       (gst_ghost_pad_do_link), (gst_ghost_pad_dispose),
+       (gst_ghost_pad_new_full), (gst_ghost_pad_set_target):
+       Do not set the internal pad as a parent anymore so we can avoid
+       hierarchy linking errors when the ghostpad has no parent yet. This also
+       fixes failed activation because of unlinked internal pads, which in
+       turn fixes the impossible case where you have to activate a pad before
+       you can add it to a running element.
+       Also fix the docs.
+
+       * gst/gstpad.c: (pre_activate), (post_activate),
+       (gst_pad_set_active), (gst_pad_activate_pull),
+       (gst_pad_activate_push), (gst_pad_check_pull_range):
+       Add some more debug info.
+       Mark activation mode in pre_activate so that we don't try to activate in
+       endless loops. Fixes #385084.
+
 2007-02-19  Wim Taymans  <wim@fluendo.com>
 
        * libs/gst/base/gstbasetransform.c: (gst_base_transform_init),
index d5c987c..ef54975 100644 (file)
@@ -66,8 +66,6 @@ Ghostpads
            | target *----->//
            (------------)
 
-  The GstGhostPad (X) is also set as the parent of the GstProxyPad (Y).
-
   The target is a pointer to the internal pads peer. It is an optimisation to
   quickly get to the peer of a ghostpad without having to dereference the
   internal->peer.
index 7546e12..8324948 100644 (file)
@@ -483,96 +483,12 @@ G_DEFINE_TYPE (GstGhostPad, gst_ghost_pad, GST_TYPE_PROXY_PAD);
 
 static void gst_ghost_pad_dispose (GObject * object);
 
-/*
- * The parent_set and parent_unset are used to make sure that:
- * _ the internal and target are only linked when the GhostPad has a parent,
- * _ the internal and target are unlinked as soon as the GhostPad is removed
- *    from it's parent.
- */
-static void
-gst_ghost_pad_parent_set (GstObject * object, GstObject * parent)
-{
-  GstPad *gpad;
-  GstPad *target;
-  GstPad *internal;
-  GstPadLinkReturn ret;
-  GstObjectClass *klass;
-
-  gpad = GST_PAD (object);
-
-  /* internal is never NULL */
-  internal = GST_PROXY_PAD_INTERNAL (gpad);
-  target = gst_proxy_pad_get_target (gpad);
-
-  if (target) {
-    if (GST_PAD_IS_SRC (internal))
-      ret = gst_pad_link (internal, target);
-    else
-      ret = gst_pad_link (target, internal);
-
-    gst_object_unref (target);
-
-    if (ret != GST_PAD_LINK_OK)
-      goto link_failed;
-  }
-
-  klass = GST_OBJECT_CLASS (gst_ghost_pad_parent_class);
-
-  if (klass->parent_set)
-    klass->parent_set (object, parent);
-
-  return;
-
-  /* ERRORS */
-link_failed:
-  {
-    /* a warning is all we can do */
-    GST_WARNING_OBJECT (gpad, "could not link internal ghostpad %s:%s",
-        GST_DEBUG_PAD_NAME (target));
-    g_warning ("could not link internal ghostpad %s:%s",
-        GST_DEBUG_PAD_NAME (target));
-    return;
-  }
-}
-
-static void
-gst_ghost_pad_parent_unset (GstObject * object, GstObject * parent)
-{
-  GstPad *gpad;
-  GstPad *target;
-  GstPad *internal;
-  GstObjectClass *klass;
-
-  gpad = GST_PAD (object);
-  internal = GST_PROXY_PAD_INTERNAL (gpad);
-  target = gst_proxy_pad_get_target (gpad);
-
-  if (target) {
-    if (GST_PAD_IS_SRC (internal))
-      gst_pad_unlink (internal, target);
-    else
-      gst_pad_unlink (target, internal);
-
-    gst_object_unref (target);
-  }
-
-  klass = GST_OBJECT_CLASS (gst_ghost_pad_parent_class);
-
-  if (klass->parent_unset)
-    klass->parent_unset (object, parent);
-}
-
-
 static void
 gst_ghost_pad_class_init (GstGhostPadClass * klass)
 {
   GObjectClass *gobject_class = (GObjectClass *) klass;
-  GstObjectClass *gstobject_class = (GstObjectClass *) klass;
 
   gobject_class->dispose = GST_DEBUG_FUNCPTR (gst_ghost_pad_dispose);
-  gstobject_class->parent_set = GST_DEBUG_FUNCPTR (gst_ghost_pad_parent_set);
-  gstobject_class->parent_unset =
-      GST_DEBUG_FUNCPTR (gst_ghost_pad_parent_unset);
 }
 
 /* see gstghostpad design docs */
@@ -582,17 +498,13 @@ gst_ghost_pad_internal_do_activate_push (GstPad * pad, gboolean active)
   gboolean ret;
   GstPad *other;
 
-  GST_LOG_OBJECT (pad, "%sactivate push on %s:%s", (active ? "" : "de"),
-      GST_DEBUG_PAD_NAME (pad));
+  GST_LOG_OBJECT (pad, "%sactivate push on %s:%s, we're ok",
+      (active ? "" : "de"), GST_DEBUG_PAD_NAME (pad));
 
-  if (GST_PAD_DIRECTION (pad) == GST_PAD_SINK) {
-    other = GST_PROXY_PAD_INTERNAL (pad);
-    ret = gst_pad_activate_push (other, active);
-  } else if (G_LIKELY ((other = gst_pad_get_peer (pad)))) {
-    ret = gst_pad_activate_push (other, active);
-    gst_object_unref (other);
-  } else
-    ret = FALSE;
+  /* in both cases (SRC and SINK) we activate just the internal pad. The targets
+   * will be activated later (or already in case of a ghost sinkpad). */
+  other = GST_PROXY_PAD_INTERNAL (pad);
+  ret = gst_pad_activate_push (other, active);
 
   return ret;
 }
@@ -607,13 +519,25 @@ gst_ghost_pad_internal_do_activate_pull (GstPad * pad, gboolean active)
       GST_DEBUG_PAD_NAME (pad));
 
   if (GST_PAD_DIRECTION (pad) == GST_PAD_SRC) {
+    /* we are activated in pull mode by our peer element, which is a sinkpad
+     * that wants to operate in pull mode. This activation has to propagate
+     * upstream throught the pipeline. We call the internal activation function,
+     * which will trigger gst_ghost_pad_do_activate_pull, which propagates even
+     * further upstream */
+    GST_LOG_OBJECT (pad, "pad is src, activate internal");
     other = GST_PROXY_PAD_INTERNAL (pad);
     ret = gst_pad_activate_pull (other, active);
   } else if (G_LIKELY ((other = gst_pad_get_peer (pad)))) {
+    /* We are SINK, the ghostpad is SRC, we propagate the activation upstream
+     * since we hold a pointer to the upstream peer. */
+    GST_LOG_OBJECT (pad, "activating peer");
     ret = gst_pad_activate_pull (other, active);
     gst_object_unref (other);
-  } else
+  } else {
+    /* this is failure, we can't activate pull if there is no peer */
+    GST_LOG_OBJECT (pad, "not src and no peer, failing");
     ret = FALSE;
+  }
 
   return ret;
 }
@@ -624,14 +548,12 @@ gst_ghost_pad_do_activate_push (GstPad * pad, gboolean active)
   gboolean ret;
   GstPad *other;
 
-  GST_LOG_OBJECT (pad, "%sactivate push on %s:%s", (active ? "" : "de"),
-      GST_DEBUG_PAD_NAME (pad));
+  GST_LOG_OBJECT (pad, "%sactivate push on %s:%s, proxy internal",
+      (active ? "" : "de"), GST_DEBUG_PAD_NAME (pad));
 
-  if (GST_PAD_DIRECTION (pad) == GST_PAD_SINK) {
-    other = GST_PROXY_PAD_INTERNAL (pad);
-    ret = gst_pad_activate_push (other, active);
-  } else
-    ret = TRUE;
+  /* just activate the internal pad */
+  other = GST_PROXY_PAD_INTERNAL (pad);
+  ret = gst_pad_activate_push (other, active);
 
   return ret;
 }
@@ -646,13 +568,23 @@ gst_ghost_pad_do_activate_pull (GstPad * pad, gboolean active)
       GST_DEBUG_PAD_NAME (pad));
 
   if (GST_PAD_DIRECTION (pad) == GST_PAD_SRC) {
+    /* the ghostpad is SRC and activated in pull mode by its peer, call the
+     * activation function of the internal pad to propagate the activation
+     * upstream */
+    GST_LOG_OBJECT (pad, "pad is src, activate internal");
     other = GST_PROXY_PAD_INTERNAL (pad);
     ret = gst_pad_activate_pull (other, active);
   } else if (G_LIKELY ((other = gst_pad_get_peer (pad)))) {
+    /* We are SINK and activated by the internal pad, propagate activation
+     * upstream because we hold a ref to the upstream peer */
+    GST_LOG_OBJECT (pad, "activating peer");
     ret = gst_pad_activate_pull (other, active);
     gst_object_unref (other);
-  } else
+  } else {
+    /* no peer, we fail */
+    GST_LOG_OBJECT (pad, "pad not src and no peer, failing");
     ret = FALSE;
+  }
 
   return ret;
 }
@@ -671,7 +603,7 @@ gst_ghost_pad_do_link (GstPad * pad, GstPad * peer)
 
   ret = GST_PAD_LINK_OK;
   /* if we are a source pad, we should call the peer link function
-   * if the peer has one */
+   * if the peer has one, see design docs. */
   if (GST_PAD_IS_SRC (pad)) {
     if (GST_PAD_LINKFUNC (peer)) {
       ret = GST_PAD_LINKFUNC (peer) (peer, pad);
@@ -777,7 +709,7 @@ gst_ghost_pad_dispose (GObject * object)
   /* disposes of the internal pad, since the ghostpad is the only possible object
    * that has a refcount on the internal pad.
    */
-  gst_object_unparent (GST_OBJECT_CAST (internal));
+  gst_object_unref (internal);
 
   GST_PROXY_UNLOCK (pad);
 
@@ -850,17 +782,17 @@ gst_ghost_pad_new_full (const gchar * name, GstPadDirection dir,
 
   GST_PROXY_LOCK (ret);
 
-  /* now make the ghostpad a parent of the internal pad */
-  if (!gst_object_set_parent (GST_OBJECT_CAST (internal),
-          GST_OBJECT_CAST (ret)))
-    goto parent_failed;
+  /* don't set parent, we can't link the internal pads if parents/grandparents
+   * are different, just take ownership. */
+  gst_object_ref (internal);
+  gst_object_sink (internal);
 
-  /* The ghostpad is the parent of the internal pad and is the only object that
+  /* The ghostpad is the owner of the internal pad and is the only object that
    * can have a refcount on the internal pad.
    * At this point, the GstGhostPad has a refcount of 1, and the internal pad has
    * a refcount of 1.
    * When the refcount of the GstGhostPad drops to 0, the ghostpad will dispose
-   * it's refcount on the internal pad in the dispose method by un-parenting it.
+   * it's refcount on the internal pad in the dispose method by un-reffing it.
    * This is why we don't take extra refcounts in the assignments below
    */
   GST_PROXY_PAD_INTERNAL (ret) = internal;
@@ -884,19 +816,6 @@ gst_ghost_pad_new_full (const gchar * name, GstPadDirection dir,
   GST_PROXY_UNLOCK (ret);
 
   return ret;
-
-  /* ERRORS */
-parent_failed:
-  {
-    GST_WARNING_OBJECT (ret, "Could not set internal pad %s:%s",
-        GST_DEBUG_PAD_NAME (internal));
-    g_critical ("Could not set internal pad %s:%s",
-        GST_DEBUG_PAD_NAME (internal));
-    GST_PROXY_UNLOCK (ret);
-    gst_object_unref (ret);
-    gst_object_unref (internal);
-    return NULL;
-  }
 }
 
 /**
@@ -1069,14 +988,14 @@ gst_ghost_pad_get_target (GstGhostPad * gpad)
  * Set the new target of the ghostpad @gpad. Any existing target
  * is unlinked and links to the new target are established.
  *
- * Returns: TRUE if the new target could be set, FALSE otherwise.
+ * Returns: TRUE if the new target could be set. This function can return FALSE
+ * when the internal pads could not be linked.
  */
 gboolean
 gst_ghost_pad_set_target (GstGhostPad * gpad, GstPad * newtarget)
 {
   GstPad *internal;
   GstPad *oldtarget;
-  GstObject *parent;
   gboolean result;
   GstPadLinkReturn lret;
 
@@ -1101,25 +1020,16 @@ gst_ghost_pad_set_target (GstGhostPad * gpad, GstPad * newtarget)
   result = gst_proxy_pad_set_target_unlocked (GST_PAD_CAST (gpad), newtarget);
 
   if (result && newtarget) {
-    /* and link to internal pad if we are not unparent-ed */
-    if ((parent = gst_object_get_parent (GST_OBJECT (gpad)))) {
-      gst_object_unref (parent);
-
-      GST_DEBUG_OBJECT (gpad,
-          "GhostPad has parent, connecting internal pad to target");
+    /* and link to internal pad */
+    GST_DEBUG_OBJECT (gpad, "connecting internal pad to target");
 
-      if (GST_PAD_IS_SRC (internal))
-        lret = gst_pad_link (internal, newtarget);
-      else
-        lret = gst_pad_link (newtarget, internal);
+    if (GST_PAD_IS_SRC (internal))
+      lret = gst_pad_link (internal, newtarget);
+    else
+      lret = gst_pad_link (newtarget, internal);
 
-      if (lret != GST_PAD_LINK_OK)
-        goto link_failed;
-    } else {
-      /* we need to connect the internal pad once we have a parent */
-      GST_DEBUG_OBJECT (gpad,
-          "GhostPad doesn't have a parent, will connect internal pad later");
-    }
+    if (lret != GST_PAD_LINK_OK)
+      goto link_failed;
   }
   GST_PROXY_UNLOCK (gpad);
 
index 1187bbc..9f50441 100644 (file)
@@ -576,6 +576,7 @@ pre_activate (GstPad * pad, GstActivateMode new_mode)
       GST_OBJECT_LOCK (pad);
       GST_DEBUG_OBJECT (pad, "setting ACTIVATE_MODE NONE, set flushing");
       GST_PAD_SET_FLUSHING (pad);
+      GST_PAD_ACTIVATE_MODE (pad) = new_mode;
       /* unlock blocked pads so element can resume and stop */
       GST_PAD_BLOCK_BROADCAST (pad);
       GST_OBJECT_UNLOCK (pad);
@@ -594,11 +595,7 @@ post_activate (GstPad * pad, GstActivateMode new_mode)
     case GST_ACTIVATE_NONE:
       /* ensures that streaming stops */
       GST_PAD_STREAM_LOCK (pad);
-      /* while we're at it set activation mode */
-      GST_OBJECT_LOCK (pad);
-      GST_DEBUG_OBJECT (pad, "setting ACTIVATE_MODE %d", new_mode);
-      GST_PAD_ACTIVATE_MODE (pad) = new_mode;
-      GST_OBJECT_UNLOCK (pad);
+      GST_DEBUG_OBJECT (pad, "stopped streaming");
       GST_PAD_STREAM_UNLOCK (pad);
       break;
   }
@@ -639,22 +636,30 @@ gst_pad_set_active (GstPad * pad, gboolean active)
   if (active) {
     switch (old) {
       case GST_ACTIVATE_PUSH:
+        GST_DEBUG_OBJECT (pad, "activating pad from push");
+        ret = TRUE;
+        break;
       case GST_ACTIVATE_PULL:
+        GST_DEBUG_OBJECT (pad, "activating pad from pull");
         ret = TRUE;
         break;
       case GST_ACTIVATE_NONE:
+        GST_DEBUG_OBJECT (pad, "activating pad from none");
         ret = (GST_PAD_ACTIVATEFUNC (pad)) (pad);
         break;
     }
   } else {
     switch (old) {
       case GST_ACTIVATE_PUSH:
+        GST_DEBUG_OBJECT (pad, "deactivating pad from push");
         ret = gst_pad_activate_push (pad, FALSE);
         break;
       case GST_ACTIVATE_PULL:
+        GST_DEBUG_OBJECT (pad, "deactivating pad from pull");
         ret = gst_pad_activate_pull (pad, FALSE);
         break;
       case GST_ACTIVATE_NONE:
+        GST_DEBUG_OBJECT (pad, "deactivating pad from none");
         ret = TRUE;
         break;
     }
@@ -666,7 +671,7 @@ gst_pad_set_active (GstPad * pad, gboolean active)
       g_critical ("Failed to deactivate pad %s:%s, very bad",
           GST_DEBUG_PAD_NAME (pad));
     } else {
-      GST_WARNING ("Failed to activate pad %s:%s", GST_DEBUG_PAD_NAME (pad));
+      GST_WARNING_OBJECT (pad, "Failed to activate pad");
     }
     GST_OBJECT_UNLOCK (pad);
   }
@@ -706,21 +711,27 @@ gst_pad_activate_pull (GstPad * pad, gboolean active)
   if (active) {
     switch (old) {
       case GST_ACTIVATE_PULL:
+        GST_DEBUG_OBJECT (pad, "activating pad from pull, was ok");
         goto was_ok;
       case GST_ACTIVATE_PUSH:
+        GST_DEBUG_OBJECT (pad,
+            "activating pad from push, deactivate push first");
         /* pad was activate in the wrong direction, deactivate it
          * and reactivate it in pull mode */
         if (G_UNLIKELY (!gst_pad_activate_push (pad, FALSE)))
           goto deactivate_failed;
         /* fallthrough, pad is deactivated now. */
       case GST_ACTIVATE_NONE:
+        GST_DEBUG_OBJECT (pad, "activating pad from none");
         break;
     }
   } else {
     switch (old) {
       case GST_ACTIVATE_NONE:
+        GST_DEBUG_OBJECT (pad, "deactivating pad from none, was ok");
         goto was_ok;
       case GST_ACTIVATE_PUSH:
+        GST_DEBUG_OBJECT (pad, "deactivating pad from push, weird");
         /* pad was activated in the other direction, deactivate it
          * in push mode, this should not happen... */
         if (G_UNLIKELY (!gst_pad_activate_push (pad, FALSE)))
@@ -728,12 +739,14 @@ gst_pad_activate_pull (GstPad * pad, gboolean active)
         /* everything is fine now */
         goto was_ok;
       case GST_ACTIVATE_PULL:
+        GST_DEBUG_OBJECT (pad, "deactivating pad from pull");
         break;
     }
   }
 
   if (gst_pad_get_direction (pad) == GST_PAD_SINK) {
     if ((peer = gst_pad_get_peer (pad))) {
+      GST_DEBUG_OBJECT (pad, "calling peer");
       if (G_UNLIKELY (!gst_pad_activate_pull (peer, active)))
         goto peer_failed;
       gst_object_unref (peer);
@@ -833,21 +846,27 @@ gst_pad_activate_push (GstPad * pad, gboolean active)
   if (active) {
     switch (old) {
       case GST_ACTIVATE_PUSH:
+        GST_DEBUG_OBJECT (pad, "activating pad from push, was ok");
         goto was_ok;
       case GST_ACTIVATE_PULL:
+        GST_DEBUG_OBJECT (pad,
+            "activating pad from push, deactivating pull first");
         /* pad was activate in the wrong direction, deactivate it
          * an reactivate it in push mode */
         if (G_UNLIKELY (!gst_pad_activate_pull (pad, FALSE)))
           goto deactivate_failed;
         /* fallthrough, pad is deactivated now. */
       case GST_ACTIVATE_NONE:
+        GST_DEBUG_OBJECT (pad, "activating pad from none");
         break;
     }
   } else {
     switch (old) {
       case GST_ACTIVATE_NONE:
+        GST_DEBUG_OBJECT (pad, "deactivating pad from none, was ok");
         goto was_ok;
       case GST_ACTIVATE_PULL:
+        GST_DEBUG_OBJECT (pad, "deactivating pad from pull, weird");
         /* pad was activated in the other direction, deactivate it
          * in pull mode, this should not happen... */
         if (G_UNLIKELY (!gst_pad_activate_pull (pad, FALSE)))
@@ -855,6 +874,7 @@ gst_pad_activate_push (GstPad * pad, gboolean active)
         /* everything is fine now */
         goto was_ok;
       case GST_ACTIVATE_PUSH:
+        GST_DEBUG_OBJECT (pad, "deactivating pad from push");
         break;
     }
   }
@@ -3668,6 +3688,8 @@ gst_pad_check_pull_range (GstPad * pad)
   if (G_LIKELY ((checkgetrangefunc = peer->checkgetrangefunc) == NULL)) {
     /* FIXME, kindoff ghetto */
     ret = GST_PAD_GETRANGEFUNC (peer) != NULL;
+    GST_CAT_LOG_OBJECT (GST_CAT_SCHEDULING, pad,
+        "no checkgetrangefunc, assuming %d", ret);
   } else {
     GST_CAT_LOG_OBJECT (GST_CAT_SCHEDULING, pad,
         "calling checkgetrangefunc %s of peer pad %s:%s",