[pulseaudio-discuss] Updating the WebRTC AudioProcessing library
tanuk at iki.fi
Tue Apr 10 12:26:55 UTC 2018
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
> 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
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:
> 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?
More information about the pulseaudio-discuss