parse: refactor to make use of gst_element_factory_make_with_properties

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>
2024-11-26 19:51:11 +00:00 · 2022-09-13 23:16:08 +02:00 · 2022-09-13 23:16:08 +02:00 · c2f890aba7
commit c2f890aba7
parent 095bca9bdb
3 changed files with 281 additions and 88 deletions
--- a/subprojects/gstreamer/gst/parse/grammar.y.in
+++ b/subprojects/gstreamer/gst/parse/grammar.y.in
@ -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));
-    }
-  }
+  return pos;
+}

-  if (pspec != NULL && target != NULL) {
+static gboolean
+collect_value (GParamSpec *pspec, gchar *value_str, GValue *v)
+{
  gboolean got_value = FALSE;

-    value_type = pspec->value_type;
+  GST_CAT_ERROR (GST_CAT_PIPELINE, "parsing property %s as a %s",
+      pspec->name, g_type_name (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))
+  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 (value_type, GST_TYPE_ELEMENT)) {
+  else if (g_type_is_a (pspec->value_type, GST_TYPE_ELEMENT)) {
     GstElement *bin;

-       bin = gst_parse_bin_from_description_full (pos, TRUE, NULL,
+     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);
+       g_value_set_object (v, bin);
       got_value = TRUE;
     }
  }
-    if (!got_value)
-      goto error;
-    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;
+  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);
+element: IDENTIFIER              {
+            $$ = gst_parse_element_new();
+            $$->factory_name = $1;
                                              }
-						gst_parse_strfree ($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 ();
+	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 = $1? gst_object_ref($1) : NULL;
-						$$->last.element = $1? gst_object_ref($1) : NULL;
+              $$->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 = $1 ? g_slist_prepend (NULL, $1) : 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 */

--- a/subprojects/gstreamer/gst/parse/types.h
+++ b/subprojects/gstreamer/gst/parse/types.h
@ -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
--- a/subprojects/gstreamer/tests/check/pipelines/parse-launch.c
+++ b/subprojects/gstreamer/tests/check/pipelines/parse-launch.c
@ -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);
  }
 }