From ec75b68984d5ae57722791ea97d292b862e149cd Mon Sep 17 00:00:00 2001 From: Stefan Sauer <ensonic@users.sf.net> Date: Thu, 21 Jan 2016 08:12:01 +0100 Subject: [PATCH] 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 | 129 +++++++++++++++--------------- gst/gsttracerrecord.h | 2 +- plugins/tracers/gstlatency.c | 4 +- plugins/tracers/gstrusage.c | 8 +- plugins/tracers/gststats.c | 31 ++++--- tests/check/gst/gsttracerrecord.c | 8 +- 6 files changed, 90 insertions(+), 92 deletions(-) diff --git a/gst/gsttracerrecord.c b/gst/gsttracerrecord.c index 54429a4298..9a99e23690 100644 --- a/gst/gsttracerrecord.c +++ b/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); -} - -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; - } + self->format = NULL; } 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 suffix will be used for the log records. The @spec must have a field for - * each value that gets logged where the field name is the value name. The field - * must be a nested structure describing the value. The sub structure must - * contain a field called 'type' of %G_TYPE_GTYPE that contains the GType of the - * value. + * The @name without the ".class" suffix will be used for the log records. + * There must be fields for each value that gets logged where the field name is + * the value name. The field must be a #GstStructure describing the value. The + * sub structure must contain a field called 'type' of %G_TYPE_GTYPE that + * contains the GType of the value. The resulting #GstTracerRecord will take + * 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 diff --git a/gst/gsttracerrecord.h b/gst/gsttracerrecord.h index 3cc791ba74..ac6515e7fa 100644 --- a/gst/gsttracerrecord.h +++ b/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, ...); diff --git a/plugins/tracers/gstlatency.c b/plugins/tracers/gstlatency.c index 72ac1e5896..af5746b12f 100644 --- a/plugins/tracers/gstlatency.c +++ b/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* */ } diff --git a/plugins/tracers/gstrusage.c b/plugins/tracers/gstrusage.c index 67c18feb78..c4110ecb8e 100644 --- a/plugins/tracers/gstrusage.c +++ b/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)); - tr_proc = gst_tracer_record_new (gst_structure_new ("proc-rusage.class", + NULL); + 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* */ } diff --git a/plugins/tracers/gststats.c b/plugins/tracers/gststats.c index 52aa456b42..d75a523d22 100644 --- a/plugins/tracers/gststats.c +++ b/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)); - tr_event = gst_tracer_record_new (gst_structure_new ("event.class", + NULL); + 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)); - tr_message = gst_tracer_record_new (gst_structure_new ("message.class", + NULL); + 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)); - tr_element_query = gst_tracer_record_new (gst_structure_new ( - "element-query.class", + NULL); + tr_element_query = gst_tracer_record_new ("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)); - tr_query = gst_tracer_record_new (gst_structure_new ("query.class", + NULL); + 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)); - tr_new_element = gst_tracer_record_new (gst_structure_new ( - "new-element.class", + NULL); + tr_new_element = gst_tracer_record_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)); - tr_new_pad = gst_tracer_record_new (gst_structure_new ("new-pad.class", + NULL); + 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* */ } diff --git a/tests/check/gst/gsttracerrecord.c b/tests/check/gst/gsttracerrecord.c index 483484f3f8..712d6f7e55 100644 --- a/tests/check/gst/gsttracerrecord.c +++ b/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;