ges: Properly position video sources in the scene by default

We try to do our best to have the video frames scaled the best way
to fill most space on the final frames, keeping aspect ratio. The user
can later on rescale or move the sources as usual but it makes the
default behaviour a better and more natural especially now that we
set default restriction caps to the video tracks.

And fix the unit test to take that change into account

ges/ges-video-source.c

@@ -27,7 +27,7 @@
  *
  * You can use the following children properties through the
  * #ges_track_element_set_child_property and alike set of methods:
- * 
+ *
  * - #gdouble `alpha`: The desired alpha for the stream.
  * - #gint `posx`: The desired x position for the stream.
  * - #gint `posy`: The desired y position for the stream
@@ -53,6 +53,7 @@
 #include "ges-video-source.h"
 #include "ges-layer.h"
 #include "gstframepositioner.h"
+#include "ges-extractable.h"
 
 #define parent_class ges_video_source_parent_class
 
@@ -62,8 +63,26 @@ struct _GESVideoSourcePrivate
   GstElement *capsfilter;
 };
 
-G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE (GESVideoSource, ges_video_source,
+static void
-    GES_TYPE_SOURCE);
+ges_video_source_set_asset (GESExtractable * extractable, GESAsset * asset)
+{
+  GESVideoSource *self = GES_VIDEO_SOURCE (extractable);
+
+  ges_video_source_get_natural_size (self,
+      &self->priv->positioner->natural_width,
+      &self->priv->positioner->natural_height);
+}
+
+static void
+ges_extractable_interface_init (GESExtractableInterface * iface)
+{
+  iface->set_asset = ges_video_source_set_asset;
+}
+
+G_DEFINE_ABSTRACT_TYPE_WITH_CODE (GESVideoSource, ges_video_source,
+    GES_TYPE_SOURCE, G_ADD_PRIVATE (GESVideoSource)
+    G_IMPLEMENT_INTERFACE (GES_TYPE_EXTRACTABLE,
+        ges_extractable_interface_init));
 
 /* TrackElement VMethods */
 
@@ -169,6 +188,10 @@ ges_video_source_create_element (GESTrackElement * trksrc)
   self->priv->positioner = GST_FRAME_POSITIONNER (positioner);
   self->priv->positioner->scale_in_compositor =
       !GES_VIDEO_SOURCE_GET_CLASS (self)->ABI.abi.disable_scale_in_compositor;
+  ges_video_source_get_natural_size (self,
+      &self->priv->positioner->natural_width,
+      &self->priv->positioner->natural_height);
+
   self->priv->capsfilter = capsfilter;
 
   return topbin;
@@ -226,7 +249,6 @@ ges_video_source_init (GESVideoSource * self)
   self->priv = ges_video_source_get_instance_private (self);
   self->priv->positioner = NULL;
   self->priv->capsfilter = NULL;
-
 }
 
 /**

ges/ges-video-uri-source.c

@@ -124,7 +124,11 @@ ges_video_uri_source_get_natural_size (GESVideoSource * source, gint * width,
   GstDiscovererStreamInfo *info;
   GESAsset *asset = ges_extractable_get_asset (GES_EXTRACTABLE (source));
 
-  g_assert (GES_IS_URI_SOURCE_ASSET (asset));
+  if (!asset) {
+    GST_DEBUG_OBJECT (source, "No asset set yet");
+    return FALSE;
+  }
+
   info = ges_uri_source_asset_get_stream_info (GES_URI_SOURCE_ASSET (asset));
 
   if (!GST_IS_DISCOVERER_VIDEO_INFO (info)) {
@@ -173,7 +177,7 @@ done:
   return TRUE;
 
 rotate:
-  GST_INFO_OBJECT (self, "Stream is rotated, taking that into account");
+  GST_INFO_OBJECT (source, "Stream is rotated, taking that into account");
   *width =
       gst_discoverer_video_info_get_height (GST_DISCOVERER_VIDEO_INFO (info));
   *height =
@@ -193,6 +197,7 @@ ges_extractable_check_id (GType type, const gchar * id, GError ** error)
 static void
 extractable_set_asset (GESExtractable * extractable, GESAsset * asset)
 {
+  GESExtractableInterface *piface, *iface;
   /* FIXME That should go into #GESTrackElement, but
    * some work is needed to make sure it works properly */
 
@@ -202,6 +207,11 @@ extractable_set_asset (GESExtractable * extractable, GESAsset * asset)
         ges_track_element_asset_get_track_type (GES_TRACK_ELEMENT_ASSET
             (asset)));
   }
+
+  iface = G_TYPE_INSTANCE_GET_INTERFACE (extractable, GES_TYPE_EXTRACTABLE,
+      GESExtractableInterface);
+  piface = g_type_interface_peek_parent (iface);
+  piface->set_asset (extractable, asset);
 }
 
 static void

ges/gstframepositioner.c

@@ -81,6 +81,56 @@ _weak_notify_cb (GstFramePositioner * pos, GObject * old)
   pos->current_track = NULL;
 }
 
