juha-h / libwebrtc

webrtc AEC module and its required dependencies module for android
18 stars 6 forks source link

Poor echo cancellation on Nokia 1 #2

Closed ndilieto closed 4 years ago

ndilieto commented 4 years ago

Using version 19.0.0 on an Android 9 Go (Nokia 1), echo cancellation works poorly: it doesn't cancel completely, and often the larsen effect starts. Same on an Asus tablet running Android 5.

Digging in the code I've found that webrtc is built without WEBRTC_ANDROID. If I define that in jni/Android.mk and also enable the extended filter using the WebRtcAec_enable_extended_filter function in the baresip module, echo cancellation works significantly better, albeit not perfectly, on both devices.

I think it would be very useful if you could enable WEBRTC_ANDROID and add some parameters to the baresip app so that users can

juha-h commented 4 years ago

Thanks for the report. Could you provide a pull request on libwebrtc changes to get started?

juha-h commented 4 years ago

Like this?

diff --git a/jni/Android.mk b/jni/Android.mk
index a9efb48..f0b4abc 100644
--- a/jni/Android.mk
+++ b/jni/Android.mk
@@ -1,7 +1,7 @@

 LOCAL_PATH := $(call my-dir)

-common_CFLAGS := -fexceptions -DWEBRTC_POSIX=1 -I$(LOCAL_PATH)/src/
+common_CFLAGS := -fexceptions -DWEBRTC_POSIX=1 -I$(LOCAL_PATH)/src/ -DWEBRTC_ANDROID
 common_LDFLAGS :=

 common_SRC_FILES := \
ndilieto commented 4 years ago

Yes that is exactly what I did. The other change I made to baresip, in the baresip/modules/webrtc_aec/aec.cpp file: I added

WebRtcAec_enable_extended_filter(WebRtcAec_aec_core(aec->inst), 1);

just after the call to WebRtcAec_enable_delay_agnostic. I also needed to add the prototype to ibwebrtc/include/modules/audio_processing/aec/echo_cancellation.h

juha-h commented 4 years ago

OK, I committed the -DWEBRTC_ANDROID change to this repo. Could you give your libwebrtc/include/modules/audio_processing/aec/echo_cancellation.h diff so that I get it correctly?

ndilieto commented 4 years ago
--- a/include/modules/audio_processing/aec/echo_cancellation.h
+++ b/include/modules/audio_processing/aec/echo_cancellation.h
@@ -243,6 +243,7 @@ struct AecCore* WebRtcAec_aec_core(void* handle);

 // Non-zero enables, zero disables.
 void WebRtcAec_enable_delay_agnostic(struct AecCore*, int enable);
+void WebRtcAec_enable_extended_filter(struct AecCore*, int enable);

 #ifdef __cplusplus
 }
--- a/baresip/modules/webrtc_aec/aec.cpp
+++ b/baresip/modules/webrtc_aec/aec.cpp
@@ -102,6 +102,7 @@ int webrtc_aec_alloc(struct aec **stp, void **ctx, struct aufilt_prm *prm)
        }

        WebRtcAec_enable_delay_agnostic(WebRtcAec_aec_core(aec->inst), 1);
+        WebRtcAec_enable_extended_filter(WebRtcAec_aec_core(aec->inst), 1);

        aec->config.nlpMode       = kAecNlpModerate;
        aec->config.skewMode      = kAecFalse;
juha-h commented 4 years ago

Any idea why WebRtcAec_enable_extended_filter prototype is not in the .h file if it is intended to be called by apps?

juha-h commented 4 years ago

I think I need to fork baresip for use in the Android app, since baresip webrtc_aec module was originally written using https://webrtc.org/native-code/ lib, which does not exist anymore. Now the lib is https://chromium.googlesource.com/external/webrtc/ and the names of the functions are not anymore the same they used to be.

juha-h commented 4 years ago

Any idea why WebRtcAec_enable_extended_filter prototype is not in the .h file if it is intended to be called by apps?

I guess it is because thepacific folks didn't need it.

ndilieto commented 4 years ago

I honestly think that webrtc is astoundingly bloated. And it's getting worse and worse in newer versions. But then why be surprised? The whole of android is a giant piece of bloat.

juha-h commented 4 years ago

Looks like by default skewMode and extended_filter are disabled and nlpMode defaults to moderate. Would need to include module params to make them configurable.

