ges: Refactor the way we plug converters in effects

Stopping to do it at the bin description level but properly
plugging them where they are needed and cleanly ghosting the pads
where it makes most sense.

This introduces support for GES to request pads on the most upstream
element in case no static pad can be ghosted.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-editing-services/-/merge_requests/187>

ges/ges-command-line-formatter.c

@@ -157,7 +157,7 @@ static GESCommandLineOption options[] = {
       },
       {
         "inpoint", "i", GST_TYPE_CLOCK_TIME, NULL,
-        "Implies that the effect has a internal content"
+        "Implies that the effect has 'internal content'"
         "(see [ges_track_element_set_has_internal_source](ges_track_element_set_has_internal_source))",
       },
       {

ges/ges-effect-asset.c

@@ -49,7 +49,7 @@ _fill_track_type (GESAsset * asset)
   gchar *bin_desc;
   const gchar *id = ges_asset_get_id (asset);
 
-  bin_desc = ges_effect_assect_id_get_type_and_bindesc (id, &ttype, NULL);
+  bin_desc = ges_effect_asset_id_get_type_and_bindesc (id, &ttype, NULL);
 
   if (bin_desc) {
     ges_track_element_asset_set_track_type (GES_TRACK_ELEMENT_ASSET (asset),
@@ -110,35 +110,282 @@ ges_effect_asset_class_init (GESEffectAssetClass * klass)
   asset_class->extract = _extract;
 }
 
+static gboolean
+find_compatible_pads (GstElement * bin, const gchar * bin_desc,
+    GstElement * child, GstCaps * valid_caps, GstPad ** srcpad,
+    GList ** sinkpads, GList ** elems_with_reqsink,
+    GList ** elems_with_reqsrc, GError ** error)
+{
+  GList *tmp, *tmptemplate;
+
+  for (tmp = child->pads; tmp; tmp = tmp->next) {
+    GstCaps *caps;
+    GstPad *pad = tmp->data;
+
+    if (GST_PAD_PEER (pad))
+      continue;
+
+    if (GST_PAD_IS_SRC (pad) && *srcpad) {
+      g_set_error (error, GES_ERROR, GES_ERROR_INVALID_EFFECT_BIN_DESCRIPTION,
+          "More than 1 source pad in effect '%s', that is not handled",
+          bin_desc);
+      return FALSE;
+    }
+
+    caps = gst_pad_query_caps (pad, NULL);
+    if (gst_caps_can_intersect (caps, valid_caps)) {
+      if (GST_PAD_IS_SINK (pad))
+        *sinkpads = g_list_append (*sinkpads, gst_object_ref (pad));
+      else
+        *srcpad = gst_object_ref (pad);
+    } else {
+      GST_LOG_OBJECT (pad, "Can't link pad %" GST_PTR_FORMAT, caps);
+    }
+
+    gst_caps_unref (caps);
+  }
+
+  tmptemplate =
+      gst_element_class_get_pad_template_list (GST_ELEMENT_GET_CLASS (child));
+  for (; tmptemplate; tmptemplate = tmptemplate->next) {
+    GstPadTemplate *template = tmptemplate->data;
+
+    if (template->direction == GST_PAD_SINK) {
+      if (template->presence == GST_PAD_REQUEST)
+        *elems_with_reqsink = g_list_append (*elems_with_reqsink, child);
+    }
+  }
+
+  return TRUE;
+}
+
+static GstPad *
+request_pad (GstElement * element, GstPadDirection direction)
+{
+  GstPad *pad = NULL;
+  GList *templates;
+
+  templates = gst_element_class_get_pad_template_list
+      (GST_ELEMENT_GET_CLASS (element));
+
+  for (; templates; templates = templates->next) {
+    GstPadTemplate *templ = (GstPadTemplate *) templates->data;
+
+    GST_LOG_OBJECT (element, "Trying template %s",
+        GST_PAD_TEMPLATE_NAME_TEMPLATE (templ));
+
+    if ((GST_PAD_TEMPLATE_DIRECTION (templ) == direction) &&
+        (GST_PAD_TEMPLATE_PRESENCE (templ) == GST_PAD_REQUEST)) {
+      pad =
+          gst_element_get_request_pad (element,
+          GST_PAD_TEMPLATE_NAME_TEMPLATE (templ));
+      if (pad)
+        break;
+    }
+  }
+
+  return pad;
+}
+
+static GstPad *
+get_pad_from_elements_with_request_pad (GstElement * effect,
+    const gchar * bin_desc, GList * requestable, GstPadDirection direction,
+    GError ** error)
+{
+  GstElement *request_element = NULL;
+
+  if (!requestable) {
+    g_set_error (error, GES_ERROR, GES_ERROR_INVALID_EFFECT_BIN_DESCRIPTION,
+        "No %spads available for effect: %s",
+        (direction == GST_PAD_SRC) ? "src" : "sink", bin_desc);
+
+    return NULL;
+  }
+
+  request_element = requestable->data;
+  if (requestable->next) {
+    GstIterator *it = gst_bin_iterate_sorted (GST_BIN (effect));
+    GValue v;
+
+    while (gst_iterator_next (it, &v) != GST_ITERATOR_DONE) {
+      GstElement *tmpe = g_value_get_object (&v);
+
+      if (g_list_find (requestable, tmpe)) {
+        request_element = tmpe;
+        if (direction == GST_PAD_SRC) {
+          break;
+        }
+      }
+      g_value_reset (&v);
+    }
+    gst_iterator_free (it);
+  }
+
+  return request_pad (request_element, direction);
+}
+
+static gboolean
+ghost_pad (GstElement * effect, const gchar * bin_desc, GstPad * pad,
+    gint n_pad, const gchar * converter_str, GError ** error)
+{
+  gchar *name;
+  GstPad *peer, *ghosted;
+  GstPadLinkReturn lret;
+  GstElement *converter;
+
+  if (!converter_str) {
+    ghosted = pad;
+    goto ghost;
+  }
+
+  converter = gst_parse_bin_from_description_full (converter_str, TRUE, NULL,
+      GST_PARSE_FLAG_NO_SINGLE_ELEMENT_BINS | GST_PARSE_FLAG_PLACE_IN_BIN,
+      error);
+
+  if (!converter) {
+    GST_ERROR_OBJECT (effect, "Could not create converter '%s'", converter_str);
+    return FALSE;
+  }
+
+  peer =
+      GST_PAD_IS_SINK (pad) ? converter->srcpads->data : converter->sinkpads->
+      data;
+
+  gst_bin_add (GST_BIN (effect), converter);
+  lret =
+      gst_pad_link (GST_PAD_IS_SINK (pad) ? peer : pad,
+      GST_PAD_IS_SINK (pad) ? pad : peer);
+
+  if (lret != GST_PAD_LINK_OK) {
+    gst_object_unref (converter);
+    g_set_error (error, GES_ERROR, GES_ERROR_INVALID_EFFECT_BIN_DESCRIPTION,
+        "Effect %s can not link converter %s with %s", bin_desc, converter_str,
+        gst_pad_link_get_name (lret));
+    return FALSE;
+  }
+
+  ghosted =
+      GST_PAD_IS_SRC (pad) ? converter->srcpads->data : converter->sinkpads->
+      data;
+
+ghost:
+
+  if (GST_PAD_IS_SINK (pad))
+    name = g_strdup_printf ("sink_%d", n_pad);
+  else
+    name = g_strdup_printf ("src");
+
+  gst_element_add_pad (effect, gst_ghost_pad_new (name, ghosted));
+  g_free (name);
+
+  return TRUE;
+}
+
+GstElement *
+ges_effect_from_description (const gchar * bin_desc, GESTrackType type,
+    GError ** error)
+{
+
+  gint n_sink = 0;
+  GstPad *srcpad = NULL;
+  GstCaps *valid_caps = NULL;
+  const gchar *converter_str = NULL;
+  GList *tmp, *sinkpads = NULL, *elems_with_reqsink = NULL,
+      *elems_with_reqsrc = NULL;
+  GstElement *effect =
+      gst_parse_bin_from_description_full (bin_desc, FALSE, NULL,
+      GST_PARSE_FLAG_PLACE_IN_BIN | GST_PARSE_FLAG_FATAL_ERRORS, error);
+
+  if (!effect) {
+    GST_ERROR ("An error occurred while creating: %s",
+        (error && *error) ? (*error)->message : "Unknown error");
+    goto err;
+  }
+
+  if (type == GES_TRACK_TYPE_VIDEO) {
+    valid_caps = gst_caps_from_string ("video/x-raw(ANY)");
+    converter_str = "videoconvert";
+  } else if (type == GES_TRACK_TYPE_AUDIO) {
+    valid_caps = gst_caps_from_string ("audio/x-raw(ANY)");
+    converter_str = "audioconvert ! audioresample ! audioconvert";
+  } else {
+    valid_caps = gst_caps_new_any ();
+  }
+
+  for (tmp = GST_BIN_CHILDREN (effect); tmp; tmp = tmp->next) {
+    if (!find_compatible_pads (effect, bin_desc, tmp->data, valid_caps, &srcpad,
+            &sinkpads, &elems_with_reqsink, &elems_with_reqsrc, error))
+      goto err;
+  }
+
+  if (!sinkpads) {
+    GstPad *sinkpad = get_pad_from_elements_with_request_pad (effect, bin_desc,
+        elems_with_reqsink, GST_PAD_SINK, error);
+    if (!sinkpad)
+      goto err;
+    sinkpads = g_list_append (sinkpads, sinkpad);
+  }
+
+  if (!srcpad) {
+    srcpad = get_pad_from_elements_with_request_pad (effect, bin_desc,
+        elems_with_reqsrc, GST_PAD_SRC, error);
+    if (!srcpad)
+      goto err;
+  }
+
+  for (tmp = sinkpads; tmp; tmp = tmp->next) {
+    if (!ghost_pad (effect, bin_desc, tmp->data, n_sink, converter_str, error))
+      goto err;
+    n_sink++;
+  }
+
+  if (!ghost_pad (effect, bin_desc, srcpad, 0, converter_str, error))
+    goto err;
+
+done:
+  g_list_free (elems_with_reqsink);
+  g_list_free (elems_with_reqsrc);
+  g_list_free_full (sinkpads, gst_object_unref);
+  gst_clear_caps (&valid_caps);
+  gst_clear_object (&srcpad);
+
+  return effect;
+
+err:
+  gst_clear_object (&effect);
+  goto done;
+}
+
 gchar *
-ges_effect_assect_id_get_type_and_bindesc (const char *id,
+ges_effect_asset_id_get_type_and_bindesc (const char *id,
     GESTrackType * track_type, GError ** error)
 {
   GList *tmp;
   GstElement *effect;
   gchar **typebin_desc = NULL;
+  const gchar *user_bindesc;
   gchar *bindesc = NULL;
 
   *track_type = GES_TRACK_TYPE_UNKNOWN;
   typebin_desc = g_strsplit (id, " ", 2);
   if (!g_strcmp0 (typebin_desc[0], "audio")) {
     *track_type = GES_TRACK_TYPE_AUDIO;
-    bindesc = g_strdup (typebin_desc[1]);
+    user_bindesc = typebin_desc[1];
   } else if (!g_strcmp0 (typebin_desc[0], "video")) {
     *track_type = GES_TRACK_TYPE_VIDEO;
-    bindesc = g_strdup (typebin_desc[1]);
+    user_bindesc = typebin_desc[1];
   } else {
-    bindesc = g_strdup (id);
+    *track_type = GES_TRACK_TYPE_UNKNOWN;
+    user_bindesc = id;
   }
 
+  bindesc = g_strdup (user_bindesc);
   g_strfreev (typebin_desc);
 
   effect = gst_parse_bin_from_description (bindesc, TRUE, error);
   if (effect == NULL) {
+    GST_ERROR ("Could not create element from: %s", bindesc);
     g_free (bindesc);
-
-    GST_ERROR ("Could not create element from: %s", id);
-
     return NULL;
   }
 
@@ -171,7 +418,15 @@ ges_effect_assect_id_get_type_and_bindesc (const char *id,
     *track_type = GES_TRACK_TYPE_VIDEO;
     GST_ERROR ("Could not determine track type for %s, defaulting to video",
         id);
+
   }
 
+  if (!(effect = ges_effect_from_description (bindesc, *track_type, error))) {
+    g_free (bindesc);
+
+    return NULL;
+  }
+  gst_object_unref (effect);
+
   return bindesc;
 }

ges/ges-effect-clip.c

@@ -80,7 +80,7 @@ extractable_get_parameters_from_id (const gchar * id, guint * n_params)
 
   for (i = 0; effects_desc[i] && i < 2; i++) {
     bin_desc =
-        ges_effect_assect_id_get_type_and_bindesc (effects_desc[i], &ttype,
+        ges_effect_asset_id_get_type_and_bindesc (effects_desc[i], &ttype,
         NULL);
 
     if (ttype == GES_TRACK_TYPE_AUDIO) {

ges/ges-effect.c

@@ -20,12 +20,17 @@
 /**
  * SECTION:geseffect
  * @title: GESEffect
- * @short_description: adds an effect build from a parse-launch style
- * bin description to a stream in a GESSourceClip or a GESLayer
+ * @short_description: adds an effect build from a parse-launch style bin
+ * description to a stream in a GESSourceClip or a GESLayer
  *
- * Currently we only support effects with 1 sinkpad and 1 sourcepad
- * with the exception of `gesaudiomixer` and `gescompositor` which
- * can be used as effects.
+ * Currently we only support effects with N sinkpads and one single srcpad.
+ * Apart from `gesaudiomixer` and `gescompositor` which can be used as effects
+ * and where sinkpads will be requested as needed based on the timeline topology
+ * GES will always request at most one sinkpad per effect (when required).
+ *
+ * > Note: GES always adds converters (`audioconvert ! audioresample !
+ * > audioconvert` for audio effects and `videoconvert` for video effects) to
+ * > make it simpler for end users.
  */
 #ifdef HAVE_CONFIG_H
 #include "config.h"
@@ -67,7 +72,7 @@ extractable_check_id (GType type, const gchar * id, GError ** error)
   gchar *bin_desc, *real_id;
   GESTrackType ttype;
 
-  bin_desc = ges_effect_assect_id_get_type_and_bindesc (id, &ttype, error);
+  bin_desc = ges_effect_asset_id_get_type_and_bindesc (id, &ttype, error);
 
   if (bin_desc == NULL)
     return NULL;
@@ -92,7 +97,7 @@ extractable_get_parameters_from_id (const gchar * id, guint * n_params)
   gchar *bin_desc;
   GESTrackType ttype;
 
-  bin_desc = ges_effect_assect_id_get_type_and_bindesc (id, &ttype, NULL);
+  bin_desc = ges_effect_asset_id_get_type_and_bindesc (id, &ttype, NULL);
 
   params[0].name = "bin-description";
   g_value_init (&params[0].value, G_TYPE_STRING);
@@ -221,37 +226,6 @@ ges_effect_finalize (GObject * object)
   G_OBJECT_CLASS (ges_effect_parent_class)->finalize (object);
 }
 
-static void
-ghost_compatible_pads (GstElement * bin, GstElement * child,
-    GstCaps * valid_caps, gint * n_src, gint * n_sink)
-{
-  GList *tmp;
-
-  for (tmp = child->pads; tmp; tmp = tmp->next) {
-    GstCaps *caps;
-    GstPad *pad = tmp->data;
-
-    if (GST_PAD_PEER (pad))
-      continue;
-
-    caps = gst_pad_query_caps (pad, NULL);
-
-    if (gst_caps_can_intersect (caps, valid_caps)) {
-      gchar *name =
-          g_strdup_printf ("%s_%d", GST_PAD_IS_SINK (pad) ? "sink" : "src",
-          GST_PAD_IS_SINK (pad) ? *n_sink++ : *n_src++);
-
-      GST_DEBUG_OBJECT (bin, "Ghosting pad: %" GST_PTR_FORMAT, pad);
-      gst_element_add_pad (GST_ELEMENT (bin), gst_ghost_pad_new (name, pad));
-      g_free (name);
-    } else {
-      GST_DEBUG_OBJECT (pad, "Can't ghost pad %" GST_PTR_FORMAT, caps);
-    }
-
-    gst_caps_unref (caps);
-  }
-}
-
 static gdouble
 _get_rate_factor (GESBaseEffect * effect, GHashTable * rate_values)
 {
@@ -321,14 +295,11 @@ _rate_sink_to_source (GESBaseEffect * effect, GstClockTime time,
 static GstElement *
 ges_effect_create_element (GESTrackElement * object)
 {
-  GESBaseEffect *base_effect = GES_BASE_EFFECT (object);
-  GESEffectClass *class;
   GList *tmp;
+  GESEffectClass *class;
   GstElement *effect;
-  gchar *bin_desc;
-  GstCaps *valid_caps;
-  gint n_src = 0, n_sink = 0;
   gboolean is_rate_effect = FALSE;
+  GESBaseEffect *base_effect = GES_BASE_EFFECT (object);
 
   GError *error = NULL;
   GESEffect *self = GES_EFFECT (object);
@@ -341,39 +312,15 @@ ges_effect_create_element (GESTrackElement * object)
       !g_strcmp0 (self->priv->bin_description, "gescompositor"))
     return gst_element_factory_make (self->priv->bin_description, NULL);
 
-  if (type == GES_TRACK_TYPE_VIDEO) {
-    bin_desc = g_strconcat ("videoconvert name=pre_video_convert ! ",
-        self->priv->bin_description, " ! videoconvert name=post_video_convert",
-        NULL);
-    valid_caps = gst_caps_from_string ("video/x-raw(ANY)");
-  } else if (type == GES_TRACK_TYPE_AUDIO) {
-    bin_desc =
-        g_strconcat ("audioconvert ! audioresample !",
-        self->priv->bin_description, NULL);
-    valid_caps = gst_caps_from_string ("audio/x-raw(ANY)");
-  } else {
-    g_assert_not_reached ();
-  }
-
-  effect = gst_parse_bin_from_description (bin_desc, FALSE, &error);
-  g_free (bin_desc);
+  effect =
+      ges_effect_from_description (self->priv->bin_description, type, &error);
   if (error != NULL) {
-    GST_ERROR ("An error occured while creating the GstElement: %s",
+    GST_ERROR ("An error occurred while creating the GstElement: %s",
         error->message);
     g_error_free (error);
     goto fail;
   }
 
-  for (tmp = GST_BIN_CHILDREN (effect); tmp; tmp = tmp->next) {
-    ghost_compatible_pads (effect, tmp->data, valid_caps, &n_src, &n_sink);
-
-    if (n_src > 1) {
-      GST_ERROR ("More than 1 source pad in the effect, that is not possible");
-
-      goto fail;
-    }
-  }
-
   ges_track_element_add_children_props (object, effect, NULL,
       blacklisted_factories, NULL);
 
@@ -394,7 +341,6 @@ ges_effect_create_element (GESTrackElement * object)
     GST_ERROR_OBJECT (object, "Failed to set rate translation functions");
 
 done:
-  gst_clear_caps (&valid_caps);
 
   return effect;
 

ges/ges-gerror.h

@@ -60,6 +60,7 @@ typedef enum
   GES_ERROR_NEGATIVE_TIME,
   GES_ERROR_NOT_ENOUGH_INTERNAL_CONTENT,
   GES_ERROR_INVALID_OVERLAP_IN_TRACK,
+  GES_ERROR_INVALID_EFFECT_BIN_DESCRIPTION,
 } GESError;
 
 G_END_DECLS

ges/ges-internal.h

@@ -219,9 +219,9 @@ G_GNUC_INTERNAL gboolean
 ges_asset_request_id_update (GESAsset *asset, gchar **proposed_id,
     GError *error);
 G_GNUC_INTERNAL gchar *
-ges_effect_assect_id_get_type_and_bindesc (const char    *id,
-                                           GESTrackType  *track_type,
-                                           GError       **error);
+ges_effect_asset_id_get_type_and_bindesc (const char    *id,
+                                          GESTrackType  *track_type,
+                                          GError       **error);
 
 G_GNUC_INTERNAL void _ges_uri_asset_cleanup (void);
 
@@ -488,7 +488,7 @@ G_GNUC_INTERNAL GESTitleSource     * ges_title_source_new      (void);
 G_GNUC_INTERNAL GESVideoTestSource * ges_video_test_source_new (void);
 
 /****************************************************
- *                GESBaseEffect                     *
+ *                GES*Effect                     *
  ****************************************************/
 G_GNUC_INTERNAL gchar *
 ges_base_effect_get_time_property_name        (GESBaseEffect * effect,
@@ -504,6 +504,10 @@ G_GNUC_INTERNAL GstClockTime
 ges_base_effect_translate_sink_to_source_time (GESBaseEffect * effect,
                                                GstClockTime time,
                                                GHashTable * time_property_values);
+G_GNUC_INTERNAL GstElement *
+ges_effect_from_description                   (const gchar *bin_desc,
+                                               GESTrackType type,
+                                               GError **error);
 
 /****************************************************
  *              GESTimelineElement                  *

tests/check/meson.build

@@ -82,6 +82,7 @@ if gstvalidate_dep.found()
     'seek_with_stop': true,
     'seek_with_stop.check_clock_sync': true,
     'edit_while_seeked_with_stop': true,
+    'complex_effect_bin_desc': true,
   }
 
   foreach scenario, is_validatetest: scenarios
@@ -107,7 +108,7 @@ if gstvalidate_dep.found()
 endif
 
 if build_gir
-  # Make sure to use the subproject gst-validate-launcher if avalaible.
+  # Make sure to use the subproject gst-validate-launcher if available.
   if gstvalidate_dep.found() and gstvalidate_dep.type_name() == 'internal'
     runtests = subproject('gst-devtools').get_variable('launcher')
   else

tests/check/scenarios/complex_effect_bin_desc.validatetest

@@ -0,0 +1,24 @@
+# Check that we can have effect with sources integrated where GES will request a pad on some elements
+# In that example, we are blending a green rectangle on top of a blue GESVideoTestSource using an effect
+meta,
+    tool = "ges-launch-$(gst_api_version)",
+    handles-states=true,
+    args = {
+        "--track-types", "video",
+        "--videosink", "$(videosink) name=videosink",
+        "--video-caps", "video/x-raw, format=I420, width=1280, height=720, framerate=30/1, chroma-site=jpeg, colorimetry=bt601",
+    },
+    configs = {
+        "$(validateflow), pad=videosink:sink, buffers-checksum=true, ignored-fields=\"stream-start={stream-id,group-id,stream}, segment={position,}\", ignored-event-types={gap}",
+    }
+
+
+add-clip, name=c0, asset-id=GESTestClip, layer-priority=0, type=GESTestClip, start=0, duration=0.1
+set-child-properties, element-name=c0, pattern=blue
+
+container-add-child,
+    container-name=c0,
+    asset-id="videotestsrc pattern=green ! video/x-raw,width=640,height=360 ! compositor sink_0::xpos=320 sink_0::ypos=180 sink_0::zorder=500",
+    child-type=GESEffect,
+    child-name=effect
+play

tests/check/scenarios/complex_effect_bin_desc/flow-expectations/log-videosink-sink-expected

@@ -0,0 +1,9 @@
+event stream-start: GstEventStreamStart, flags=(GstStreamFlags)GST_STREAM_FLAG_NONE;
+event caps: video/x-raw, format=(string)I420, width=(int)1280, height=(int)720, framerate=(fraction)30/1, chroma-site=(string)jpeg, colorimetry=(string)bt601;
+event segment: format=TIME, start=0:00:00.000000000, offset=0:00:00.000000000, stop=0:00:00.100000000, flags=0x01, time=0:00:00.000000000, base=0:00:00.000000000
+buffer: checksum=d2d49287a7d0ddd7b5fadbb60c3220623119fea8, pts=0:00:00.000000000, dur=0:00:00.033333333
+buffer: checksum=d2d49287a7d0ddd7b5fadbb60c3220623119fea8, pts=0:00:00.033333333, dur=0:00:00.033333334
+buffer: checksum=d2d49287a7d0ddd7b5fadbb60c3220623119fea8, pts=0:00:00.066666667, dur=0:00:00.033333333
+event segment: format=TIME, start=0:00:00.100000000, offset=0:00:00.000000000, stop=0:00:00.100000001, flags=0x01, time=0:00:00.100000000, base=0:00:00.100000000
+buffer: checksum=b4a126ab26f314a74ef860a9af457327a28d680b, pts=0:00:00.100000000, dur=0:00:00.000000001
+event eos: (no structure)