GNOME Bugzilla – Bug 793917
audioaggregator: Conversion API is confusing
Last modified: 2018-02-28 23:52:53 UTC
Currently audioaggregator always does conversion by default for the sinkpads and srcpad. Disabling this would require the convert_buffer vfunc to be NULL, which is what audiointerleave does. In addition there is a GstAudioAggregatorConvertPad type. Using it does not decide whether conversion is possible, it only a) caches the converter (it is otherwise recreated for each buffer!) and b) provides a property to set the converter parameters. To clear up this confusion I would propose the following: 1) Conversion is only decided by the existence of convert_buffer, and by default that's exactly what it does 2) GstAudioAggregatorPad already caches the converter, and equally a converter is cached for the srcpad. It makes no sense to recreate it all the time (but it should be flushed correctly when needed) 3) Let GstAudioAggregatorConvertPad only be there for the property. If you want the default conversion behaviour and the property, use that pad type. Otherwise not. Thoughts?
basically the effect of my proposal would be to get rid of all the "if (GST_IS_AUDIO_AGGREGATOR_CONVERT_PAD (pad))" and always do that.
And documenting that the *only* use of the different pad class is the property, and everything else is controlled by the vfunc and conversion happens by default.
Ok, after some discussion with Mathieu on IRC, ignore the above. Instead: 1) Aggregator gets API to select the srcpad type 2) AudioAggregator only ever does conversion if AudioAggregatorConvertPad is used 2.1) convert_buffer vfunc moves to AudioAggregatorPad 2.2) AudioAggregatorConvertPad implements conversion 2.3) srcpad also becomes a AudioAggregatorConvertPad unless overridden by subclass This is also closer to what VideoAggregator already does. And keeps concerns better separated than the confusion we have now.
Created attachment 369111 [details] [review] audioaggregator: refactor conversion API For the rationale, see: https://bugzilla.gnome.org/show_bug.cgi?id=793917 Also test audiomixer conversion of current output buffer
Created attachment 369121 [details] [review] audioaggregator: refactor conversion API For the rationale, see: https://bugzilla.gnome.org/show_bug.cgi?id=793917 Also test audiomixer conversion of current output buffer
Attachment 369121 [details] pushed as 10835e9 - audioaggregator: refactor conversion API