pitivi / gst-editing-services

MIRROR of the GStreamer Editing Services repository for pull requests. The official git repository is at http://cgit.freedesktop.org/gstreamer/gst-editing-services/
Other
1 stars 1 forks source link

Reimplement Audio mixing #12

Closed thiblahute closed 11 years ago

thiblahute commented 11 years ago

It is a meta bug, to follow progress of the audio mixing bug, each bug that is related should inclide a ref to that bug.

Blocks: https://github.com/pitivi/pitivi/issues/20

thiblahute commented 11 years ago

Branch is https://github.com/pitivi/gst-editing-services/tree/audio-mixing

thiblahute commented 11 years ago

review of https://github.com/MathieuDuponchelle/PitiviGes/tree/audio_mixing between cfd26bc991ce10b7e08100ab6d900c70b11c932d and 4173d78a9cf367ad2215b4ebbec31ac84d1d8ae1

-/****************************************************
  *              GstElement vmetods                  *
  ****************************************************/
 static GstPad *
 _request_new_pad (GstElement * element, GstPadTemplate * templ,
     const gchar * name, const GstCaps * caps)
 {
-  GstPad *volume_srcpad, *audioconvert_sinkpad, *tmpghost;
-
+  GstPad *audioresample_srcpad, *audioconvert_sinkpad, *tmpghost;
   PadInfos *infos = g_slice_new0 (PadInfos);
   GESSmartAdder *self = GES_SMART_ADDER (element);
+  GstPad *ghost;
+  GstElement *audioconvert, *audioresample;

try to keep declaration of variable with assigned value after the others

   infos->adder_pad = gst_element_request_pad (self->adder, templ, NULL, caps);
   if (infos->adder_pad == NULL) {
@@ -162,52 +88,46 @@ _request_new_pad (GstElement * element, GstPadTemplate * templ,

     return NULL;
   }
-  infos->self = gst_object_ref (self);
+
+  infos->self = self;

Why don't youneed to keep a ref? (or why was I keeping a ref?)

+static void
+_ghost_pad_added_cb (GstElement * element, GstPad * srcpad, GstElement * bin)
+{
+  GstPad *ghost;
+
+  gst_element_no_more_pads (element);

You need to emit nmp after adding the pad to the bin

+static gboolean
+_pspec_equal (gconstpointer key_spec_1, gconstpointer key_spec_2)
+static guint
+_pspec_hash (gconstpointer key_spec)

maybe make those internal helpers

+static GstElement *
+_create_bin (const gchar * bin_name, GstElement * decodebin, ...)

Be carefull with the heap! :P

+static void
+_add_elements_properties_to_hashtable (GESUriSource * self, ...)

Here we only want volume and mute to be exposed... take that into account, I did not implement that generically on purpose.

+/* Only works with float beware. TODO : macro ? */

should you just state that in the func name

+static void
+_sync_element_to_layer_property (GESTrackElement * trksrc,
+    GstElement * element, const gchar * meta, const gchar * propname)
+  GESTimelineElement *parent;
+  GESLayer *layer;
+  gfloat value;
+
+  parent = ges_timeline_element_get_parent (GES_TIMELINE_ELEMENT (trksrc));
+  layer = ges_clip_get_layer (GES_CLIP (parent));
+
+  gst_object_unref (parent);
+
+  if (layer != NULL) {
+
+    ges_meta_container_get_float (GES_META_CONTAINER (layer), meta, &value);
+    g_object_set (element, propname, value, NULL);

And the keyframes?! :) Also that should be done whenever the object changes layer, and when the propery changes on the layer.

MathieuDuponchelle commented 11 years ago

I pushed on the branch, fixing the tests and your observations, except for the syncing according to keyframes, which I'll do later.

thiblahute commented 11 years ago

Done :)