Re: [pulseaudio-discuss] Updating the WebRTC AudioProcessing library

2018-04-16 Thread Tanu Kaskinen
On Thu, 2018-04-12 at 11:31 +0200, Tomaž Šolc wrote:
> On 10. 04. 2018 14:26, Tanu Kaskinen wrote:
> > > 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?
> 
> The upstream code is in the middle of migration between two 
> configuration systems:
> 
> Some settings use the new webrtc::AudioProcessing::Config class. That 
> class includes some defaults that have been defined upstream. I made 
> module-echo-cancel respect those defaults (high_pass_filter, 
> residual_echo_detector).
> 
> Other settings still use the old webrtc::Config class. That class has no 
> defaults. Hence I left in place the old DEFAULT_... macros.

Ok, sounds reasonable.

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


Re: [pulseaudio-discuss] Updating the WebRTC AudioProcessing library

2018-04-12 Thread Tomaž Šolc

On 10. 04. 2018 14:26, Tanu Kaskinen wrote:

On Mon, 2018-04-09 at 11:56 +0200, Tomaž Šolc wrote:

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.


I see. I restored back the "trace" argument.


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


Unfortunately the whole webrtc part of module-echo-cancel is an exercise 
in using an internal, undocumented API that is subject to change at any 
time. See e.g. discussion here. It's from 2012 but AFAIK this situation 
has not changed:


https://groups.google.com/forum/#!topic/discuss-webrtc/pyxD1_BeJvs

Still, we find it useful, since it seems to be the best open source echo 
canceller available at the moment.



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?


The upstream code is in the middle of migration between two 
configuration systems:


Some settings use the new webrtc::AudioProcessing::Config class. That 
class includes some defaults that have been defined upstream. I made 
module-echo-cancel respect those defaults (high_pass_filter, 
residual_echo_detector).


Other settings still use the old webrtc::Config class. That class has no 
defaults. Hence I left in place the old DEFAULT_... macros.


Best regards
Tomaž
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss


Re: [pulseaudio-discuss] Updating the WebRTC AudioProcessing library

2018-04-09 Thread Tomaž Šolc

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.


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.


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


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.


That makes sense. Thanks for the suggestion.

Best regards
Tomaž
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss


Re: [pulseaudio-discuss] Updating the WebRTC AudioProcessing library

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