check/gst/gstghostpad.c: Added test for removing an element with ghostpad from a bin.

Original commit message from CVS:
* check/gst/gstghostpad.c: (GST_START_TEST), (gst_ghost_pad_suite):
Added test for removing an element with ghostpad from a bin.
Fixed test as current implementation does the right thing.

* gst/gstghostpad.c: (gst_proxy_pad_class_init),
(gst_proxy_pad_do_query_type), (gst_proxy_pad_do_event),
(gst_proxy_pad_do_query), (gst_proxy_pad_do_internal_link),
(gst_proxy_pad_do_bufferalloc), (gst_proxy_pad_do_activate),
(gst_proxy_pad_do_activatepull), (gst_proxy_pad_do_activatepush),
(gst_proxy_pad_do_chain), (gst_proxy_pad_do_getrange),
(gst_proxy_pad_do_checkgetrange), (gst_proxy_pad_do_getcaps),
(gst_proxy_pad_do_acceptcaps), (gst_proxy_pad_do_fixatecaps),
(gst_proxy_pad_do_setcaps), (gst_proxy_pad_set_target),
(gst_proxy_pad_get_target), (gst_proxy_pad_init),
(gst_proxy_pad_dispose), (gst_proxy_pad_finalize),
(gst_ghost_pad_class_init), (gst_ghost_pad_do_activate_push),
(gst_ghost_pad_do_link), (gst_ghost_pad_do_unlink),
(gst_ghost_pad_set_internal), (gst_ghost_pad_dispose),
(gst_ghost_pad_new_notarget), (gst_ghost_pad_new),
(gst_ghost_pad_get_target), (gst_ghost_pad_set_target):
* gst/gstghostpad.h:
Clean up ghostpads, remove properties for internal stuff.
Make threadsafe.
Fix refcounting.
Prepare for switching targets, not all use cases work yet.

ChangeLog

@@ -1,3 +1,31 @@
+2005-07-29  Wim Taymans  <wim@fluendo.com>
+
+	* check/gst/gstghostpad.c: (GST_START_TEST), (gst_ghost_pad_suite):
+	Added test for removing an element with ghostpad from a bin.
+	Fixed test as current implementation does the right thing.
+
+	* gst/gstghostpad.c: (gst_proxy_pad_class_init),
+	(gst_proxy_pad_do_query_type), (gst_proxy_pad_do_event),
+	(gst_proxy_pad_do_query), (gst_proxy_pad_do_internal_link),
+	(gst_proxy_pad_do_bufferalloc), (gst_proxy_pad_do_activate),
+	(gst_proxy_pad_do_activatepull), (gst_proxy_pad_do_activatepush),
+	(gst_proxy_pad_do_chain), (gst_proxy_pad_do_getrange),
+	(gst_proxy_pad_do_checkgetrange), (gst_proxy_pad_do_getcaps),
+	(gst_proxy_pad_do_acceptcaps), (gst_proxy_pad_do_fixatecaps),
+	(gst_proxy_pad_do_setcaps), (gst_proxy_pad_set_target),
+	(gst_proxy_pad_get_target), (gst_proxy_pad_init),
+	(gst_proxy_pad_dispose), (gst_proxy_pad_finalize),
+	(gst_ghost_pad_class_init), (gst_ghost_pad_do_activate_push),
+	(gst_ghost_pad_do_link), (gst_ghost_pad_do_unlink),
+	(gst_ghost_pad_set_internal), (gst_ghost_pad_dispose),
+	(gst_ghost_pad_new_notarget), (gst_ghost_pad_new),
+	(gst_ghost_pad_get_target), (gst_ghost_pad_set_target):
+	* gst/gstghostpad.h:
+	Clean up ghostpads, remove properties for internal stuff.
+	Make threadsafe.
+	Fix refcounting.
+	Prepare for switching targets, not all use cases work yet.
+
 2005-07-29  Wim Taymans  <wim@fluendo.com>
 
 	* docs/design/part-gstghostpad.txt:

check/gst/gstghostpad.c

@@ -70,6 +70,46 @@ GST_START_TEST (test_remove1)
 
 GST_END_TEST;
 
+/* test if removing a bin also cleans up the ghostpads 
+ */
+GST_START_TEST (test_remove2)
+{
+  GstElement *b1, *b2, *src, *sink;
+  GstPad *srcpad, *sinkpad;
+  GstPadLinkReturn ret;
+
+  b1 = gst_element_factory_make ("pipeline", NULL);
+  b2 = gst_element_factory_make ("bin", NULL);
+  src = gst_element_factory_make ("fakesrc", NULL);
+  sink = gst_element_factory_make ("fakesink", NULL);
+
+  fail_unless (gst_bin_add (GST_BIN (b2), sink));
+  fail_unless (gst_bin_add (GST_BIN (b1), src));
+  fail_unless (gst_bin_add (GST_BIN (b1), b2));
+
+  sinkpad = gst_element_get_pad (sink, "sink");
+  gst_element_add_pad (b2, gst_ghost_pad_new ("sink", sinkpad));
+  gst_object_unref (sinkpad);
+
+  srcpad = gst_element_get_pad (src, "src");
+  /* get the ghostpad */
+  sinkpad = gst_element_get_pad (b2, "sink");
+
+  ret = gst_pad_link (srcpad, sinkpad);
+  fail_unless (ret == GST_PAD_LINK_OK);
+  gst_object_unref (srcpad);
+  gst_object_unref (sinkpad);
+
+  /* now remove the sink from the bin */
+  gst_bin_remove (GST_BIN (b2), sink);
+
+  srcpad = gst_element_get_pad (src, "src");
+  /* pad is still linked to ghostpad */
+  fail_if (!gst_pad_is_linked (srcpad));
+}
+
+GST_END_TEST;
+
 /* test if linking fails over different bins using a pipeline
  * like this:
  *
@@ -186,23 +226,16 @@ GST_START_TEST (test_ghost_pads)
   /* unreffing the bin will unref all elements, which will unlink and unparent
    * all pads */
 
