tracerrecord: don't leak the spec structures

Change the gst_tracer_record_new() api to take the parameters the make the
spec structure directly. This allows us to own the top-level structure and
also collect the args so that we can take ownership of the sub-structures.

https://bugzilla.gnome.org/show_bug.cgi?id=760821

gst/gsttracerrecord.c

@@ -37,20 +37,11 @@
 #include "gststructure.h"
 #include "gsttracerrecord.h"
 #include "gstvalue.h"
+#include <gobject/gvaluecollector.h>
 
 GST_DEBUG_CATEGORY_EXTERN (tracer_debug);
 #define GST_CAT_DEFAULT tracer_debug
 
-
-enum
-{
-  PROP_0,
-  PROP_SPEC,
-  PROP_LAST
-};
-
-static GParamSpec *properties[PROP_LAST];
-
 struct _GstTracerRecord
 {
   GstObject parent;
@@ -77,7 +68,11 @@ build_field_template (GQuark field_id, const GValue * value, gpointer user_data)
   GstTracerValueFlags flags = GST_TRACER_VALUE_FLAGS_NONE;
   gboolean res;
 
-  g_return_val_if_fail (G_VALUE_TYPE (value) == GST_TYPE_STRUCTURE, FALSE);
+  if (G_VALUE_TYPE (value) != GST_TYPE_STRUCTURE) {
+    GST_WARNING ("expected field of type GstStructure, but %s is %s",
+        g_quark_to_string (field_id), G_VALUE_TYPE_NAME (value));
+    return FALSE;
+  }
 
   sub = gst_value_get_structure (value);
   gst_structure_get (sub, "type", G_TYPE_GTYPE, &type, "flags",
@@ -125,7 +120,7 @@ gst_tracer_record_build_format (GstTracerRecord * self)
   g_string_append_c (s, ';');
 
   self->format = g_string_free (s, FALSE);
-  GST_INFO ("new format string: %s", self->format);
+  GST_DEBUG ("new format string: %s", self->format);
   g_free (name);
 }
 
@@ -134,41 +129,12 @@ gst_tracer_record_dispose (GObject * object)
 {
   GstTracerRecord *self = GST_TRACER_RECORD (object);
 
+  if (self->spec) {
+    gst_structure_free (self->spec);
+    self->spec = NULL;
+  }
   g_free (self->format);
-}
+  self->format = NULL;
-
-static void
-gst_tracer_record_set_property (GObject * object, guint prop_id,
-    const GValue * value, GParamSpec * pspec)
-{
-  GstTracerRecord *self = GST_TRACER_RECORD_CAST (object);
-
-  switch (prop_id) {
-    case PROP_SPEC:
-      self->spec = g_value_get_boxed (value);
-      gst_tracer_record_build_format (self);
-      break;
-    default:
-      G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
-      break;
-  }
-}
-
-static void
-gst_tracer_record_get_property (GObject * object, guint prop_id,
-    GValue * value, GParamSpec * pspec)
-{
-  GstTracerRecord *self = GST_TRACER_RECORD_CAST (object);
-
-  switch (prop_id) {
-    case PROP_SPEC:
-      // TODO(ensonic): copy?
-      g_value_set_boxed (value, self->spec);
-      break;
-    default:
-      G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
-      break;
-  }
 }
 
 static void
@@ -176,16 +142,7 @@ gst_tracer_record_class_init (GstTracerRecordClass * klass)
 {
   GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
 
-  gobject_class->set_property = gst_tracer_record_set_property;
-  gobject_class->get_property = gst_tracer_record_get_property;
   gobject_class->dispose = gst_tracer_record_dispose;
-
-  properties[PROP_SPEC] =
-      g_param_spec_boxed ("spec", "Spec", "Log record specification",
-      GST_TYPE_STRUCTURE,
-      G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS);
-
-  g_object_class_install_properties (gobject_class, PROP_LAST, properties);
 }
 
 static void
@@ -195,17 +152,20 @@ gst_tracer_record_init (GstTracerRecord * self)
 
 /**
  * gst_tracer_record_new:
- * @spec: the record specification
+ * @name: name of new record, must end on ".class".
+ * @firstfield: name of first field to set
+ * @...: additional arguments
+
  *
  * Create a new tracer record. The record instance can be used to efficiently
  * log entries using gst_tracer_record_log().
  *
- * The name of the @spec #GstStructure must end on '.class'. This name without
+ * The @name without the ".class" suffix will be used for the log records.
- * the suffix will be used for the log records. The @spec must have a field for
+ * There must be fields for each value that gets logged where the field name is
- * each value that gets logged where the field name is the value name. The field
+ * the value name. The field must be a #GstStructure describing the value. The
- * must be a nested structure describing the value. The sub structure must
+ * sub structure must contain a field called 'type' of %G_TYPE_GTYPE that
- * contain a field called 'type' of %G_TYPE_GTYPE that contains the GType of the
+ * contains the GType of the value. The resulting #GstTracerRecord will take
- * value.
+ * ownership of the field structures.
  *
  * The way to deal with optional values is to log an additional boolean before
  * the optional field, that if %TRUE signals that the optional field is valid
@@ -221,9 +181,50 @@ gst_tracer_record_init (GstTracerRecord * self)
  * Returns: a new #GstTracerRecord
  */
 GstTracerRecord *
-gst_tracer_record_new (GstStructure * spec)
+gst_tracer_record_new (const gchar * name, const gchar * firstfield, ...)
 {
-  return g_object_new (GST_TYPE_TRACER_RECORD, "spec", spec, NULL);
+  GstTracerRecord *self;
+  GstStructure *structure;
+  va_list varargs;
+
+  va_start (varargs, firstfield);
+  structure = gst_structure_new_empty (name);
+  if (structure) {
+    gchar *err = NULL;
+    GType type;
+    GQuark id;
+
+    while (firstfield) {
+      GValue val = { 0, };
+
+      id = g_quark_from_string (firstfield);
+      type = va_arg (varargs, GType);
+
+      /* all fields passed here must be GstStructures which we take over */
+      if (type != GST_TYPE_STRUCTURE) {
+        GST_WARNING ("expected field of type GstStructure, but %s is %s",
+            firstfield, g_type_name (type));
+      }
+
+      G_VALUE_COLLECT_INIT (&val, type, varargs, G_VALUE_NOCOPY_CONTENTS, &err);
+      if (G_UNLIKELY (err)) {
+        g_critical ("%s", err);
+        break;
+      }
+      /* see boxed_proxy_collect_value */
+      val.data[1].v_uint &= ~G_VALUE_NOCOPY_CONTENTS;
+      gst_structure_id_take_value (structure, id, &val);
+
+      firstfield = va_arg (varargs, gchar *);
+    }
+  }
+  va_end (varargs);
+
+  self = g_object_newv (GST_TYPE_TRACER_RECORD, 0, NULL);
+  self->spec = structure;
+  gst_tracer_record_build_format (self);
+
+  return self;
 }
 
 #ifndef GST_DISABLE_GST_DEBUG

gst/gsttracerrecord.h

@@ -87,7 +87,7 @@ typedef enum
 
 #ifdef GST_USE_UNSTABLE_API
 
-GstTracerRecord * gst_tracer_record_new (GstStructure *spec);
+GstTracerRecord * gst_tracer_record_new (const gchar * name, const gchar * firstfield, ...);
 
 #ifndef GST_DISABLE_GST_DEBUG
 void              gst_tracer_record_log (GstTracerRecord *self, ...);

plugins/tracers/gstlatency.c

@@ -203,7 +203,7 @@ gst_latency_tracer_class_init (GstLatencyTracerClass * klass)
 
   /* announce trace formats */
   /* *INDENT-OFF* */
-  tr_latency = gst_tracer_record_new (gst_structure_new ("latency.class",
+  tr_latency = gst_tracer_record_new ("latency.class",
       "src", GST_TYPE_STRUCTURE, gst_structure_new ("scope",
           "type", G_TYPE_GTYPE, G_TYPE_STRING,
           "related-to", GST_TYPE_TRACER_VALUE_SCOPE, GST_TRACER_VALUE_SCOPE_PAD,
@@ -220,7 +220,7 @@ gst_latency_tracer_class_init (GstLatencyTracerClass * klass)
           "min", G_TYPE_UINT64, G_GUINT64_CONSTANT (0),
           "max", G_TYPE_UINT64, G_MAXUINT64,
           NULL),
-      NULL));
+      NULL);
   /* *INDENT-ON* */
 }
 

