Review of video mixing. about gst-editing-services

[thiblahute]

MathieuDuponchelle/PitiviGes@cf2698a

[thiblahute]

MathieuDuponchelle/PitiviGes@c1cfb81

[thiblahute]

MathieuDuponchelle/PitiviGes@939bf71

[MathieuDuponchelle]

MathieuDuponchelle/PitiviGes@b2cadba

done.

[thiblahute]

You should also check if videomixer is present in ges_init and WARN the user/disable the feature if not I think.

[thiblahute]

From cbdd871 to 01a5963

• It should should be called smart-video-mixer.c I think

• Great news :D

• Really?

• Not sure it is a WARNING here

• I though it was 0.75 the real number....

• We really want the debugging to be ges* filtrable here! Either you remove that,

• or you make in inside the GES namespace

• Not so dummy....

• Sure you do not want to set x-y-z-alpha here?!

• Why don't you take the lock before and unlock after that whole switch? (would be

• the same thing in term of execution)

• I think you include it in the .c also...

• OOPS...

  diff --git a/ges/ges-smart-mixer.c b/ges/ges-smart-mixer.c

It should should be called smart-video-mixer.c I think

@@ -0,0 +1,237 @@
+/* -*- Mode: C; indent-tabs-mode: nil; c-basic-offset: 4; tab-width: 4 -*-  */
+/*
+ * gst-editing-services
+ *
+ * Copyright (C) 2013 Thibault Saunier <[email protected]>

Great news :D

+#include <gst/audio/audio.h>

Really?

+static GstPadProbeReturn
+parse_metadata (GstPad * mixer_pad, GstPadProbeInfo * info, gpointer unused)
+{
+  GstFramePositionnerMeta *meta;
+
+  meta =
+      (GstFramePositionnerMeta *) gst_buffer_get_meta ((GstBuffer *) info->data,
+      gst_frame_positionner_meta_api_get_type ());
+
+  if (!meta) {
+    GST_WARNING ("The current source should use a framepositionner");

Not sure it is a WARNING here

+/****************************************************
+ *              GstElement vmetods                  *
+ ****************************************************/
+static GstPad *
+_request_new_pad (GstElement * element, GstPadTemplate * templ,
+    const gchar * name, const GstCaps * caps)
+{
+  GstPad *videoconvert_srcpad, *videoconvert_sinkpad, *tmpghost;
+  PadInfos *infos = g_slice_new0 (PadInfos);
+  GESSmartMixer *self = GES_SMART_MIXER (element);
+  GstPad *ghost;
+  GstElement *videoconvert;
+
+  infos->mixer_pad = gst_element_request_pad (self->mixer,
+      gst_element_class_get_pad_template (GST_ELEMENT_GET_CLASS (self->mixer),
+          "sink_%u"), NULL, NULL);
+
+  g_object_set (G_OBJECT (infos->mixer_pad), "alpha", 0.5, NULL);

I though it was 0.75 the real number....

+
+GST_DEBUG_CATEGORY_STATIC (gst_frame_positionner_debug_category);
+#define GST_CAT_DEFAULT gst_frame_positionner_debug_category
+

We really want the debugging to be ges* filtrable here! Either you remove that,
or you make in inside the GES namespace

+      "frame positionner", "Metadata",
+      "This dummy element provides with tagging facilities",

Not so dummy....

+gst_frame_positionner_init (GstFramePositionner * framepositionner)
+{
+}

Sure you do not want to set x-y-z-alpha here?!

+  switch (property_id) {
+    case PROP_ALPHA:
+      GST_OBJECT_LOCK (framepositionner);
+      framepositionner->alpha = g_value_get_double (value);
+      GST_OBJECT_UNLOCK (framepositionner);
+      break;
+    case PROP_POSX:
+      GST_OBJECT_LOCK (framepositionner);
+      framepositionner->posx = g_value_get_int (value);
+      GST_OBJECT_UNLOCK (framepositionner);
+      break;
+    case PROP_POSY:
+      GST_OBJECT_LOCK (framepositionner);
+      framepositionner->posy = g_value_get_int (value);
+      GST_OBJECT_UNLOCK (framepositionner);
+      break;
+    case PROP_ZORDER:
+      GST_OBJECT_LOCK (framepositionner);
+      framepositionner->zorder = g_value_get_uint (value);
+      GST_OBJECT_UNLOCK (framepositionner);
+      break;

Why don't you take the lock before and unlock after that whole switch? (would be
the same thing in term of execution)

+#define _GST_FRAME_POSITIONNER_H_
+
+#include <gst/base/gstbasetransform.h>

I think you include it in the .c also...

GST_END_TEST;

+static gboolean
+print_position (GstElement * pipeline)
+{
+  gint64 pos;
+
+  gst_element_query_position (pipeline, GST_FORMAT_TIME, &pos);
+  GST_ERROR ("position : %" GST_TIME_FORMAT, GST_TIME_ARGS (pos));

Oops

[MathieuDuponchelle]

MathieuDuponchelle/PitiviGes@d1e5133

[thiblahute]

Impressive commit :P

[MathieuDuponchelle]

I know I know, so much time spent on that one.

[MathieuDuponchelle]

MathieuDuponchelle/PitiviGes@c752bde
MathieuDuponchelle/PitiviGes@37c68a7
MathieuDuponchelle/PitiviGes@17fc842
MathieuDuponchelle/PitiviGes@cbd2f07
MathieuDuponchelle/PitiviGes@af8bcdf

[thiblahute]

This is now merged, but I am not closing until you write some documentation about the new feature...

setting as Work needed

[MathieuDuponchelle]

Closing because merged