[pulseaudio-discuss] Updating the WebRTC AudioProcessing library

[Tomaž Šolc]

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ž