plugins/tracers/gstrusage.c

@@ -284,7 +284,7 @@ gst_rusage_tracer_class_init (GstRUsageTracerClass * klass)
 
   /* announce trace formats */
   /* *INDENT-OFF* */
-  tr_thread = gst_tracer_record_new (gst_structure_new ("thread-rusage.class",
+  tr_thread = gst_tracer_record_new ("thread-rusage.class",
       "thread-id", GST_TYPE_STRUCTURE, gst_structure_new ("scope",
           "type", G_TYPE_GTYPE, G_TYPE_UINT64,
           "related-to", GST_TYPE_TRACER_VALUE_SCOPE, GST_TRACER_VALUE_SCOPE_THREAD,
@@ -313,8 +313,8 @@ gst_rusage_tracer_class_init (GstRUsageTracerClass * klass)
           "min", G_TYPE_UINT64, G_GUINT64_CONSTANT (0),
           "max", G_TYPE_UINT64, G_MAXUINT64,
           NULL),
-      NULL));
+      NULL);
-  tr_proc = gst_tracer_record_new (gst_structure_new ("proc-rusage.class",
+  tr_proc = gst_tracer_record_new ("proc-rusage.class",
       "thread-id", GST_TYPE_STRUCTURE, gst_structure_new ("scope",
           "type", G_TYPE_GTYPE, G_TYPE_UINT64,
           "related-to", GST_TYPE_TRACER_VALUE_SCOPE, GST_TRACER_VALUE_SCOPE_PROCESS,
@@ -343,7 +343,7 @@ gst_rusage_tracer_class_init (GstRUsageTracerClass * klass)
           "min", G_TYPE_UINT64, G_GUINT64_CONSTANT (0),
           "max", G_TYPE_UINT64, G_MAXUINT64,
           NULL),
-      NULL));
+      NULL);
   /* *INDENT-ON* */
 }
 

