tracelib: use sequence numbers instead of object addresses for hash-keys

The use of object adresses causes issues when a object is released and later an
object of the same type is allocated at the same address. Thaks to g_slice this
is quite likely to happen in practise.

The current implementation is not optimal. We might consider storing the actual
stats in the qdata and using a gpointer array instead of the hastable to keep
them for statistics and cleanup.

1 files changed, 63 insertions, 82 deletions

diff --git a/src/gsttracelib.c b/src/gsttracelib.c
index 7a0ad30..22d74c9 100644
--- a/src/gsttracelib.c
+++ b/src/gsttracelib.c
@@ -104,6 +104,8 @@ static GstClockTime areal = G_GUINT64_CONSTANT(0);
 
 #define GSTTL_TIME_AS_SECOND(t) ((gdouble)t/(gdouble)GST_SECOND)
 
+/* for statistics */
+static GQuark idquark;
 G_LOCK_DEFINE (_stats);
 
 typedef struct {
@@ -137,7 +139,7 @@ typedef struct {
   /* time spend in this element */
   GstClockTime treal;
   /* hierarchy */
-  GstElement *parent;
+  gpointer parent_id;
 } GsttlElementStats;
 
 typedef struct {
@@ -447,33 +449,11 @@ _check_playing (GstElement *element)
   }
 }
 
-
-/* FIXME: we are using the object addresses as hashtable index right now
- * - this is broken as object can be released and new object can be allocated at
- *   the same address
- *   - the problem can be seen in "sticky pad-names", e.g. "tempsink_sink" in
- *     playbin2
- *
- * - reffing all objects to block the address reuse has weired side-effects
- * - we can use a weak-pointer to NULL a cookie in the stats to figure
- *   that this is a new object, but then we only know that things will go wrong
-
- * - we can't add an 'id' via g_object_set_qdata either as we look up the
- *   objects also after they are gone (element_stats->parent)
- * - we could use a weak ref and move all freed object to a list that is used
- *   for later statistis, ideally at the end all objects are in the lists
- *   - this still needs to deal with element_stats->parent
- *   - maybe also use the 'id' and add a _get_bin_stats_by_id()
- */
-
 static GsttlElementStats*
 _fill_element_stats (GstElement *element)
 {
   GsttlElementStats *stats = g_new0(GsttlElementStats,1);
-  if(GST_OBJECT_NAME (element)) {
-    /* yes we leak the names right now ... */
-    stats->name = g_strdup (GST_OBJECT_NAME (element));
-  }
+
   if(GST_IS_BIN(element)) {
     num_bins++;
   }
@@ -484,26 +464,34 @@ _fill_element_stats (GstElement *element)
 }
 
 static GsttlElementStats*
-_get_bin_stats (GstElement *element)
+_get_bin_stats_by_id (gpointer id)
 {
-  GsttlElementStats *stats;
+  return g_hash_table_lookup (bins, id);
+}
 
-  //if(!GST_IS_ELEMENT(element)) G_BREAKPOINT();
+static GsttlElementStats* _get_bin_stats (GstElement *element);
 
