parse: refactor to make use of gst_element_factory_make_with_properties
authorMathieu Duponchelle <mathieu@centricular.com>
Tue, 13 Sep 2022 21:16:08 +0000 (23:16 +0200)
committerGStreamer Marge Bot <gitlab-merge-bot@gstreamer-foundation.org>
Wed, 19 Oct 2022 11:21:04 +0000 (11:21 +0000)
Instead of creating the element first, then setting properties and
presets, we gather those and construct the element with the properties.

This means users of gst_parse_launch can now set construct-only
properties.

Fixes https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1380

Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/3026>

subprojects/gstreamer/gst/parse/grammar.y.in
subprojects/gstreamer/gst/parse/types.h
subprojects/gstreamer/tests/check/pipelines/parse-launch.c

index e544f5c..7d95927 100644 (file)
@@ -41,6 +41,7 @@
 static guint __strings;
 static guint __links;
 static guint __chains;
+static guint __elements;
 gchar *
 __gst_parse_strdup (gchar *org)
 {
@@ -96,6 +97,24 @@ __gst_parse_chain_free (chain_t *data)
   __chains--;
 }
 
+element_t *
+__gst_parse_element_new (void)
+{
+  element_t *ret;
+  __elements++;
+  ret = g_slice_new0 (element_t);
+  /* g_print ("@%p: ALLOCATED ELEMENT (%3u):\n", ret, __elements); */
+  return ret;
+}
+void
+__gst_parse_element_free (element_t *data)
+{
+  /* g_print ("@%p: FREEING ELEMENT   (%3u):\n", data, __elements - 1); */
+  g_slice_free (element_t, data);
+  g_return_if_fail (__elements > 0);
+  __elements--;
+}
+
 #endif /* __GST_PARSE_TRACE */
 
 /*******************************************************************************************
@@ -367,17 +386,9 @@ error:
   goto out;
 }
 
-static void gst_parse_element_set (gchar *value, GstElement *element, graph_t *graph)
-{
-  GParamSpec *pspec = NULL;
+static gchar *
+gst_parse_split_assignment (gchar *value) {
   gchar *pos = value;
-  GValue v = { 0, };
-  GObject *target = NULL;
-  GType value_type;
-
-  /* do nothing if assignment is for missing element */
-  if (element == NULL)
-    goto out;
 
   /* parse the string, so the property name is null-terminated and pos points
      to the beginning of the value */
@@ -398,62 +409,32 @@ static void gst_parse_element_set (gchar *value, GstElement *element, graph_t *g
   }
   gst_parse_unescape (pos);
 
