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
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