-  /* FIXME: ghost pads need to drop their internal pad in the unlink function,
-   * but can't right now. So internal pads have a ref from their parent, and the
-   * internal pads' targets have refs from the internals. When we do the last
-   * unref on the ghost pads, these refs should go away.
-   */
-
   assert_gstrefcount (fsrc, 2); /* gisrc */
   assert_gstrefcount (gsink, 1);
   assert_gstrefcount (gsrc, 1);
   assert_gstrefcount (fsink, 2);        /* gisink */
 
-  assert_gstrefcount (gisrc, 2);        /* gsink -- fixme drop ref in unlink */
+  assert_gstrefcount (gisrc, 1);        /* gsink */
   assert_gstrefcount (isink, 2);        /* gsink */
-  assert_gstrefcount (gisink, 2);       /* gsrc -- fixme drop ref in unlink */
+  assert_gstrefcount (gisink, 1);       /* gsrc */
   assert_gstrefcount (isrc, 2); /* gsrc */
 
-  /* while the fixme isn't fixed, check cleanup */
   gst_object_unref (gsink);
   assert_gstrefcount (isink, 1);
   assert_gstrefcount (gisrc, 1);
@@ -216,6 +249,11 @@ GST_START_TEST (test_ghost_pads)
   assert_gstrefcount (fsink, 2);        /* gisrc */
   gst_object_unref (gisink);
   assert_gstrefcount (fsink, 1);
+
+  gst_object_unref (fsrc);
+  gst_object_unref (isrc);
+  gst_object_unref (isink);
+  gst_object_unref (fsink);
 }
 
 GST_END_TEST;
@@ -228,6 +266,7 @@ gst_ghost_pad_suite (void)
 
   suite_add_tcase (s, tc_chain);
   tcase_add_test (tc_chain, test_remove1);
+  tcase_add_test (tc_chain, test_remove2);
   tcase_add_test (tc_chain, test_link);
   tcase_add_test (tc_chain, test_ghost_pads);
 

gst/gstghostpad.c

@@ -37,21 +37,18 @@
 typedef struct _GstProxyPad GstProxyPad;
 typedef struct _GstProxyPadClass GstProxyPadClass;
 
-
+#define GST_PROXY_GET_LOCK(pad)	(GST_PROXY_PAD (pad)->proxy_lock)
-enum
+#define GST_PROXY_LOCK(pad)	(g_mutex_lock (GST_PROXY_GET_LOCK (pad)))
-{
+#define GST_PROXY_UNLOCK(pad)	(g_mutex_unlock (GST_PROXY_GET_LOCK (pad)))
-  PROXY_PROP_0,
-  PROXY_PROP_TARGET
-};
 
 struct _GstProxyPad
 {
   GstPad pad;
 
+  /* with PROXY_LOCK */
+  GMutex *proxy_lock;
   GstPad *target;
 
-  GMutex *property_lock;
-
   /*< private > */
   gpointer _gst_reserved[1];
 };
@@ -67,12 +64,9 @@ struct _GstProxyPadClass
 
 G_DEFINE_TYPE (GstProxyPad, gst_proxy_pad, GST_TYPE_PAD);
 
+static GstPad *gst_proxy_pad_get_target (GstPad * pad);
 
 static void gst_proxy_pad_dispose (GObject * object);
-static void gst_proxy_pad_set_property (GObject * object, guint prop_id,
-    const GValue * value, GParamSpec * pspec);
-static void gst_proxy_pad_get_property (GObject * object, guint prop_id,
-    GValue * value, GParamSpec * pspec);
 static void gst_proxy_pad_finalize (GObject * object);
 
 #ifndef GST_DISABLE_LOADSAVE
@@ -88,12 +82,6 @@ gst_proxy_pad_class_init (GstProxyPadClass * klass)
 
   gobject_class->dispose = GST_DEBUG_FUNCPTR (gst_proxy_pad_dispose);
   gobject_class->finalize = GST_DEBUG_FUNCPTR (gst_proxy_pad_finalize);