-  if (!element)
-    return NULL;
+static inline GsttlElementStats*
+_get_bin_or_element_stats (GstElement *element, GHashTable *ht)
+{
+  GsttlElementStats *stats;
+  gpointer id;
 
   G_LOCK (_stats);
-  if (!(stats = g_hash_table_lookup (bins, element))) {
-    stats = _fill_element_stats(element);
-    g_hash_table_insert (bins, element, (gpointer)stats);
+  id = g_object_get_qdata ((GObject *)element, idquark);
+  stats = id ? g_hash_table_lookup (ht, id) : NULL;
+  if (!id || !stats) {
+    id = GUINT_TO_POINTER (num_elements+1);
+    stats = _fill_element_stats (element);
+    g_object_set_qdata ((GObject *)element, idquark, id);
+    g_hash_table_insert (ht, id, (gpointer)stats);
   }
   G_UNLOCK (_stats);
-  if(G_UNLIKELY(!stats->parent)) {
+  if(G_UNLIKELY(!stats->parent_id)) {
     GstElement *parent = GST_ELEMENT_PARENT (element);
     if(parent) {
-      _get_bin_stats (parent);
-      stats->parent=parent;
+      GsttlElementStats *parent_stats = _get_bin_stats (parent);
+      stats->parent_id = GUINT_TO_POINTER (parent_stats->index + 1);
     }
   }
   if(G_UNLIKELY(!stats->name)) {
@@ -515,34 +503,23 @@ _get_bin_stats (GstElement *element)
 }
 
 static GsttlElementStats*
-_get_element_stats (GstElement *element)
+_get_bin_stats (GstElement *element)
 {
-  GsttlElementStats *stats;
+  if (!element)
+    return NULL;
 
-  //if(!GST_IS_ELEMENT(element)) G_BREAKPOINT();
+  g_assert(GST_IS_BIN(element));
+  return _get_bin_or_element_stats (element, bins);
+}
 
-  if (!element || GST_IS_BIN(element))
+static GsttlElementStats*
+_get_element_stats (GstElement *element)
+{
+  if (!element)
     return NULL;
 
-  G_LOCK (_stats);
-  if (!(stats = g_hash_table_lookup (elements, element))) {
-    stats = _fill_element_stats(element);
-    g_hash_table_insert (elements, element, (gpointer)stats);
-  }
-  G_UNLOCK (_stats);
-  if(G_UNLIKELY(!stats->parent)) {
-    GstElement *parent = GST_ELEMENT_PARENT (element);
-    if(parent) {
-      _get_bin_stats (parent);
-      stats->parent=parent;
-    }
-  }
-  if(G_UNLIKELY(!stats->name)) {
-    if(GST_OBJECT_NAME (element)) {
-      stats->name = g_strdup (GST_OBJECT_NAME (element));
-    }
-  }
-  return stats;
+  g_assert(!GST_IS_BIN(element));
+  return _get_bin_or_element_stats (element, elements);
 }
 
 /* FIXME: this looks a bit weired, check that we really want to do this */
@@ -569,13 +546,7 @@ static GsttlPadStats*
 _fill_pad_stats (GstPad *pad)
 {
   GsttlPadStats *stats = g_new0(GsttlPadStats,1);
-  GstObject *parent = GST_OBJECT_PARENT(pad);
 
-  /* yes we leak the names right now ... */
-  if (parent && GST_OBJECT_NAME(parent) && GST_OBJECT_NAME(pad)) {
-    stats->name = g_strdup_printf ("%s_%s",
-      GST_OBJECT_NAME(parent), GST_OBJECT_NAME(pad));
-  }
   if(GST_IS_GHOST_PAD(pad)) {
     num_ghostpads++;
   }
@@ -592,29 +563,38 @@ static GsttlPadStats*
 _get_pad_stats (GstPad *pad)
 {
   GsttlPadStats *stats;
-  GstObject *parent;
+  gpointer id;
 
   if(!pad)
     return NULL;
+  
+  //g_assert(GST_IS_PAD(pad));
 
   G_LOCK (_stats);
-  if (!(stats = g_hash_table_lookup (pads, pad))) {
+  id = g_object_get_qdata ((GObject *)pad, idquark);
+  stats = id ? g_hash_table_lookup (pads, id) : NULL;
+  if (!id || !stats) {
+    id = GUINT_TO_POINTER (num_pads+1);
     stats = _fill_pad_stats (pad);
-    g_hash_table_insert (pads, pad, (gpointer)stats);
+    g_object_set_qdata ((GObject *)pad, idquark, id);
+    g_hash_table_insert (pads, id, (gpointer)stats);
   }
   G_UNLOCK (_stats);
-  parent = GST_OBJECT_PARENT (pad);
-  if (GST_IS_ELEMENT (parent)) {
-    /* pad is regular pad */
-    _get_element_stats (GST_ELEMENT_CAST(parent));
-    if(G_UNLIKELY(!stats->name) && GST_OBJECT_NAME(parent) && GST_OBJECT_NAME(pad)) {
-      stats->name = g_strdup_printf ("%s_%s", GST_OBJECT_NAME(parent), GST_OBJECT_NAME(pad));
-    }
-  } else if (GST_IS_GHOST_PAD (parent)) {
-    /* pad is proxy pad */
-    _get_pad_stats (GST_PAD_CAST(parent));
-    if(G_UNLIKELY(!stats->name) && GST_OBJECT_NAME(parent) && GST_OBJECT_NAME(pad)) {
-      stats->name = g_strdup_printf ("%s_%s", GST_OBJECT_NAME(parent), GST_OBJECT_NAME(pad));
+  if(G_UNLIKELY(!stats->name)) {
+    GstObject *parent = GST_OBJECT_PARENT (pad);
+    /* yes we leak the names right now ... */
+    if (GST_IS_ELEMENT (parent)) {
+      /* pad is regular pad */
+      GST_IS_BIN (parent) ? _get_bin_stats (GST_ELEMENT_CAST(parent)) : _get_element_stats (GST_ELEMENT_CAST(parent));
+      if(GST_OBJECT_NAME(parent) && GST_OBJECT_NAME(pad)) {
+        stats->name = g_strdup_printf ("%s_%s", GST_OBJECT_NAME(parent), GST_OBJECT_NAME(pad));
+      }
+    } else if (GST_IS_GHOST_PAD (parent)) {
+      /* pad is proxy pad */
+      _get_pad_stats (GST_PAD_CAST(parent));
+      if(GST_OBJECT_NAME(parent) && GST_OBJECT_NAME(pad)) {
+        stats->name = g_strdup_printf ("%s_%s", GST_OBJECT_NAME(parent), GST_OBJECT_NAME(pad));
+      }
     }
   }
   return stats;
@@ -778,8 +758,8 @@ _accum_element_stats (gpointer value, gpointer user_data)
 {
   GsttlElementStats *stats=(GsttlElementStats *)value;
 
-  if (stats->parent) {
-    GsttlElementStats *parent_stats = _get_bin_stats (stats->parent);
+  if (stats->parent_id) {
+    GsttlElementStats *parent_stats = _get_bin_stats_by_id (stats->parent_id);
 
     parent_stats->num_events += stats->num_events;
     parent_stats->num_messages += stats->num_messages;
@@ -842,7 +822,7 @@ _check_bin_parent (gpointer key, gpointer value, gpointer user_data)
 {
   GsttlElementStats *stats=(GsttlElementStats *)value;
   
-  return (stats->parent == user_data);
+  return (stats->parent_id == user_data);
 }
 
 static gboolean
@@ -1835,6 +1815,7 @@ gst_preload_init (void)
   }
 
   /* init structures to gather stats */
+  idquark = g_quark_from_static_string ("gsttl:id");
   threads = g_hash_table_new_full (NULL,NULL,NULL,(GDestroyNotify)g_free);
   elements = g_hash_table_new_full (NULL,NULL,NULL,(GDestroyNotify)g_free);
   bins = g_hash_table_new_full (NULL,NULL,NULL,(GDestroyNotify)g_free);