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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