plugins/tracers/gststats.c

@@ -518,7 +518,7 @@ gst_stats_tracer_class_init (GstStatsTracerClass * klass)
 {
   /* announce trace formats */
   /* *INDENT-OFF* */
-  tr_buffer = gst_tracer_record_new (gst_structure_new ("buffer.class",
+  tr_buffer = gst_tracer_record_new ("buffer.class",
       "thread-id", GST_TYPE_STRUCTURE, gst_structure_new ("scope",
           "type", G_TYPE_GTYPE, G_TYPE_UINT64,
           "related-to", GST_TYPE_TRACER_VALUE_SCOPE, GST_TRACER_VALUE_SCOPE_THREAD,
@@ -571,8 +571,8 @@ gst_stats_tracer_class_init (GstStatsTracerClass * klass)
           "type", G_TYPE_GTYPE, GST_TYPE_BUFFER_FLAGS,
           "description", G_TYPE_STRING, "flags of the buffer",
           NULL),
-      NULL));
+      NULL);
-  tr_event = gst_tracer_record_new (gst_structure_new ("event.class",
+  tr_event = gst_tracer_record_new ("event.class",
       "thread-id", GST_TYPE_STRUCTURE, gst_structure_new ("scope",
           "type", G_TYPE_GTYPE, G_TYPE_UINT64,
           "related-to", GST_TYPE_TRACER_VALUE_SCOPE, GST_TRACER_VALUE_SCOPE_THREAD,
@@ -593,8 +593,8 @@ gst_stats_tracer_class_init (GstStatsTracerClass * klass)
           "type", G_TYPE_GTYPE, G_TYPE_STRING,
           "description", G_TYPE_STRING, "name of the event",
           NULL),
-      NULL));
+      NULL);
-  tr_message = gst_tracer_record_new (gst_structure_new ("message.class",
+  tr_message = gst_tracer_record_new ("message.class",
       "thread-id", GST_TYPE_STRUCTURE, gst_structure_new ("scope",
           "type", G_TYPE_GTYPE, G_TYPE_UINT64,
           "related-to", GST_TYPE_TRACER_VALUE_SCOPE, GST_TRACER_VALUE_SCOPE_THREAD,
@@ -615,9 +615,8 @@ gst_stats_tracer_class_init (GstStatsTracerClass * klass)
           "type", G_TYPE_GTYPE, GST_TYPE_STRUCTURE,
           "description", G_TYPE_STRING, "message structure",
           NULL),
-      NULL));
+      NULL);
-  tr_element_query = gst_tracer_record_new (gst_structure_new (
+  tr_element_query = gst_tracer_record_new ("element-query.class",
-      "element-query.class",
       "thread-id", GST_TYPE_STRUCTURE, gst_structure_new ("scope",
           "type", G_TYPE_GTYPE, G_TYPE_UINT64,
           "related-to", GST_TYPE_TRACER_VALUE_SCOPE, GST_TRACER_VALUE_SCOPE_THREAD,
@@ -634,8 +633,8 @@ gst_stats_tracer_class_init (GstStatsTracerClass * klass)
           "type", G_TYPE_GTYPE, G_TYPE_STRING,
           "description", G_TYPE_STRING, "name of the query",
           NULL),
-      NULL));
+      NULL);
-  tr_query = gst_tracer_record_new (gst_structure_new ("query.class",
+  tr_query = gst_tracer_record_new ("query.class",
       "thread-id", GST_TYPE_STRUCTURE, gst_structure_new ("scope",
           "type", G_TYPE_GTYPE, G_TYPE_UINT64,
           "related-to", GST_TYPE_TRACER_VALUE_SCOPE, GST_TRACER_VALUE_SCOPE_THREAD,
@@ -673,10 +672,8 @@ gst_stats_tracer_class_init (GstStatsTracerClass * klass)
           "description", G_TYPE_STRING, "query result",
           "flags", GST_TYPE_TRACER_VALUE_FLAGS, GST_TRACER_VALUE_FLAGS_OPTIONAL,
           NULL),
-      /* TODO(ensonic): "buffer-flags" */
+      NULL);
-      NULL));
+  tr_new_element = gst_tracer_record_new ("new-element.class",
-  tr_new_element = gst_tracer_record_new (gst_structure_new (
-      "new-element.class",
       "thread-id", GST_TYPE_STRUCTURE, gst_structure_new ("scope",
           "type", G_TYPE_GTYPE, G_TYPE_UINT64,
           "related-to", GST_TYPE_TRACER_VALUE_SCOPE, GST_TRACER_VALUE_SCOPE_THREAD,
@@ -705,8 +702,8 @@ gst_stats_tracer_class_init (GstStatsTracerClass * klass)
           "type", G_TYPE_GTYPE, G_TYPE_BOOLEAN,
           "description", G_TYPE_STRING, "is element a bin",
           NULL),
-      NULL));
+      NULL);
-  tr_new_pad = gst_tracer_record_new (gst_structure_new ("new-pad.class",
+  tr_new_pad = gst_tracer_record_new ("new-pad.class",
       "thread-id", GST_TYPE_STRUCTURE, gst_structure_new ("scope",
           "type", G_TYPE_GTYPE, G_TYPE_UINT64,
           "related-to", GST_TYPE_TRACER_VALUE_SCOPE, GST_TRACER_VALUE_SCOPE_THREAD,
@@ -735,7 +732,7 @@ gst_stats_tracer_class_init (GstStatsTracerClass * klass)
           "type", G_TYPE_GTYPE, GST_TYPE_PAD_DIRECTION,
           "description", G_TYPE_STRING, "ipad direction",
           NULL),
-      NULL));
+      NULL);
   /* *INDENT-ON* */
 }
 