-  if (GST_IS_CHILD_PROXY (element) && strstr (value, "::") != NULL) {
-    if (!gst_child_proxy_lookup (GST_CHILD_PROXY (element), value, &target, &pspec)) {
-      /* do a delayed set */
-      gst_parse_add_delayed_set (element, value, pos);
-    }
-  } else {
-    pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (element), value);
-    if (pspec != NULL) {
-      target = G_OBJECT (g_object_ref (element));
-      GST_CAT_LOG_OBJECT (GST_CAT_PIPELINE, target, "found %s property", value);
-    } else {
-      SET_ERROR (graph->error, GST_PARSE_ERROR_NO_SUCH_PROPERTY, \
-          _("no property \"%s\" in element \"%s\""), value, \
-          GST_ELEMENT_NAME (element));
-    }
-  }
-
-  if (pspec != NULL && target != NULL) {
-    gboolean got_value = FALSE;
-
-    value_type = pspec->value_type;
-
-    GST_CAT_LOG_OBJECT (GST_CAT_PIPELINE, element, "parsing property %s as a %s",
-        pspec->name, g_type_name (value_type));
-
-    g_value_init (&v, value_type);
-    if (gst_value_deserialize_with_pspec (&v, pos, pspec))
-      got_value = TRUE;
-    else if (g_type_is_a (value_type, GST_TYPE_ELEMENT)) {
-       GstElement *bin;
+  return pos;
+}
 
-       bin = gst_parse_bin_from_description_full (pos, TRUE, NULL,
-           GST_PARSE_FLAG_NO_SINGLE_ELEMENT_BINS | GST_PARSE_FLAG_PLACE_IN_BIN, NULL);
-       if (bin) {
-         g_value_set_object (&v, bin);
-         got_value = TRUE;
-       }
-    }
-    if (!got_value)
-      goto error;
-    g_object_set_property (target, pspec->name, &v);
+static gboolean
+collect_value (GParamSpec *pspec, gchar *value_str, GValue *v)
+{
+  gboolean got_value = FALSE;
+
+  GST_CAT_ERROR (GST_CAT_PIPELINE, "parsing property %s as a %s",
+      pspec->name, g_type_name (pspec->value_type));
+
+  g_value_init (v, pspec->value_type);
+  if (gst_value_deserialize_with_pspec (v, value_str, pspec))
+    got_value = TRUE;
+  else if (g_type_is_a (pspec->value_type, GST_TYPE_ELEMENT)) {
+     GstElement *bin;
+
+     bin = gst_parse_bin_from_description_full (value_str, TRUE, NULL,
+         GST_PARSE_FLAG_NO_SINGLE_ELEMENT_BINS | GST_PARSE_FLAG_PLACE_IN_BIN, NULL);
+     if (bin) {
+       g_value_set_object (v, bin);
+       got_value = TRUE;
+     }
   }
 
-out:
-  gst_parse_strfree (value);
-  if (G_IS_VALUE (&v))
-    g_value_unset (&v);
-  if (target)
-    gst_object_unref (target);
-  return;
-
-error:
-  SET_ERROR (graph->error, GST_PARSE_ERROR_COULD_NOT_SET_PROPERTY,
-         _("could not set property \"%s\" in element \"%s\" to \"%s\""),
-        value, GST_ELEMENT_NAME (element), pos);
-  goto out;
+  return got_value;
 }
 
 static void gst_parse_element_preset (gchar *value, GstElement *element, graph_t *graph)
@@ -474,7 +455,6 @@ static void gst_parse_element_preset (gchar *value, GstElement *element, graph_t
     goto error;
 
 out:
-  gst_parse_strfree (value);
   return;
 
 not_a_preset:
@@ -490,6 +470,187 @@ error:
   goto out;
 }
 
