Dear Tanu, thanks for your comments. My answers to individual questions are in-line below. 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. On 01. 04. 2018 12:42, Tanu Kaskinen wrote: > You removed the "trace" module argument, which can break previously- > working configuration files. Since you added the logging support > back in a different way, the "trace" module argument doesn't really > need to be removed, since it could still control whether the logging > should be enabled or not. The whole "trace" functionality has been removed upstream. I think retaining that parameter isn't useful and using it for something else is confusing. As far as I can see, its existence wasn't even documented anywhere. > The previous logging code had separation for different log levels, > now only pa_log() is used (which is an alias for pa_log_error()). > Does webrtc not provide any more log level separation? The logging > is setup with this call: > > rtc::LogMessage::AddLogToStream(logsink, rtc::LS_INFO); > > Is LS_INFO the webrtc log level? If so, then it looks like all other > log levels than "info" are discarded. Unfortunately, the only way webrtc provides for plugging into its logging system is via the generic LogSink class, which doesn't receive the numeric log level. You can only set the minimum level for the messages that it receives. So at best, all webrtc log levels must be squashed into a single pa_log level (I've changed that to pa_log_info() now) I've now added a "log_level" argument that allows you to set the minimum webrtc level that is logged by PA: https://github.com/avian2/pulseaudio/commit/d97eb2122e9e5f1b251e97661b1b241b959c7b7c Anyway, upstream webrtc logging doesn't seem very useful at the moment. Even at highest verbosity only a few messages are generated on module load (which is why I originally didn't bother too much with it). I: webrtc: (audio_processing_impl.cc:441): Capture post processor activated: 0 I: Render pre processor activated: 0 I: webrtc: (audio_processing_impl.cc:704): Highpass filter activated: 0 I: webrtc: (audio_processing_impl.cc:717): Gain Controller 2 activated: 0 > You removed DEFAULT_HIGH_PASS_FILTER, which I think is not a good > change. It's good to be able to control the defaults in pulseaudio. 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). > The version in AC_INIT in configure.ac needs to be incremented (to > 0.4, I presume). > > Regarding Debian dependency handling: The original binary package > name was libwebrtc-audio-processing0 (note the 0 at the end). > Version 0.3 broke API and ABI compatibility, which is why the package > name was changed to libwebrtc-audio-processing1, so in fact Debian > does depend on a particular version. Since it looks like > compatibility will be broken again, the Debian package name will > change to libwebrtc-audio- processing2. That makes sense. Thanks for the suggestion. Best regards Tomaž
