On Mon, 2018-04-09 at 11:56 +0200, Tomaž Šolc wrote: > 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. When there's a pressing need to break compatibility, then that can be done, but not otherwise. > 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. It's not clear to me that "tracing" and "logging" are different concepts in this case. If they are, then removing the "trace" modarg is fine (though adding a warning that it's ignored would be better). 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... > > 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). Shouldn't we then remove all the other defaults as well? -- Tanu https://liberapay.com/tanuk https://www.patreon.com/tanuk
