Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable echo cancellation by default #4214

Merged

Conversation

Krzmbrzl
Copy link
Member

@Krzmbrzl Krzmbrzl commented May 28, 2020

Now that the echo canceller has been fixed, it is time to default-enable
echo cancelling. As multi-channel cancelling is more CPU intensive, we
default to mixed channel cancellation.

Don't enable it on MacOS though as right now we don't support echo
cancellation for that platform.

Closes: #4178

Changelog:

| Improved: Echo cancellation now enabled by default

@Krzmbrzl Krzmbrzl added the audio label May 28, 2020
@Krzmbrzl Krzmbrzl added this to the 1.4.0 milestone May 28, 2020
@TerryGeng
Copy link
Contributor

TerryGeng commented May 28, 2020

Since echo cancellation only works for some platform (where you can hijack the system audio output as your echo source), did this change take this fact into account?

I'm running mumble at Mac, and I'm afraid that this option will be marked as on in the settings window but is off in the background, and since this option will be disabled by

if (air->canEcho(outputInterface)) {
qcbEcho->setEnabled(true);
qcbEcho->setToolTip(QObject::tr("If enabled this tries to cancel out echo from the audio stream.\n"
"Mixed echo cancellation mixes all speaker outputs in one mono stream and passes that stream to "
"the echo canceller, while multichannel echo cancellation passes all audio channels to the echo canceller directly.\n"
"Multichannel echo cancellation requires more CPU, so you should try mixed first"));
} else {
qcbEcho->setEnabled(false);
qcbEcho->setToolTip(QObject::tr("Echo cancellation is not supported for the interface "
"combination \"%1\" (in) and \"%2\" (out).").arg(air->name).arg(outputInterface));
}
, it will look like this option is forced to be on, but actually that is not the case in the background.

Update: This worry confirmed by seeing that

int echo = 0;
if (r.bEcho)
echo = r.bEchoMulti ? 2 : 1;
loadComboBox(qcbEcho, echo);
}

Where this checkbox is initialized with no check by calling canEcho function.

Proposed solution:
add

loadComboBox(qcbEcho, 0); 

after L417.

@fedetft
Copy link
Contributor

fedetft commented May 28, 2020

I see a bEchoMulti = true; in Settings, won't this enable multichannel echo cancellation by default?

@TerryGeng
Copy link
Contributor

I see a bEchoMulti = true; in Settings, won't this enable multichannel echo cancellation by default?

I think now that bEcho is true by default, we need to set bEchoMulti to false.

@Krzmbrzl
Copy link
Member Author

@TerryGeng yes you are right. I forgot about the Mac situation.

@fedetft you are also correct. I'll have to set that to false then. I don't even know why this would be enabled by default why echo cancelling itself is disabled by default 🤷

@Krzmbrzl Krzmbrzl force-pushed the default-enable-echo-cancellation branch from 76fec60 to 73cb14c Compare May 28, 2020 08:42
@Krzmbrzl
Copy link
Member Author

Both should be fixed now

@Krzmbrzl Krzmbrzl closed this May 28, 2020
@Krzmbrzl Krzmbrzl reopened this May 28, 2020
Now that the echo canceller has been fixed, it is time to default-enable
echo cancelling. As multi-channel cancelling is more CPU intensive, we
default to mixed channel cancellation.

Don't enable it on MacOS though as right now we do not support echo
cancellation for that platform.
@Krzmbrzl Krzmbrzl force-pushed the default-enable-echo-cancellation branch from 73cb14c to 5914172 Compare May 28, 2020 08:44
@TerryGeng
Copy link
Contributor

TerryGeng commented May 28, 2020

Thanks for the quick reply! Hmmmmm.... It looks like it is not only the problem of mac.
ALSA -> not supported
ASIO -> supported
CoreAudio(Mac) -> not supported
JACK -> not supported
OSS -> not supported
PortAudio -> not supported
PulseAudio -> it depends
WSAPI -> it depends

SInce whether echo cancellation is supported depends on the Audio backend used, not just the OS. I suggest that adopting my proposed way:

Add loadComboBox(qcbEcho, 0); after L417 of

if (air->canEcho(outputInterface)) {
qcbEcho->setEnabled(true);
qcbEcho->setToolTip(QObject::tr("If enabled this tries to cancel out echo from the audio stream.\n"
"Mixed echo cancellation mixes all speaker outputs in one mono stream and passes that stream to "
"the echo canceller, while multichannel echo cancellation passes all audio channels to the echo canceller directly.\n"
"Multichannel echo cancellation requires more CPU, so you should try mixed first"));
} else {
qcbEcho->setEnabled(false);
qcbEcho->setToolTip(QObject::tr("Echo cancellation is not supported for the interface "
"combination \"%1\" (in) and \"%2\" (out).").arg(air->name).arg(outputInterface));
}

This will essentially check with the current audio setup to determine whether echo cancellation can be enabled, and then set the checkbox. So we don't have to worry that we freeze our combo box when it is set to "on".

@Krzmbrzl
Copy link
Member Author

Krzmbrzl commented May 28, 2020

@TerryGeng sounds good to me. I think though that this should be a separate PR. Since you brought it up: Do you want to create said PR to also receive the glory for the change/fix? :D

@Krzmbrzl Krzmbrzl merged commit 663fe98 into mumble-voip:master May 28, 2020
@Krzmbrzl Krzmbrzl deleted the default-enable-echo-cancellation branch November 9, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable echo cancellation by default
3 participants