+typedef struct
+{
+  gchar *name;
+  gchar *value;
+} proxied_property_t;
+
+static void
+proxied_property_free (proxied_property_t *pp) {
+  g_slice_free (proxied_property_t, pp);
+}
+
+static GstElement * gst_parse_element_make (graph_t *graph, element_t *data) {
+  GstElementFactory *loaded_factory;
+  GstElementFactory *factory = gst_element_factory_find (data->factory_name);
+  GObjectClass *klass;
+  GParamSpec *pspec = NULL;
+  GSList *tmp;
+  gboolean is_proxy;
+  GSList *proxied = NULL;
+  guint n_params = 0;
+  guint n_params_alloc = 16;
+  const gchar **names_array;
+  GValue *values_array;
+  GstElement *ret = NULL;
+
+  if (!factory) {
+               SET_ERROR (graph->error, GST_PARSE_ERROR_NO_SUCH_ELEMENT, _("no element \"%s\""), data->factory_name);
+    return NULL;
+  }
+
+  loaded_factory =
+    GST_ELEMENT_FACTORY (gst_plugin_feature_load (GST_PLUGIN_FEATURE
+        (factory)));
+
+  gst_object_unref (factory);
+
+  klass = g_type_class_ref (gst_element_factory_get_element_type (loaded_factory));
+
+  is_proxy = g_type_is_a (gst_element_factory_get_element_type (loaded_factory), GST_TYPE_CHILD_PROXY);
+
+  names_array = g_new0 (const gchar *, n_params_alloc);
+  values_array = g_new0 (GValue, n_params_alloc);
+
+  for (tmp = data->values; tmp; tmp = tmp->next) {
+    gchar *name = tmp->data;
+    gchar *value = gst_parse_split_assignment (tmp->data);
+
+    if (is_proxy && strstr (name, "::") != NULL) {
+      proxied_property_t *pp = g_slice_new (proxied_property_t);
+      pp->name = name;
+      pp->value = value;
+      proxied = g_slist_prepend (proxied, pp);
+      continue;
+    }
+
+    pspec = g_object_class_find_property (klass, name);
+
+    if (pspec != NULL) {
+      if (G_UNLIKELY (n_params == n_params_alloc)) {
+        n_params_alloc *= 2u;
+        names_array =
+            g_realloc (names_array, sizeof (const gchar *) * n_params_alloc);
+        values_array = g_realloc (values_array, sizeof (GValue) * n_params_alloc);
+        memset (&values_array[n_params], 0,
+            sizeof (GValue) * (n_params_alloc - n_params));
+      }
+
+      if (!collect_value (pspec, value, &values_array[n_params])) {
+        SET_ERROR (graph->error, GST_PARSE_ERROR_COULD_NOT_SET_PROPERTY,
+               _("could not set property \"%s\" in element \"%s\" to \"%s\""),
+         name, data->factory_name, value);
+        g_value_unset (&values_array[n_params]);
+        goto done;
+      } else {
+        names_array[n_params] = name;
+      }
+
+      ++n_params;
+    } else {
+      SET_ERROR (graph->error, GST_PARSE_ERROR_NO_SUCH_PROPERTY, \
+          _("no property \"%s\" in element \"%s\""), name, \
+          data->factory_name);
+      goto done;
+    }
+  }
+
+  ret = gst_element_factory_create_with_properties (factory, n_params, names_array,
+      values_array);
+
+  for (tmp = proxied; tmp; tmp = tmp->next) {
+    GObject *target = NULL;
+    proxied_property_t *pp = tmp->data;
+
+    if (!gst_child_proxy_lookup (GST_CHILD_PROXY (ret), pp->name, &target, &pspec)) {
+      /* do a delayed set */
+      gst_parse_add_delayed_set (ret, pp->name, pp->value);
+    } else {
+      GValue v = { 0, };
+
+      if (!collect_value (pspec, pp->value, &v)) {
+        SET_ERROR (graph->error, GST_PARSE_ERROR_COULD_NOT_SET_PROPERTY,
+               _("could not set property \"%s\" in child of element \"%s\" to \"%s\""),
+         pp->name, data->factory_name, pp->value);
+        g_value_unset (&v);
+        goto done;
+      } else {
+        g_object_set_property (target, pspec->name, &v);
+        g_value_unset (&v);
+      }
+    }
+  }
+
+  for (tmp = data->presets; tmp; tmp = tmp->next) {
+    gst_parse_element_preset (tmp->data, ret, graph);
+  }
+
+done:
+  g_slist_free_full (proxied, (GDestroyNotify) proxied_property_free);
+  gst_object_unref (loaded_factory);
+  g_type_class_unref (klass);
+  g_free (names_array);
+  while (n_params--)
+    g_value_unset (&values_array[n_params]);
+  g_free (values_array);
+
+  return ret;
+}
+
+static void gst_parse_element_set (gchar *value, GstElement *element, graph_t *graph)
+{
+  GParamSpec *pspec = NULL;
+  gchar *pos;
+  GValue v = { 0, };
+  GObject *target = NULL;
+
+  /* do nothing if assignment is for missing element */
+  if (element == NULL)
+    goto out;
+
+  pos = gst_parse_split_assignment (value);
+
+  if (GST_IS_CHILD_PROXY (element) && strstr (value, "::") != NULL) {
+    if (!gst_child_proxy_lookup (GST_CHILD_PROXY (element), value, &target, &pspec)) {
+      /* do a delayed set */
+      gst_parse_add_delayed_set (element, value, pos);
+    }
+  } else {
+    pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (element), value);
+    if (pspec != NULL) {
+      target = G_OBJECT (g_object_ref (element));
+      GST_CAT_LOG_OBJECT (GST_CAT_PIPELINE, target, "found %s property", value);
+    } else {
+      SET_ERROR (graph->error, GST_PARSE_ERROR_NO_SUCH_PROPERTY, \
+          _("no property \"%s\" in element \"%s\""), value, \
+          GST_ELEMENT_NAME (element));
+    }
+  }
+
+  if (pspec != NULL && target != NULL) {
+    if (!collect_value (pspec, pos, &v)) {
+      goto error;
+    } else {
+      g_object_set_property (target, pspec->name, &v);
+    }
+  }
+
+out:
+  gst_parse_strfree (value);
+  if (G_IS_VALUE (&v))
+    g_value_unset (&v);
+  if (target)
+    gst_object_unref (target);
+  return;
+
+error:
+  SET_ERROR (graph->error, GST_PARSE_ERROR_COULD_NOT_SET_PROPERTY,
+         _("could not set property \"%s\" in element \"%s\" to \"%s\""),
+        value, GST_ELEMENT_NAME (element), pos);
+  goto out;
+}
+
 static void gst_parse_free_reference (reference_t *rr)
 {
   if(rr->element) gst_object_unref(rr->element);
@@ -517,6 +678,14 @@ static void gst_parse_free_chain (chain_t *ch)
   gst_parse_chain_free (ch);
 }
 