+static gboolean
+auto_position (GstFramePositioner * self)
+{
+  gint scaled_width = -1, scaled_height = -1, x, y;
+
+  if (self->user_positioned) {
+    GST_DEBUG_OBJECT (self, "Was positioned by the user, not touching anymore");
+    return FALSE;
+  }
+
+  if (!self->natural_width || !self->natural_height)
+    return FALSE;
+
+  if (!self->track_width || !self->track_height) {
+    GST_INFO_OBJECT (self, "Track doesn't have a proper size, not "
+        "positioning the source");
+    return FALSE;
+  }
+
+  if (self->track_width == self->natural_width &&
+      self->track_height == self->natural_height)
+    return TRUE;
+
+  scaled_height =
+      gst_util_uint64_scale_int (self->natural_height, self->track_width,
+      self->natural_width);
+  scaled_width = self->track_width;
+  if (scaled_height > self->track_height) {
+    scaled_height = self->track_height;
+    scaled_width =
+        gst_util_uint64_scale_int (self->natural_width, self->track_height,
+        self->natural_height);
+  }
+
+  x = MAX (0, gst_util_uint64_scale_int_round (1,
+          self->track_width - scaled_width, 2));
+  y = MAX (0, gst_util_uint64_scale_int_round (1,
+          self->track_height - scaled_height, 2));
+
+  GST_INFO_OBJECT (self, "Scalling video to match track size from "
+      "%dx%d to %dx%d",
+      self->natural_width, self->natural_height, scaled_width, scaled_height);
+  self->width = scaled_width;
+  self->height = scaled_height;
+  self->posx = x;
+  self->posy = y;
+
+  return TRUE;
+}
+
 static void
 gst_frame_positioner_update_properties (GstFramePositioner * pos,
     gboolean track_mixing, gint old_track_width, gint old_track_height)
@@ -107,18 +157,22 @@ gst_frame_positioner_update_properties (GstFramePositioner * pos,
     gst_caps_set_simple (caps, "pixel-aspect-ratio", GST_TYPE_FRACTION,
         pos->par_n, pos->par_d, NULL);
 
-  if (old_track_width && pos->width == old_track_width &&
+  if (!auto_position (pos)) {
-      old_track_height && pos->height == old_track_height &&
-      pos->track_height && pos->track_width &&
-      ((float) old_track_width / (float) old_track_height) ==
-      ((float) pos->track_width / (float) pos->track_height)) {
 
-    GST_DEBUG_OBJECT (pos, "Following track size width old_track: %d -- pos: %d"
+    if (old_track_width && pos->width == old_track_width &&
-        " || height, old_track %d -- pos: %d",
+        old_track_height && pos->height == old_track_height &&
-        old_track_width, pos->width, old_track_height, pos->height);
+        pos->track_height && pos->track_width &&
+        ((float) old_track_width / (float) old_track_height) ==
+        ((float) pos->track_width / (float) pos->track_height)) {
 
-    pos->width = pos->track_width;
+      GST_DEBUG_OBJECT (pos,
-    pos->height = pos->track_height;
+          "Following track size width old_track: %d -- pos: %d"
+          " || height, old_track %d -- pos: %d", old_track_width, pos->width,
+          old_track_height, pos->height);
+
+      pos->width = pos->track_width;
+      pos->height = pos->track_height;
+    }
   }
 
   GST_DEBUG_OBJECT (caps, "setting caps");
@@ -263,6 +317,9 @@ gst_frame_positioner_class_init (GstFramePositionerClass * klass)
   GstBaseTransformClass *base_transform_class =
       GST_BASE_TRANSFORM_CLASS (klass);
 
+  GST_DEBUG_CATEGORY_INIT (framepositioner, "framepositioner",
+      GST_DEBUG_FG_YELLOW, "ges frame positioner");
+
   gst_element_class_add_static_pad_template (GST_ELEMENT_CLASS (klass),
       &gst_frame_positioner_src_template);
   gst_element_class_add_static_pad_template (GST_ELEMENT_CLASS (klass),
@@ -379,19 +436,23 @@ gst_frame_positioner_set_property (GObject * object, guint property_id,
       break;
     case PROP_POSX:
       framepositioner->posx = g_value_get_int (value);
+      framepositioner->user_positioned = TRUE;
       break;
     case PROP_POSY:
       framepositioner->posy = g_value_get_int (value);
+      framepositioner->user_positioned = TRUE;
       break;
     case PROP_ZORDER:
       framepositioner->zorder = g_value_get_uint (value);
       break;
     case PROP_WIDTH:
+      framepositioner->user_positioned = TRUE;
       framepositioner->width = g_value_get_int (value);
       gst_frame_positioner_update_properties (framepositioner, track_mixing,
           0, 0);
       break;
     case PROP_HEIGHT:
+      framepositioner->user_positioned = TRUE;
       framepositioner->height = g_value_get_int (value);
       gst_frame_positioner_update_properties (framepositioner, track_mixing,
           0, 0);

ges/gstframepositioner.h

@@ -51,7 +51,9 @@ struct _GstFramePositioner
   gint posy;
   guint zorder;
   gint width;
+  gint natural_width;
   gint height;
+  gint natural_height;
   gint track_width;
   gint track_height;
   gint fps_n;
@@ -60,6 +62,8 @@ struct _GstFramePositioner
   gint par_n;
   gint par_d;
 
+  gboolean user_positioned;
+
   /*  This should never be made public, no padding needed */
 };
 

tests/check/scenarios/check_video_track_restriction_scale.scenario

@@ -6,7 +6,9 @@ description, handles-states=true,
 
 add-clip, name=clip, asset-id=GESTestClip, layer-priority=0, type=GESTestClip, duration=1.0
 
-check-child-properties, element-name=clip, width=0, height=0
+# VideoTestSource natural size is 1280x720, so keeping aspect ratio, mean
+# that it will be scaled to 1200x675 (placed at x=0, y=163)
+check-child-properties, element-name=clip, width=1200, height=675, posx=0, posy=163
 
 set-child-properties, element-name=clip, width=1024, height=768
 check-child-properties, element-name=clip, width=1024, height=768