[Bug 786344] audioaggregator: implement input conversion

[GStreamer (GNOME Bugzilla)]

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

--- Comment #3 from Mathieu Duponchelle <mduponchelle1 at gmail.com> ---
(In reply to Olivier Crête from comment #2)
> Review of attachment 357684 [details] [review]:
> 
> 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 ?
> 

It's a *class* private, no macros for that :)

> @@ +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?
> 

Well I'd rather the function says what it's actually doing :)

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

gst_caps_get_structure is (transfer none) :)

> @@ +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.
> 

Indeed, fixed :)

> @@ +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
> 

I actually only needed locking in the else branch anyway, updated :)

> @@ +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.
> 

Side-stepped the issue entirely, by keeping a ref on the input buffer, as we
discussed on IRC.

> ::: 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 ?

I've changed the name of that class method to perform_conversion, if not called
during class_init the default behaviour is to not perform conversion, as it is
only checked once it doesn't really need a boolean parameter.

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