[pulseaudio-discuss] Updating the WebRTC AudioProcessing library

[Tanu Kaskinen]

On Mon, 2018-03-26 at 15:25 +0200, Tomaž Šolc wrote:
> Dear Arun, all
> 
> I've managed to update the audio processing library and 
> module-echo-cancel with the code from upstream. My working version is 
> currently synced to webrtc commit 3133857 (Mar 13), as used by Chromium 
> 67.0.3370.0. It's based on Arun's previous updates and the very helpful 
> UPDATING.md. It's already a bit out of date again, as I see that some 
> more AEC-related work has been committed upstream since then.
> 
> The code is on GitHub. See branches "webrtc_update_3133857" in the
> following repos. The commit history is currently a bit hairy, since
> there was a lot of trial-and-error. I can make a set of clean patches.
> 
> https://github.com/avian2/webrtc-audio-processing
> 
> https://github.com/avian2/pulseaudio

Thanks for your work! I had a glance at the pulseaudio changes (not
really a proper review, I'm hoping that Arun will do that), and I have
some remarks:

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.

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.

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

>  From my initial tests (with default settings) it seems that the updated
> module works significantly better than the old one - at least for my
> specific use case in an embedded device. I've tested it on x86_64 and 
> armhf. However, I cannot reliably test if all module options work. I 
> haven't tested beamforming.
> 
> Some things that might still need work:
> 
> I see that existing Makefiles do some selection on what headers to
> install and what not, but it was not clear to me how this selection was
> made. I think upstream makes no distinction between "public" and
> "private" headers. So far I've only made changes that were needed to
> compile module-echo-cancel.

(I have no idea about that.)

> Package versioning. I've updated the libtool interface numbers, but left
> autotools package version and Debian package numbers (the repos above
> also have a "debian" branch that I used for testing). I've noticed that
> the "pulseaudio" Debian package does not depend on any specific
> "libwebrtc-audio-processing1" version. I'm not sure what is correct
> approach here.

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.

-- 
Tanu

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