[pulseaudio-discuss] Updating the WebRTC AudioProcessing library

[Tanu Kaskinen]

On Mon, 2018-04-09 at 11:56 +0200, Tomaž Šolc wrote:
> 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.

When there's a pressing need to break compatibility, then that can be
done, but not otherwise.

> 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.

It's not clear to me that "tracing" and "logging" are different
concepts in this case. If they are, then removing the "trace" modarg is
fine (though adding a warning that it's ignored would be better).

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...

> > 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).

Shouldn't we then remove all the other defaults as well?

-- 
Tanu

https://liberapay.com/tanuk
https://www.patreon.com/tanuk