On 10. 04. 2018 14:26, Tanu Kaskinen wrote: > On Mon, 2018-04-09 at 11:56 +0200, Tomaž Šolc wrote: >> In general I think there's a question of how heavy a wrapper the >> "module-echo-cancellation" is supposed to be around the webrtc code. The >> upstream is very actively reworking large parts of the code. Most >> likely closely following the changes and ensuring backwards >> compatibility in PulseAudio, even at the level of module arguments, >> isn't feasible in the long term. > > When there's a pressing need to break compatibility, then that can be > done, but not otherwise. I see. I restored back the "trace" argument. > The lack of documentation is a valid point. Generally I'm of the > opinion is that if some API isn't documented, it shouldn't be used. In > module-echo-cancel's case that would mean most of the webrtc options > shouldn't be used, which of course isn't a good... Unfortunately the whole webrtc part of module-echo-cancel is an exercise in using an internal, undocumented API that is subject to change at any time. See e.g. discussion here. It's from 2012 but AFAIK this situation has not changed: https://groups.google.com/forum/#!topic/discuss-webrtc/pyxD1_BeJvs Still, we find it useful, since it seems to be the best open source echo canceller available at the moment. >> I removed this default because the new webrtc configuration class seems >> to define some sensible defaults itself. Individual components no longer >> need to be explicitly enabled/disabled like before. >> >> I think upstream has a much better knowledge of which defaults make >> sense. For example, high pass filter is disabled by default in the >> current upstream (while DEFAULT_HIGH_PASS_FILTER was set to true). > > Shouldn't we then remove all the other defaults as well? The upstream code is in the middle of migration between two configuration systems: Some settings use the new webrtc::AudioProcessing::Config class. That class includes some defaults that have been defined upstream. I made module-echo-cancel respect those defaults (high_pass_filter, residual_echo_detector). Other settings still use the old webrtc::Config class. That class has no defaults. Hence I left in place the old DEFAULT_... macros. Best regards Tomaž
