[Bug 786344] audioaggregator: implement input conversion

[GStreamer (GNOME Bugzilla)]

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

--- Comment #7 from Mathieu Duponchelle <mduponchelle1 at gmail.com> ---
(In reply to Sebastian Dröge (slomo) from comment #6)
> Review of attachment 360227 [details] [review]:
> 
> ::: gst-libs/gst/audio/gstaudioaggregator.c
> @@ +57,3 @@
> +    GST_PAD_SINK,
> +    GST_PAD_REQUEST,
> +    SINK_CAPS);
> 
> Subclasses might want to limit the caps in one way or another. E.g. only
> allow mono inputs.
> 

They can do that by overriding the static template after calling
perform_conversion , does that work for you ?

> @@ +135,3 @@
> +      if (pad->priv->converter_config)
> +        g_value_take_boxed (value,
> +            gst_structure_copy (pad->priv->converter_config));
> 
> Or just g_value_set_boxed(value, config)? That copies internally then. Less
> noise
> 

Indeed, fixed

> @@ +592,3 @@
> +  /* TODO: handle different rates on sinkpads, a bit complex
> +   * because offsets will have to be updated, and audio resampling
> +   * has a latency to take into account
> 
> You should prevent that in get_caps() then if it's not supported yet
> (ignoring the race condition between getcaps and setcaps).
> 

Yeah, I'm doing that already actually :)

> ::: gst-libs/gst/audio/gstaudioaggregator.h
> @@ +168,3 @@
>  
> +GST_EXPORT
> +void gst_audio_aggregator_class_perform_conversion (GstAudioAggregatorClass
> * klass);
> 
> Should probably take a boolean so sub-subclasses can override it

Hm I see, do you figure that's an actual use case? I mean we do have liveadder,
but afaiu it's mostly for backwards compatibility, I don't figure we actually
want to extend the inheritance tree much further, it's already pretty deep as
it is? The lack of a boolean was actually on purpose, that's also why I renamed
that method from set_convertible to perform_conversion.

Related to the point above however, we may want to accept a static pad template
here, to make it clearer for the subclass how to restrict the input caps, this
could eventually be used by a sub-subclass to effectively disable conversion.

> , and maybe a vfunc that does the conversion and can be overridden?

That would be nice, however I would not add that preemptively, but wait till we
have an actual use case to test the API with?

> ::: gst/audiomixer/gstaudiomixer.h
> @@ -53,3 @@
> -
> -  /* target caps (set via property) */
> -  GstCaps *filter_caps;
> 
> \o/ It's gone finally

Yeah, we need to be careful when we merge that however because GES uses it, I
figured while we were still in bad we could break API and get rid of this :)

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