Does -DWEBRTC_ANDROID alone help at all before the params are available?

juha-h commented 4 years ago

I honestly think that webrtc is astoundingly bloated. And it's getting worse and worse in newer versions. But then why be surprised? The whole of android is a giant piece of bloat.

I agree. webrtc source is huge. That is why I chose to use the older version (the same I think pjsip is using).

ndilieto commented 4 years ago

Yes WEBRTC_ANDROID with extended filter helps but I would like to experiment with the other parameters and it would be very useful to be able to do so from the gui. Also calling the debug function to log the delay would help.

BTW I bisected the commit that took the old code away:

https://webrtc-review.googlesource.com/c/src/+/161238 https://webrtc.googlesource.com/src/+/62ea0aaea04942f79a1ac98eca736b3ec0bcc860

Edit: and this is the even earlier commit where they switched to C++

https://codereview.webrtc.org/1713923002 https://webrtc.googlesource.com/src/+/8df5d4f15ba9f29c994103905e7cb7909e6ba969

juha-h commented 4 years ago

ndilieto writes:

Yes WEBRTC_ANDROID with extended filter helps

So should extended_filter default to enabled?

ndilieto commented 4 years ago

I'd say definitely yes for the two devices I mentioned but if it is good for all devices, I can't say. My gut feeling tells me yes but I may be wrong.

juha-h commented 4 years ago

ndilieto writes:

Yes WEBRTC_ANDROID with extended filter helps but I would like to experiment with the other parameters and it would be very useful to be able to do so from the gui. Also calling the debug function to log the delay would help.

Should webrtc_aec_debug be called when the call ends?

ndilieto commented 4 years ago

Should webrtc_aec_debug be called when the call ends?

That function looks to me like it is intended to be called at regular intervals, say 5 seconds. The filter will estimate the delay and that function prints the current values.

juha-h commented 4 years ago

Should webrtc_aec_debug be called when the call ends?

That function looks to me like it is intended to be called at regular intervals, say 5 seconds. The filter will estimate the delay and that function prints the current values.

As a starter, how about a quick hack where the debug function is called each time the numeric keypad icon is touched?

juha-h commented 4 years ago

Regarding the issue itself, I have not experienced any echo problems with my Nokia 3.1 when making calls over mobile network.

I your case, is this both with and without speaker phone?

ndilieto commented 4 years ago

The numeric keypad hack would be great, thanks.

My issue: without the changes and with speaker phone on it invariably creates larsen effect after a couple of seconds and is therefore unusable. Without speakerphone, it doesn't cancel well and also has larsen effect if volume is high.

With the changes and speaker phone on it is almost usable (larsen only if volume is very high); without speakerphone it works quite well.

juha-h commented 4 years ago

ndilieto writes:

Should webrtc_aec_debug be called when the call ends?

That call looks to me it is intended to be called at regular intervals, say 5 seconds. The filter will estimate the delay and that function prints the current values.

It turned out that it is not possible to call webrtc_aec_debug via baresip API.

What you might try is to make the call from encode.cpp and/or decode.cpp every Nth time encode/decode function is called.

juha-h commented 4 years ago

I'll close this issue, since the requested changes have been made to the lib. I also added Extended Filter setting to my Android baresip apps. The other two AEC settings are not there yet. I would not like to complicate the settings more that what is necessary and I don't know if non-default nplMode and skewMode values really help. If they do, please create an issue about them to baresip-studio project. Thanks again for your feedback.

juha-h commented 3 years ago

@ndilieto I created new branch "mobile" that can be used to build libwebrtc that uses aec mobile mode (aecm). It based on current webrtc source that I took from https://gitlab.freedesktop.org/pulseaudio/webrtc-audio-processing project.

I also created new baresip module webrtc_aecm that uses mobile version of libwebrtc. Would you like to give it a try?

assegaf commented 2 years ago

@ndilieto I created new branch "mobile" that can be used to build libwebrtc that uses aec mobile mode (aecm). It based on current webrtc source that I took from https://gitlab.freedesktop.org/pulseaudio/webrtc-audio-processing project.

I also created new baresip module webrtc_aecm that uses mobile version of libwebrtc. Would you like to give it a try?

it is time for me to try echo cancellation, where I can get the webrtc_aecm module source ?

juha-h commented 2 years ago

It is included in baresip project: https://github.com/baresip/baresip.