+static void gst_parse_free_element (element_t *el)
+{
+  g_slist_free_full (el->values, gst_parse_strfree);
+  g_slist_free_full (el->presets, gst_parse_strfree);
+  gst_parse_strfree (el->factory_name);
+  gst_parse_element_free (el);
+}
+
 static void gst_parse_free_delayed_link (DelayedLink *link)
 {
   g_free (link->src_pad);
@@ -787,7 +956,7 @@ static int yyerror (void *scanner, graph_t *graph, const char *s);
     chain_t *cc;
     link_t *ll;
     reference_t rr;
-    GstElement *ee;
+    element_t *ee;
     GSList *pp;
     graph_t *gg;
 }
@@ -815,7 +984,7 @@ static int yyerror (void *scanner, graph_t *graph, const char *s);
                  gst_parse_free_chain($$);     } <cc>
 %destructor {  gst_parse_free_link ($$);       } <ll>
 %destructor {  gst_parse_free_reference(&($$));} <rr>
-%destructor {  gst_object_unref ($$);          } <ee>
+%destructor {  gst_parse_free_element ($$);            } <ee>
 %destructor {  GSList *walk;
                for(walk=$$;walk;walk=walk->next)
                  gst_parse_strfree (walk->data);
@@ -845,17 +1014,16 @@ static int yyerror (void *scanner, graph_t *graph, const char *s);
 *      identity silence=false name=frodo
 *   (cont'd)
 **************************************************************/
-element:       IDENTIFIER                    { $$ = gst_element_factory_make ($1, NULL);
-                                               if ($$ == NULL) {
-                                                 add_missing_element(graph, $1);
-                                                 SET_ERROR (graph->error, GST_PARSE_ERROR_NO_SUCH_ELEMENT, _("no element \"%s\""), $1);
-                                               }
-                                               gst_parse_strfree ($1);
+element: IDENTIFIER              {
+            $$ = gst_parse_element_new();
+            $$->factory_name = $1;
                                               }
-       |       element PRESET            { gst_parse_element_preset ($2, $1, graph);
+  | element PRESET               {
+            $$->presets = g_slist_append ($$->presets, $2);
                                                $$ = $1;
                                              }
-       |       element ASSIGNMENT            { gst_parse_element_set ($2, $1, graph);
+       |       element ASSIGNMENT            {
+            $$->values = g_slist_append ($$->values, $2);
                                                $$ = $1;
                                              }
        ;
@@ -870,13 +1038,24 @@ element: IDENTIFIER                    { $$ = gst_element_factory_make ($1, NULL);
 *
 **************************************************************/
 elementary:
-       element                               { $$ = gst_parse_chain_new ();
-                                               /* g_print ("@%p: CHAINing elementary\n", $$); */
-                                               $$->first.element = $1? gst_object_ref($1) : NULL;
-                                               $$->last.element = $1? gst_object_ref($1) : NULL;
-                                               $$->first.name = $$->last.name = NULL;
-                                               $$->first.pads = $$->last.pads = NULL;
-                                               $$->elements = $1 ? g_slist_prepend (NULL, $1) : NULL;
+       element                               {
+            GstElement *element = NULL;
+
+            $$ = gst_parse_chain_new ();
+
+            if ($1 && !(element = gst_parse_element_make (graph, $1))) {
+                                                 add_missing_element(graph, $1->factory_name);
+            } else {
+              /* g_print ("@%p: CHAINing elementary\n", $$); */
+              $$->first.element = element ? gst_object_ref(element) : NULL;
+              $$->last.element = element ? gst_object_ref(element) : NULL;
+              $$->first.name = $$->last.name = NULL;
+              $$->first.pads = $$->last.pads = NULL;
+              $$->elements = element ? g_slist_prepend (NULL, element) : NULL;
+            }
+
+            gst_parse_free_element ($1);
+
                                              }
        | bin                                 { $$=$1; }
        ;
@@ -1198,7 +1377,7 @@ priv_gst_parse_launch (const gchar *str, GError **error, GstParseContext *ctx,
 
 #ifdef __GST_PARSE_TRACE
   GST_CAT_DEBUG (GST_CAT_PIPELINE, "TRACE: tracing enabled");
-  __strings = __chains = __links = 0;
+  __strings = __chains = __links = __elements = 0;
 #endif /* __GST_PARSE_TRACE */
 
   /* g_print("Now scanning: %s\n", str); */
@@ -1306,11 +1485,11 @@ priv_gst_parse_launch (const gchar *str, GError **error, GstParseContext *ctx,
 out:
 #ifdef __GST_PARSE_TRACE
   GST_CAT_DEBUG (GST_CAT_PIPELINE,
-      "TRACE: %u strings, %u chains and %u links left", __strings, __chains,
-      __links);
-  if (__strings || __chains || __links) {
-    g_warning ("TRACE: %u strings, %u chains and %u links left", __strings,
-        __chains, __links);
+      "TRACE: %u strings, %u chains, %u links and %u elements left", __strings, __chains,
+      __links, __elements);
+  if (__strings || __chains || __links || __elements) {
+    g_warning ("TRACE: %u strings, %u chains, %u links and %u elements left", __strings,
+        __chains, __links, __elements);
   }
 #endif /* __GST_PARSE_TRACE */
 
index 5dbb959..961af7e 100644 (file)
@@ -24,6 +24,13 @@ typedef struct {
   reference_t last;
 } chain_t;
 
+typedef struct {
+  gchar *factory_name;
+  GSList *values;
+  GSList *presets;
+} element_t;
+
+
 typedef struct _graph_t graph_t;
 struct _graph_t {
   chain_t *chain; /* links are supposed to be done now */
@@ -55,12 +62,16 @@ G_GNUC_INTERNAL  link_t *__gst_parse_link_new (void);
 G_GNUC_INTERNAL  void  __gst_parse_link_free (link_t *data);
 G_GNUC_INTERNAL  chain_t *__gst_parse_chain_new (void);
 G_GNUC_INTERNAL  void  __gst_parse_chain_free (chain_t *data);
+G_GNUC_INTERNAL  element_t *__gst_parse_element_new (void);
+G_GNUC_INTERNAL  void  __gst_parse_element_free (element_t *data);
 #  define gst_parse_strdup __gst_parse_strdup
 #  define gst_parse_strfree __gst_parse_strfree
 #  define gst_parse_link_new __gst_parse_link_new
 #  define gst_parse_link_free __gst_parse_link_free
 #  define gst_parse_chain_new __gst_parse_chain_new
 #  define gst_parse_chain_free __gst_parse_chain_free
+#  define gst_parse_element_new __gst_parse_element_new
+#  define gst_parse_element_free __gst_parse_element_free
 #else /* __GST_PARSE_TRACE */
 #  define gst_parse_strdup g_strdup
 #  define gst_parse_strfree g_free
@@ -68,6 +79,8 @@ G_GNUC_INTERNAL  void __gst_parse_chain_free (chain_t *data);
 #  define gst_parse_link_free(l) g_slice_free (link_t, l)
 #  define gst_parse_chain_new() g_slice_new0 (chain_t)
 #  define gst_parse_chain_free(c) g_slice_free (chain_t, c)
+#  define gst_parse_element_new() g_slice_new0 (element_t)
+#  define gst_parse_element_free(e) g_slice_free (element_t, e)
 #endif /* __GST_PARSE_TRACE */
 
 static inline void
index 58c2e53..d313ae4 100644 (file)
@@ -379,6 +379,7 @@ GST_START_TEST (expected_to_fail_pipes)
   const gchar **s;
 
   for (s = expected_failures; *s != NULL; s++) {
+    GST_ERROR ("Running %s", *s);
     expected_fail_pipe (*s);
   }
 }