Experimental Echo Cancellation for MacOS #4694
Conversation
|
I think the UI needs some modifications as well... It was just a check box before, but now it seems to become a tedious combo box with two options but the native macOS's echo canceller is none of these. |
Krzmbrzl
added
audio
client
feature-request
I guess on MacOS, you'd have to hide the combo box and all related entries and instead show a checkbox as none of the other stuff is available on Mac (afaik) 🤷 |
|
@Krzmbrzl I have learned that Windows has platform-dependent echo canceller as well, and I think maybe people will consider adding support for it later, therefore I adopted a more flexible method that allowing each audio backend specifies its own list of echo cancellation options.
|
TerryGeng
force-pushed
the
mac-echo
branch
5 times, most recently
from
b6562d5
to
892b4f3
Compare
|
In that case I think I would call that entry |
|
@Krzmbrzl Yeah make sense! Will do. |
Sry but I disagree. |
Well, but I think one should be able to provoke a test situation, e.g.: two people not using any headsets and disabling all echo cancel and sending noise all the time vs. same sitation, but enable your echo cancel. Regarding the tooltip in the screenshot above: |
|
That's what we have a tooltip for. There you can be a bit more verbose about what "native" means. If a user doesn't know what an OS is, we could also write it as "operating system" in the tooltip. But if they still don't know that, then that's not our problem 🤷 |
I don't see why we can't use the obvious terms (KISS). |
|
Because we should not use overly long descriptions as entries in the UI. That's what tooltips are for |
According to Chromium's thread (see the commit message), Apple may support Acoustic Echo Cancellation, which means it takes the position between the microphone and speakers into account. I don't know for sure what Apple has done in this echo cancellation component and what is the condition to trigger such an Acoustic Echo Cancellation. As I explained before, the whole thing is poorly documented. That's the reason for the word "potential". |
|
|
While as @toby63 has said, there're ways to cook up a test situation. My preliminary test shows it works, but not all the time, or sometimes it just works in a non-obvious way. I may conduct a serious test with a comparison of record files with/without echo cancellation. But anyway, I still want to know if it works in a real-world scenario. |
We can hide that discussion (later).
Thats of course a valid goal.
In that case I would leave this information out.
|
|
@Krzmbrzl Yeah... The commit order you've listed is clearly the best one for understanding the work done in this PR. However, it's not the way my understanding progressed. It's more like I didn't consider too much but not until I arrived at one point before I discovered I have to do something... These commits are entangled therefore it is hard to rearrange them logically but keep each commit compilable. Squashing them into one big commit (includes both the refactor and the AEC feature, since the AEC was added before the enum refactor and was also modified in the refactor) could be a solution to reduce the number of commits, but I'm not sure this is the best way... Or I can dissect the commit history and provide a logical commit history, but I just don't have enough power to do this... P.S. Sorry for the Speex team again... Can I just leave it in the commit message? I'm tired of keep rebasing... |
CoreAudioInput::run() was in a messy shape. This refactor cut it into smaller pieces that is easier for maintenance. CoreAudioInit has been moved from the header into CoreAudio.mm, since it inherited DeferInit, a class defined in Global.h. Including Global.h would import symbol "g" from Global.h, thereafter causing problem when compiling because Qt's MOC compiler would create a mega-header file "mocs_compilation.cpp" that include all headers file all together and the "g" defined in Global.h would override some native symbols defined in other headers.
…IP AU Apple doesn't provide a way to hijack system audio stream into Mumble so speex's echo cancellation cannot work for Mac. It was reported in https://developers.google.com/web/updates/2018/03/macos-native-echo-cancellation that Mac has a native echo cancellation AU. This commit uses this native AU to enable echo cancellation for Mac users. Unfortunately, this function is poorly documented by Apple. The best reference of this function is from Chromium's code, https://github.com/chromium/chromium/blob/master/media/audio/mac/audio_low_latency_input_mac.cc Limited support from Apple made invoking this AU a very unfriendly task. Also, the subtleness of echo cancellation made it hard to be tested. I would just submit this PR and see the reactions from other testers. Implement mumble-voip#1775.
Provided a QList of EchoCancellationOption in AudioInputRegistrar, that would be displayed in audio input settings panel. Each audio backend can added its platform dependent echo canceller to this list.
`lupdate` doesn't scan .mm file by default.
Scanning directory './src'...
Scanning directory './src/mumble'...
Updating 'src/mumble/mumble_en.ts'...
Found 1959 source text(s) (5 new and 1954 already existing)
|
I think the problem here is that your commits are entangled because they partially violate the 1-change-per-commit rule. E.g. some commits introduce a certain change plus a small refactoring at a different place. In such cases the small refactoring should be done in a preceding, separate commit. I have squashed the two commits that implement echo cancellation for macOS and left the rest pretty much alone. For the future it'd be nice if you could keep that (= that the commits should eventually end up in logical chunks) in mind while working on something like this. What I usually do if I have a commit that e.g. introduces a feature and then some follow-up commits and then I want to change that feature again is to rebase my commit history so that the feature commit is now the most recent (thus I am changing the order of commits by moving the feature commit to the front). By doing this regularly the complexity also remains rather small. For this PR however I think it's fine. Not having the commits in the logical order is a somewhat cosmetic issue after all and is therefore outweighed by the actual changes you have made. Thus I'll merge this PR as is as soon as the CI has completed. EDIT: I also fixed the Speex thingy for you ;) |
|
Thanks! Will pay attention in the future. |
|
Noticed this thread only now, may be too late, but in the mumble sources in It basically boils down to playing a youtube video while running mumble with the Unfortunately the documentation is geared towards testing Mumble's echo cancellation, not the OS native one and of course the Hope this helps, unfortunately I can't try myself as I don't have a Mac. |
In mumble-voip#4694 (dafbce2 to be precise) refactored how the echo cancellation setting was stored. During that refactoring however the wrong macro was used for actually saving the setting, causing it to not actually being saved. This commit makes sure the correct macro is used for the job so saving now works as expected. Fixes mumble-voip#4761
In mumble-voip#4694 the echo cancellation settings were refactored. During this, the default value was changed from being Speex's mixed channel mode to being off. This commit restores the previous default except for macOS where Speex echo cancellation does not work. There Apple's echo cancellation system is set as the default. Although that only works with a certain combination of input and output devices, it doesn't make the situation worse for other combinations, so we might as well enable it by default.
In mumble-voip#4694 the echo cancellation settings were refactored. It was however forgotten to provide compatibility with the old (now removed) settings. This would cause these settings to get lost with the update. This commit ensures that the old settings are preserved and are converted to the new setting.
PR mumble-voip#4694 (dafbce2) refactored how the echo cancellation setting was stored. During that refactoring however the wrong macro was used for actually saving the setting, causing it to not actually being saved. This commit makes sure the correct macro is used for the job so saving now works as expected. Fixes mumble-voip#4761
In mumble-voip#4694 the echo cancellation settings were refactored. During this, the default value was changed from being Speex's mixed channel mode to being off. This commit restores the previous default except for macOS where Speex echo cancellation does not work. There Apple's echo cancellation system is set as the default. Although that only works with a certain combination of input and output devices, it doesn't make the situation worse for other combinations, so we might as well enable it by default.
In mumble-voip#4694 the echo cancellation settings were refactored. It was however forgotten to provide compatibility with the old (now removed) settings. This would cause these settings to get lost with the update. This commit ensures that the old settings are preserved and are converted to the new setting.
Some USB audio interfaces have both input and output stream, but they are accidentally ignored in mumble-voip#4694.
Some USB audio interfaces have both input and output stream, but they are accidentally ignored in mumble-voip#4694.
Some USB audio interfaces have both input and output stream, but they are accidentally ignored in mumble-voip#4694.
Some USB audio interfaces have both input and output stream, but they are accidentally ignored in #4694. Independent from that this PR changes the macro names for saving and loading values to and from the settings in order to make that part of the code less confusing.
Some USB audio interfaces have both input and output stream, but they are accidentally ignored in mumble-voip#4694.
Apple doesn't provide a way to hijack the system audio stream into Mumble
so speex's echo cancellation cannot work for Mac.
It was reported in https://developers.google.com/web/updates/2018/03/macos-native-echo-cancellation
that Mac has a native echo cancellation AU. This commit uses this
native AU to enable echo cancellation for Mac users.
Unfortunately, this function is poorly documented by Apple. The best
reference of this function is from Chromium's code,
https://github.com/chromium/chromium/blob/master/media/audio/mac/audio_low_latency_input_mac.cc
Limited support from Apple made invoking this AU a very unfriendly
task. Also, the subtleness of echo cancellation made it hard to be
tested. I would just submit this PR and see the reactions from other
testers.
Implement #1775.
Welcome, all Mac users! Have a try and give me some feedback!