[Bug 786344] audioaggregator: implement input conversion

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=786344

Olivier Crête <olivier.crete at ocrete.ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #357684|none                        |needs-work
             status|                            |

--- Comment #2 from Olivier Crête <olivier.crete at ocrete.ca> ---
Review of attachment 357684:
 --> (https://bugzilla.gnome.org/review?bug=786344&attachment=357684)

Generally looks like a good first step!

::: gst-libs/gst/audio/gstaudioaggregator.c
@@ +300,3 @@
+G_DEFINE_ABSTRACT_TYPE_WITH_CODE (GstAudioAggregator, gst_audio_aggregator,
+    GST_TYPE_AGGREGATOR, g_type_add_class_private (g_define_type_id,
+        sizeof (GstAudioAggregatorClassPrivate)));

Any reason not to use G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE ?

@@ +504,3 @@
+/* Unref after usage */
+static GstAudioAggregatorPad *
+gst_audio_aggregator_get_first_configured_pad (GstAggregator * agg)

s/first/random (or any?)/ .. I don't thing being the first in the list means
anything?

@@ +539,3 @@
+
+  if (downstream_caps && !gst_caps_is_empty (downstream_caps))
+    s2 = gst_caps_get_structure (downstream_caps, 0);

s2 gets leaked

@@ +548,3 @@
+    gst_structure_fixate_field_nearest_int (s, "rate",
+        first_configured_pad->info.rate);
+    gst_object_unref (first_configured_pad);

You will leak the pad if the first if() succeeds... You probably want to
release it independantly from this.

@@ +593,3 @@
+    GST_OBJECT_UNLOCK (aaggpad);
+    gst_pad_push_event (GST_PAD (aaggpad), gst_event_new_reconfigure ());
+    GST_OBJECT_LOCK (aaggpad);

re-lock not needed, you can just unlock the mutex in the else branch too

@@ +752,3 @@
+   * format */
+  if (aaggpad->priv->buffer) {
+    GstBuffer *inbuf = gst_aggregator_pad_get_buffer (aggpad);

It's not safe to call this function with the pad's object lock held.

::: gst/audiomixer/gstaudiointerleave.c
@@ -585,3 @@
   aagg_class->aggregate_one_buffer =
gst_audio_interleave_aggregate_one_buffer;

-

You seem to be missing the _set_convertible() in this subclass ?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.