-  gobject_class->set_property = GST_DEBUG_FUNCPTR (gst_proxy_pad_set_property);
-  gobject_class->get_property = GST_DEBUG_FUNCPTR (gst_proxy_pad_get_property);
-
-  g_object_class_install_property (G_OBJECT_CLASS (klass), PROXY_PROP_TARGET,
-      g_param_spec_object ("target", "Target", "The proxy pad's target",
-          GST_TYPE_PAD, G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY));
 
 #ifndef GST_DISABLE_LOADSAVE
   {
@@ -108,41 +96,57 @@ gst_proxy_pad_class_init (GstProxyPadClass * klass)
 const GstQueryType *
 gst_proxy_pad_do_query_type (GstPad * pad)
 {
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
+  const GstQueryType *res;
 
   g_return_val_if_fail (target != NULL, NULL);
 
-  return gst_pad_get_query_types (target);
+  res = gst_pad_get_query_types (target);
+  gst_object_unref (target);
+
+  return res;
 }
 
 static gboolean
 gst_proxy_pad_do_event (GstPad * pad, GstEvent * event)
 {
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  gboolean res;
+  GstPad *target = gst_proxy_pad_get_target (pad);
 
   g_return_val_if_fail (target != NULL, FALSE);
 
-  return gst_pad_send_event (target, event);
+  res = gst_pad_send_event (target, event);
+  gst_object_unref (target);
+
+  return res;
 }
 
 static gboolean
 gst_proxy_pad_do_query (GstPad * pad, GstQuery * query)
 {
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  gboolean res;
+  GstPad *target = gst_proxy_pad_get_target (pad);
 
   g_return_val_if_fail (target != NULL, FALSE);
 
-  return gst_pad_query (target, query);
+  res = gst_pad_query (target, query);
+  gst_object_unref (target);
+
+  return res;
 }
 
 static GList *
 gst_proxy_pad_do_internal_link (GstPad * pad)
 {
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
+  GList *res;
 
   g_return_val_if_fail (target != NULL, NULL);
 
-  return gst_pad_get_internal_links (target);
+  res = gst_pad_get_internal_links (target);
+  gst_object_unref (target);
+
+  return res;
 }
 
 static GstFlowReturn
@@ -150,7 +154,7 @@ gst_proxy_pad_do_bufferalloc (GstPad * pad, guint64 offset, guint size,
     GstCaps * caps, GstBuffer ** buf)
 {
   GstFlowReturn result;
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
   GstPad *peer;
 
   g_return_val_if_fail (target != NULL, GST_FLOW_NOT_LINKED);
@@ -166,24 +170,27 @@ gst_proxy_pad_do_bufferalloc (GstPad * pad, guint64 offset, guint size,
     result = GST_FLOW_NOT_LINKED;
   }
 
+  gst_object_unref (target);
+
   return result;
 }
 
 static gboolean
 gst_proxy_pad_do_activate (GstPad * pad)
 {
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  gboolean res;
 
-  g_return_val_if_fail (target != NULL, FALSE);
+  res = gst_pad_activate_push (pad, TRUE);
 
-  return gst_pad_activate_push (pad, TRUE);
+  return res;
 }
 
 static gboolean
 gst_proxy_pad_do_activatepull (GstPad * pad, gboolean active)
 {
   GstActivateMode old;
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
+  gboolean res;
 
   g_return_val_if_fail (target != NULL, FALSE);
 
@@ -193,16 +200,20 @@ gst_proxy_pad_do_activatepull (GstPad * pad, gboolean active)
 
   if ((active && old == GST_ACTIVATE_PULL)
       || (!active && old == GST_ACTIVATE_NONE))
-    return TRUE;
+    res = TRUE;
   else
-    return gst_pad_activate_pull (target, active);
+    res = gst_pad_activate_pull (target, active);
+  gst_object_unref (target);
+
+  return res;
 }
 
 static gboolean
 gst_proxy_pad_do_activatepush (GstPad * pad, gboolean active)
 {
   GstActivateMode old;
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
+  gboolean res;
 
   g_return_val_if_fail (target != NULL, FALSE);
 
@@ -212,40 +223,52 @@ gst_proxy_pad_do_activatepush (GstPad * pad, gboolean active)
 
   if ((active && old == GST_ACTIVATE_PUSH)
       || (!active && old == GST_ACTIVATE_NONE))
-    return TRUE;
+    res = TRUE;
   else
-    return gst_pad_activate_push (target, active);
+    res = gst_pad_activate_push (target, active);
+  gst_object_unref (target);
+
+  return res;
 }
 
 static GstFlowReturn
 gst_proxy_pad_do_chain (GstPad * pad, GstBuffer * buffer)
 {
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
+  GstFlowReturn res;
 
   g_return_val_if_fail (target != NULL, GST_FLOW_NOT_LINKED);
 
-  return gst_pad_chain (target, buffer);
+  res = gst_pad_chain (target, buffer);
+  gst_object_unref (target);
+
+  return res;
 }
 
 static GstFlowReturn
 gst_proxy_pad_do_getrange (GstPad * pad, guint64 offset, guint size,
     GstBuffer ** buffer)
 {
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
+  GstFlowReturn res;
 
   g_return_val_if_fail (target != NULL, GST_FLOW_NOT_LINKED);
 
-  return gst_pad_get_range (target, offset, size, buffer);
+  res = gst_pad_get_range (target, offset, size, buffer);
+  gst_object_unref (target);
+
+  return res;
 }
 
 static gboolean
 gst_proxy_pad_do_checkgetrange (GstPad * pad)
 {
   gboolean result;
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
   GstPad *peer;
 
   g_return_val_if_fail (target != NULL, FALSE);
+
   peer = gst_pad_get_peer (target);
   if (peer) {
     result = gst_pad_check_pull_range (peer);
@@ -253,115 +276,126 @@ gst_proxy_pad_do_checkgetrange (GstPad * pad)
   } else {
     result = FALSE;
   }
+  gst_object_unref (target);
+
   return result;
 }
 
 static GstCaps *
 gst_proxy_pad_do_getcaps (GstPad * pad)
 {
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
+  GstCaps *res;
 
   g_return_val_if_fail (target != NULL, NULL);
 
-  return gst_pad_get_caps (target);
+  res = gst_pad_get_caps (target);
+  gst_object_unref (target);
+
+  return res;
 }
 
 static gboolean
 gst_proxy_pad_do_acceptcaps (GstPad * pad, GstCaps * caps)
 {
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
+  gboolean res;
 
   g_return_val_if_fail (target != NULL, FALSE);
 
-  return gst_pad_accept_caps (target, caps);
+  res = gst_pad_accept_caps (target, caps);
+  gst_object_unref (target);
+
+  return res;
 }
 
 static void
 gst_proxy_pad_do_fixatecaps (GstPad * pad, GstCaps * caps)
 {
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
 
   g_return_if_fail (target != NULL);
 
   gst_pad_fixate_caps (target, caps);
+  gst_object_unref (target);
 }
 
 static gboolean
 gst_proxy_pad_do_setcaps (GstPad * pad, GstCaps * caps)
 {
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
+  gboolean res;
 
   g_return_val_if_fail (target != NULL, FALSE);
 
-  return gst_pad_set_caps (target, caps);
+  res = gst_pad_set_caps (target, caps);
+  gst_object_unref (target);
+
+  return res;
 }
 
 #define SETFUNC(member, kind) \
   if (target->member) \
     gst_pad_set_##kind##_function (pad, gst_proxy_pad_do_##kind)
 
-static void
+static gboolean
-gst_proxy_pad_set_property (GObject * object, guint prop_id,
+gst_proxy_pad_set_target (GstPad * pad, GstPad * target)
-    const GValue * value, GParamSpec * pspec)
 {
-  GstPad *pad = GST_PAD (object);
+  GST_PROXY_LOCK (pad);
-
+  /* clear old target */
-  switch (prop_id) {
+  if (GST_PROXY_PAD_TARGET (pad)) {
-    case PROXY_PROP_TARGET:{
+    gst_object_unref (GST_PROXY_PAD_TARGET (pad));
-      GstPad *target;
+    GST_PROXY_PAD_TARGET (pad) = NULL;
-
-      target = GST_PAD_CAST (gst_object_ref
-          (GST_OBJECT_CAST (g_value_get_object (value))));
-      GST_PROXY_PAD_TARGET (object) = target;
-
-      /* really, all these should have default implementations so I can set them
-       * in the _init() instead of here */
-      SETFUNC (querytypefunc, query_type);
-      SETFUNC (eventfunc, event);
-      SETFUNC (queryfunc, query);
-      SETFUNC (intlinkfunc, internal_link);
-      SETFUNC (activatefunc, activate);
-      SETFUNC (activatepullfunc, activatepull);
-      SETFUNC (activatepushfunc, activatepush);
-      SETFUNC (getcapsfunc, getcaps);
-      SETFUNC (acceptcapsfunc, acceptcaps);
-      SETFUNC (fixatecapsfunc, fixatecaps);
-      SETFUNC (setcapsfunc, setcaps);
-
-      if (GST_PAD_DIRECTION (pad) == GST_PAD_SINK) {
-        SETFUNC (bufferallocfunc, bufferalloc);
-        SETFUNC (chainfunc, chain);
-      } else {
-        SETFUNC (getrangefunc, getrange);
-        SETFUNC (checkgetrangefunc, checkgetrange);
-      }
-
-      break;
-    }
-    default:
-      G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
-      break;
   }
+
+  if (target) {
+    /* set and ref new target if any */
+    GST_PROXY_PAD_TARGET (pad) = gst_object_ref (target);
+
+    /* really, all these should have default implementations so I can set them
+     * in the _init() instead of here */
+    SETFUNC (querytypefunc, query_type);
+    SETFUNC (eventfunc, event);
+    SETFUNC (queryfunc, query);
+    SETFUNC (intlinkfunc, internal_link);
+    SETFUNC (activatefunc, activate);
+    SETFUNC (activatepullfunc, activatepull);
+    SETFUNC (activatepushfunc, activatepush);
+    SETFUNC (getcapsfunc, getcaps);
+    SETFUNC (acceptcapsfunc, acceptcaps);
+    SETFUNC (fixatecapsfunc, fixatecaps);
+    SETFUNC (setcapsfunc, setcaps);
+
+    if (GST_PAD_DIRECTION (pad) == GST_PAD_SINK) {
+      SETFUNC (bufferallocfunc, bufferalloc);
+      SETFUNC (chainfunc, chain);
+    } else {
+      SETFUNC (getrangefunc, getrange);
+      SETFUNC (checkgetrangefunc, checkgetrange);
+    }
+  }
+  GST_PROXY_UNLOCK (pad);
+
+  return TRUE;
 }
 
-static void
+static GstPad *
-gst_proxy_pad_get_property (GObject * object, guint prop_id,
+gst_proxy_pad_get_target (GstPad * pad)
-    GValue * value, GParamSpec * pspec)
 {
-  switch (prop_id) {
+  GstPad *target;
-    case PROXY_PROP_TARGET:
+
-      g_value_set_object (value, GST_PROXY_PAD_TARGET (object));
+  GST_PROXY_LOCK (pad);
-      break;
+  target = GST_PROXY_PAD_TARGET (pad);
-    default:
+  gst_object_ref (target);
-      G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
+  GST_PROXY_UNLOCK (pad);
-      break;
+
-  }
+  return target;
 }
 
 static void
 gst_proxy_pad_init (GstProxyPad * pad)
 {
-  pad->property_lock = g_mutex_new ();
+  pad->proxy_lock = g_mutex_new ();
 }
 
 static void
@@ -369,9 +403,9 @@ gst_proxy_pad_dispose (GObject * object)
 {
   GstPad *pad = GST_PAD (object);
 
-  if (GST_PROXY_PAD_TARGET (pad)) {
+  GST_PROXY_LOCK (pad);
-    gst_object_replace ((GstObject **) & GST_PROXY_PAD_TARGET (pad), NULL);
+  gst_object_replace ((GstObject **) & GST_PROXY_PAD_TARGET (pad), NULL);
-  }
+  GST_PROXY_UNLOCK (pad);
 
   G_OBJECT_CLASS (gst_proxy_pad_parent_class)->dispose (object);
 }
@@ -381,8 +415,8 @@ gst_proxy_pad_finalize (GObject * object)
 {
   GstProxyPad *pad = GST_PROXY_PAD (object);
 
-  g_mutex_free (pad->property_lock);
+  g_mutex_free (pad->proxy_lock);
-  pad->property_lock = NULL;
+  pad->proxy_lock = NULL;
 
   G_OBJECT_CLASS (gst_proxy_pad_parent_class)->finalize (object);
 }
@@ -422,16 +456,11 @@ gst_proxy_pad_save_thyself (GstObject * object, xmlNodePtr parent)
  */
 
 
-enum
-{
-  GHOST_PROP_0,
-  GHOST_PROP_INTERNAL
-};
-
 struct _GstGhostPad
 {
   GstProxyPad pad;
 
+  /* with PROXY_LOCK */
   GstPad *internal;
   gulong notify_id;
 
@@ -450,13 +479,10 @@ struct _GstGhostPadClass
 
 G_DEFINE_TYPE (GstGhostPad, gst_ghost_pad, GST_TYPE_PROXY_PAD);
 
+static gboolean gst_ghost_pad_set_internal (GstGhostPad * pad,
+    GstPad * internal);
 
 static void gst_ghost_pad_dispose (GObject * object);
-static void gst_ghost_pad_set_property (GObject * object, guint prop_id,
-    const GValue * value, GParamSpec * pspec);
-static void gst_ghost_pad_get_property (GObject * object, guint prop_id,
-    GValue * value, GParamSpec * pspec);
-
 
 /* Work around g_logv's use of G_GNUC_PRINTF because gcc chokes on %P, which we
  * use for GST_PTR_FORMAT. */
@@ -476,12 +502,6 @@ gst_ghost_pad_class_init (GstGhostPadClass * klass)
   GObjectClass *gobject_class = (GObjectClass *) klass;
 
   gobject_class->dispose = GST_DEBUG_FUNCPTR (gst_ghost_pad_dispose);
-  gobject_class->set_property = GST_DEBUG_FUNCPTR (gst_ghost_pad_set_property);
-  gobject_class->get_property = GST_DEBUG_FUNCPTR (gst_ghost_pad_get_property);
-
-  g_object_class_install_property (G_OBJECT_CLASS (klass), GHOST_PROP_INTERNAL,
-      g_param_spec_object ("internal", "Internal",
-          "The ghost pad's internal pad", GST_TYPE_PAD, G_PARAM_READWRITE));
 }
 
 /* will only be called for src pads (afaict) */
@@ -511,10 +531,10 @@ gst_ghost_pad_do_activate_push (GstPad * pad, gboolean active)
 
   ret = gst_proxy_pad_do_activatepush (pad, active);
 
-  GST_LOCK (pad);
+  GST_PROXY_LOCK (pad);
   if ((internal = GST_GHOST_PAD (pad)->internal))
     gst_object_ref (internal);
-  GST_UNLOCK (pad);
+  GST_PROXY_UNLOCK (pad);
 
   if (internal) {
     ret &= gst_pad_activate_push (internal, active);
@@ -528,41 +548,49 @@ static GstPadLinkReturn
 gst_ghost_pad_do_link (GstPad * pad, GstPad * peer)
 {
   GstPad *internal, *target;
+  GstPadLinkReturn ret;
+
+  target = gst_proxy_pad_get_target (pad);
 
-  target = GST_PROXY_PAD_TARGET (pad);
   g_return_val_if_fail (target != NULL, GST_PAD_LINK_NOSCHED);
 
   /* proxy the peer into the bin */
   internal = g_object_new (GST_TYPE_PROXY_PAD,
       "name", NULL,
       "direction", GST_PAD_DIRECTION (peer),
-      "template", GST_PAD_PAD_TEMPLATE (peer), "target", peer, NULL);
+      "template", GST_PAD_PAD_TEMPLATE (peer), NULL);
-  g_object_set (pad, "internal", internal, NULL);
 
-  if ((GST_PAD_IS_SRC (internal) &&
+  gst_proxy_pad_set_target (internal, peer);
-          gst_pad_link (internal, target) == GST_PAD_LINK_OK) ||
+  gst_ghost_pad_set_internal (GST_GHOST_PAD (pad), internal);
-      (GST_PAD_IS_SINK (internal) &&
+
-          (gst_pad_link (target, internal) == GST_PAD_LINK_OK))) {
+  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)
     gst_pad_set_active (internal, GST_PAD_ACTIVATE_MODE (pad));
-    return GST_PAD_LINK_OK;
+  else
-  } else {
+    gst_ghost_pad_set_internal (GST_GHOST_PAD (pad), NULL);
-    g_object_set (pad, "internal", NULL, NULL);
+
-    return GST_PAD_LINK_REFUSED;
+  return ret;
-  }
 }
 
 static void
 gst_ghost_pad_do_unlink (GstPad * pad)
 {
-  GstPad *target = GST_PROXY_PAD_TARGET (pad);
+  GstPad *target = gst_proxy_pad_get_target (pad);
 
   g_return_if_fail (target != NULL);
 
   if (target->unlinkfunc)
     target->unlinkfunc (target);
 
-  /* FIXME: should do this here, but locks in deep_notify prevent it */
+  gst_ghost_pad_set_internal (GST_GHOST_PAD (pad), NULL);
-  /* g_object_set (pad, "internal", NULL, NULL); */
+
+  gst_object_unref (target);
 }
 
 static void
@@ -581,84 +609,57 @@ on_int_notify (GstPad * internal, GParamSpec * unused, GstGhostPad * pad)
     gst_caps_unref (caps);
 }
 