tests/check/gst/gsttracerrecord.c

@@ -72,11 +72,11 @@ GST_START_TEST (serialize_message_logging)
   gchar *str;
 
   /* *INDENT-OFF* */
-  tr = gst_tracer_record_new (gst_structure_new ("test.class",
+  tr = gst_tracer_record_new ("test.class",
       "string", GST_TYPE_STRUCTURE, gst_structure_new ("value",
           "type", G_TYPE_GTYPE, G_TYPE_STRING,
           NULL),
-      NULL));
+      NULL);
   /* *INDENT-ON* */
 
   save_messages = TRUE;
@@ -104,7 +104,7 @@ GST_START_TEST (serialize_static_record)
   GstPadDirection enum_val;
 
   /* *INDENT-OFF* */
-  tr = gst_tracer_record_new (gst_structure_new ("test.class",
+  tr = gst_tracer_record_new ("test.class",
       "string", GST_TYPE_STRUCTURE, gst_structure_new ("value",
           "type", G_TYPE_GTYPE, G_TYPE_STRING,
           NULL),
@@ -117,7 +117,7 @@ GST_START_TEST (serialize_static_record)
       "enum", GST_TYPE_STRUCTURE, gst_structure_new ("value",
           "type", G_TYPE_GTYPE, GST_TYPE_PAD_DIRECTION,
           NULL),
-      NULL));
+      NULL);
   /* *INDENT-ON* */
 
   save_messages = TRUE;