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