-static void
+static gboolean
-gst_ghost_pad_set_property (GObject * object, guint prop_id,
+gst_ghost_pad_set_internal (GstGhostPad * pad, GstPad * internal)
-    const GValue * value, GParamSpec * pspec)
 {
-  GstGhostPad *pad = GST_GHOST_PAD (object);
+  GST_PROXY_LOCK (pad);
+  /* first remove old internal pad */
+  if (pad->internal) {
+    GstPad *intpeer;
 
-  switch (prop_id) {
+    gst_pad_set_activatepull_function (pad->internal, NULL);
-    case GHOST_PROP_INTERNAL:{
-      GstPad *internal;
 
-      g_mutex_lock (GST_PROXY_PAD (pad)->property_lock);
+    g_signal_handler_disconnect (pad->internal, pad->notify_id);
 
-      if (pad->internal) {
+    intpeer = gst_pad_get_peer (pad->internal);
-        GstPad *intpeer;
+    if (intpeer) {
-
+      if (GST_PAD_IS_SRC (pad->internal))
-        gst_pad_set_activatepull_function (pad->internal, NULL);
+        gst_pad_unlink (pad->internal, intpeer);
-
+      else
-        g_signal_handler_disconnect (pad->internal, pad->notify_id);
+        gst_pad_unlink (intpeer, pad->internal);
-
+      gst_object_unref (intpeer);
-        intpeer = gst_pad_get_peer (pad->internal);
-        if (intpeer) {
-          if (GST_PAD_IS_SRC (pad->internal)) {
-            gst_pad_unlink (pad->internal, intpeer);
-          } else {
-            gst_pad_unlink (intpeer, pad->internal);
-          }
-          gst_object_unref (intpeer);
-        }
-
-        /* should dispose it */
-        gst_object_unparent (GST_OBJECT_CAST (pad->internal));
-      }
-
-      internal = g_value_get_object (value);    /* no extra refcount... */
-
-      if (internal) {
-        if (!gst_object_set_parent (GST_OBJECT_CAST (internal),
-                GST_OBJECT_CAST (pad))) {
-          gst_critical ("Could not set internal pad %" GST_PTR_FORMAT,
-              internal);
-          g_mutex_unlock (GST_PROXY_PAD (pad)->property_lock);
-          return;
-        }
-
-        /* could be more general here, iterating over all writable properties...
-         * taking the short road for now tho */
-        pad->notify_id = g_signal_connect (internal, "notify::caps",
-            G_CALLBACK (on_int_notify), pad);
-        on_int_notify (internal, NULL, pad);
-        gst_pad_set_activatepull_function (internal,
-            gst_ghost_proxy_pad_do_activate_pull);
-
-        /* a ref was taken by set_parent */
-      }
-
-      pad->internal = internal;
-
-      g_mutex_unlock (GST_PROXY_PAD (pad)->property_lock);
-
-      break;
     }
-    default:
+    /* should dispose it */
-      G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
+    gst_object_unparent (GST_OBJECT_CAST (pad->internal));
-      break;
   }
-}
 
-static void
+  /* then set new internal pad */
-gst_ghost_pad_get_property (GObject * object, guint prop_id,
+  if (internal) {
-    GValue * value, GParamSpec * pspec)
+    if (!gst_object_set_parent (GST_OBJECT_CAST (internal),
-{
+            GST_OBJECT_CAST (pad)))
-  switch (prop_id) {
+      goto could_not_set;
-    case GHOST_PROP_INTERNAL:
+
-      g_value_set_object (value, GST_GHOST_PAD (object)->internal);
+    /* could be more general here, iterating over all writable properties...
-      break;
+     * taking the short road for now tho */
-    default:
+    pad->notify_id = g_signal_connect (internal, "notify::caps",
-      G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
+        G_CALLBACK (on_int_notify), pad);
-      break;
+    on_int_notify (internal, NULL, pad);
+    gst_pad_set_activatepull_function (internal,
+        gst_ghost_proxy_pad_do_activate_pull);
+    /* a ref was taken by set_parent */
+  }
+  pad->internal = internal;
+
+  GST_PROXY_UNLOCK (pad);
+
+  return TRUE;
+
+  /* ERRORS */
+could_not_set:
+  {
+    gst_critical ("Could not set internal pad %" GST_PTR_FORMAT, internal);
+    GST_PROXY_UNLOCK (pad);
+    return FALSE;
   }
 }
 
@@ -671,16 +672,46 @@ gst_ghost_pad_init (GstGhostPad * pad)
 static void
 gst_ghost_pad_dispose (GObject * object)
 {
-  g_object_set (object, "internal", NULL, NULL);
+  gst_ghost_pad_set_internal (GST_GHOST_PAD (object), NULL);
 
   G_OBJECT_CLASS (gst_ghost_pad_parent_class)->dispose (object);
 }
 
+/**
+ * gst_ghost_pad_new_notarget:
+ * @name: the name of the new pad, or NULL to assign a default name.
+ * @dir: the direction of the ghostpad
+ *
+ * Create a new ghostpad without a target with the given direction.
+ * A target can be set on the ghostpad later with the 
+ * #gst_ghost_pad_set_target() function.
+ *
+ * The created ghostpad will not have a padtemplate.
+ *
+ * Returns: a new #GstPad, or NULL in case of an error.
+ */
+GstPad *
+gst_ghost_pad_new_notarget (const gchar * name, GstPadDirection dir)
+{
+  GstPad *ret;
+
+  ret = g_object_new (GST_TYPE_GHOST_PAD, "name", name, "direction", dir, NULL);
+
+  gst_pad_set_activatepush_function (ret, gst_ghost_pad_do_activate_push);
+  gst_pad_set_link_function (ret, gst_ghost_pad_do_link);
+  gst_pad_set_unlink_function (ret, gst_ghost_pad_do_unlink);
+
+  return ret;
+}
+
 /**
  * gst_ghost_pad_new:
  * @name: the name of the new pad, or NULL to assign a default name.
  * @target: the pad to ghost.
  *
+ * Create a new ghostpad with @target as the target. The direction and
+ * padtemplate will be taken from the target pad.
+ *
  * Will ref the target.
  *
  * Returns: a new #GstPad, or NULL in case of an error.
@@ -691,16 +722,47 @@ gst_ghost_pad_new (const gchar * name, GstPad * target)
   GstPad *ret;
 
   g_return_val_if_fail (GST_IS_PAD (target), NULL);
-  g_return_val_if_fail (!GST_PAD_IS_LINKED (target), NULL);
+  g_return_val_if_fail (!gst_pad_is_linked (target), NULL);
-
-  ret = g_object_new (GST_TYPE_GHOST_PAD,
-      "name", name,
-      "direction", GST_PAD_DIRECTION (target),
-      "template", GST_PAD_PAD_TEMPLATE (target), "target", target, NULL);
-
-  gst_pad_set_activatepush_function (ret, gst_ghost_pad_do_activate_push);
-  gst_pad_set_link_function (ret, gst_ghost_pad_do_link);
-  gst_pad_set_unlink_function (ret, gst_ghost_pad_do_unlink);
 
+  if ((ret = gst_ghost_pad_new_notarget (name, GST_PAD_DIRECTION (target)))) {
+    g_object_set (G_OBJECT (ret),
+        "template", GST_PAD_PAD_TEMPLATE (target), NULL);
+    gst_ghost_pad_set_target (GST_GHOST_PAD (ret), target);
+  }
   return ret;
 }
+
+/**
+ * gst_ghost_pad_get_target:
+ * @gpad: the #GstGhostpad
+ *
+ * Get the target pad of #gpad. Unref after usage.
+ *
+ * Returns: the target #GstPad, can be NULL if the ghostpad
+ * has no target set. Unref after usage.
+ */
+GstPad *
+gst_ghost_pad_get_target (GstGhostPad * gpad)
+{
+  g_return_val_if_fail (GST_IS_GHOST_PAD (gpad), NULL);
+
+  return gst_proxy_pad_get_target (GST_PAD_CAST (gpad));
+}
+
+/**
+ * gst_ghost_pad_set_target:
+ * @gpad: the #GstGhostpad
+ * @newtarget: the new pad target
+ *
+ * Set the new target of the ghostpad @gpad. Any existing target
+ * is unlinked.
+ *
+ * Returns: TRUE if the new target could be set, FALSE otherwise.
+ */
+gboolean
+gst_ghost_pad_set_target (GstGhostPad * gpad, GstPad * newtarget)
+{
+  g_return_val_if_fail (GST_IS_GHOST_PAD (gpad), FALSE);
+
+  return gst_proxy_pad_set_target (GST_PAD_CAST (gpad), newtarget);
+}

gst/gstghostpad.h

@@ -31,23 +31,23 @@
 
 G_BEGIN_DECLS
 
-
 #define GST_TYPE_GHOST_PAD		(gst_ghost_pad_get_type ())
 #define GST_IS_GHOST_PAD(obj)		(G_TYPE_CHECK_INSTANCE_TYPE ((obj), GST_TYPE_GHOST_PAD))
 #define GST_IS_GHOST_PAD_CLASS(klass)	(G_TYPE_CHECK_CLASS_TYPE ((klass), GST_TYPE_GHOST_PAD))
 #define GST_GHOST_PAD(obj)		(G_TYPE_CHECK_INSTANCE_CAST ((obj), GST_TYPE_GHOST_PAD, GstGhostPad))
 #define GST_GHOST_PAD_CLASS(klass)	(G_TYPE_CHECK_CLASS_CAST ((klass), GST_TYPE_GHOST_PAD, GstGhostPadClass))
 
-
 typedef struct _GstGhostPad GstGhostPad;
 typedef struct _GstGhostPadClass GstGhostPadClass;
 
-
 GType		 gst_ghost_pad_get_type 	(void);
-GstPad*		 gst_ghost_pad_new		(const gchar *name, GstPad *target);
 
+GstPad*		 gst_ghost_pad_new		(const gchar *name, GstPad *target);
+GstPad*		 gst_ghost_pad_new_notarget	(const gchar *name, GstPadDirection dir);
+
+GstPad*		 gst_ghost_pad_get_target	(GstGhostPad *gpad);
+gboolean	 gst_ghost_pad_set_target	(GstGhostPad *gpad, GstPad *newtarget);
 
 G_END_DECLS
 
-
 #endif /* __GST_GHOST_PAD_H__ */

tests/check/gst/gstghostpad.c

@@ -70,6 +70,46 @@ GST_START_TEST (test_remove1)
 
 GST_END_TEST;
 
+/* test if removing a bin also cleans up the ghostpads 
+ */
+GST_START_TEST (test_remove2)
+{
+  GstElement *b1, *b2, *src, *sink;
+  GstPad *srcpad, *sinkpad;
+  GstPadLinkReturn ret;
+
+  b1 = gst_element_factory_make ("pipeline", NULL);
+  b2 = gst_element_factory_make ("bin", NULL);
+  src = gst_element_factory_make ("fakesrc", NULL);
+  sink = gst_element_factory_make ("fakesink", NULL);
+
+  fail_unless (gst_bin_add (GST_BIN (b2), sink));
+  fail_unless (gst_bin_add (GST_BIN (b1), src));
+  fail_unless (gst_bin_add (GST_BIN (b1), b2));
+
+  sinkpad = gst_element_get_pad (sink, "sink");
+  gst_element_add_pad (b2, gst_ghost_pad_new ("sink", sinkpad));
+  gst_object_unref (sinkpad);
+
+  srcpad = gst_element_get_pad (src, "src");
+  /* get the ghostpad */
+  sinkpad = gst_element_get_pad (b2, "sink");
+
+  ret = gst_pad_link (srcpad, sinkpad);
+  fail_unless (ret == GST_PAD_LINK_OK);
+  gst_object_unref (srcpad);
+  gst_object_unref (sinkpad);
+
+  /* now remove the sink from the bin */
+  gst_bin_remove (GST_BIN (b2), sink);
+
+  srcpad = gst_element_get_pad (src, "src");
+  /* pad is still linked to ghostpad */
+  fail_if (!gst_pad_is_linked (srcpad));
+}
+
+GST_END_TEST;
+
 /* test if linking fails over different bins using a pipeline
  * like this:
  *
@@ -186,23 +226,16 @@ GST_START_TEST (test_ghost_pads)
   /* unreffing the bin will unref all elements, which will unlink and unparent
    * all pads */
 
-  /* FIXME: ghost pads need to drop their internal pad in the unlink function,
-   * but can't right now. So internal pads have a ref from their parent, and the
-   * internal pads' targets have refs from the internals. When we do the last
-   * unref on the ghost pads, these refs should go away.
-   */
-
   assert_gstrefcount (fsrc, 2); /* gisrc */
   assert_gstrefcount (gsink, 1);
   assert_gstrefcount (gsrc, 1);
   assert_gstrefcount (fsink, 2);        /* gisink */
 
-  assert_gstrefcount (gisrc, 2);        /* gsink -- fixme drop ref in unlink */
+  assert_gstrefcount (gisrc, 1);        /* gsink */
   assert_gstrefcount (isink, 2);        /* gsink */
-  assert_gstrefcount (gisink, 2);       /* gsrc -- fixme drop ref in unlink */
+  assert_gstrefcount (gisink, 1);       /* gsrc */
   assert_gstrefcount (isrc, 2); /* gsrc */
 
-  /* while the fixme isn't fixed, check cleanup */
   gst_object_unref (gsink);
   assert_gstrefcount (isink, 1);
   assert_gstrefcount (gisrc, 1);
@@ -216,6 +249,11 @@ GST_START_TEST (test_ghost_pads)
   assert_gstrefcount (fsink, 2);        /* gisrc */
   gst_object_unref (gisink);
   assert_gstrefcount (fsink, 1);
+
+  gst_object_unref (fsrc);
+  gst_object_unref (isrc);
+  gst_object_unref (isink);
+  gst_object_unref (fsink);
 }
 
 GST_END_TEST;
@@ -228,6 +266,7 @@ gst_ghost_pad_suite (void)
 
   suite_add_tcase (s, tc_chain);
   tcase_add_test (tc_chain, test_remove1);
+  tcase_add_test (tc_chain, test_remove2);
   tcase_add_test (tc_chain, test_link);
   tcase_add_test (tc_chain, test_ghost_pads);