FEAT(client, server): Add more control over UDP audio stream. #5684

irakr · 2022-05-15T19:57:53Z

As requested in #5517 this commit implements the ability to control
the inbound and outbound UDP audio/voice stream by providing additional
options in the network settings. The client and the server works
together to provide these individual controls.

An additional section called "UDP voice stream mode" has been added.
Each of the options can be selected to specify the behaviour of the UDP
as well as TCP voice stream.

Also a protobuf message component called "force_tcp_only_voice" is
added under UserState message which is used to force the server to
use TCP mode only.

Fixes #5517

Co-Authored-By: Krzmbrzl [email protected]

Checks

My commits follow the commit guidelines

Krzmbrzl

This is just a first, quick review.
Note that you should run clang-format over the source tree. You'll have to use clang-format 10 for that or else the formatting will be off. If you don't have that at hand, I can format your PR for you.

src/Mumble.proto

src/mumble/ConfigWidget.cpp

src/mumble/NetworkConfig.cpp

src/mumble/ServerHandler.cpp

Krzmbrzl · 2022-05-17T06:16:20Z

src/mumble/Settings.h

-	bool bReconnect   = true;
-	bool bAutoConnect = false;
-	bool bQoS         = true;
+	bool bTCPCompat                   = false;


I know that this setting existed before, but do you happen to know what it is for? Do we still need that?

It is the flag that represents "Forced TCP mode" from the settings. It is the flag that tells whether currently we are in forced TCP mode or not. My changes rely on this.

src/mumble/Settings.h

src/mumble/SettingsKeys.h

Krzmbrzl · 2022-05-23T06:22:43Z

The commits should be in reversed order. First the commit making the code code changes and then the commit updating the translation files. This is because the latter does not make sense without the former.

irakr · 2022-05-23T06:36:04Z

The commits should be in reversed order. First the commit making the code code changes and then the commit updating the translation files. This is because the latter does not make sense without the former.

Oh my bad. Sure will do that. Thanks.

irakr · 2022-06-05T17:23:35Z

I hope the commits are as expected. If not, do let me know.

Krzmbrzl · 2022-06-05T17:34:40Z

Sorry for the delay. I didn't really have the time to review the changes more thoroughly yet, but I did not forget about this PR! I will probably get to doing it in the coming days though, so stay tuned 👀

irakr · 2022-06-05T17:43:25Z

Not a big deal😊. Please take your time.

Krzmbrzl · 2022-07-25T09:56:35Z

src/mumble/NetworkConfig.cpp

@@ -34,6 +35,8 @@ NetworkConfig::NetworkConfig(Settings &st) : ConfigWidget(st) {
 	qlePort->setAccessibleName(tr("Port"));
 	qleUsername->setAccessibleName(tr("Username"));
 	qlePassword->setAccessibleName(tr("Password"));
+
+	connect(qcbTcpMode, &QCheckBox::stateChanged, this, &NetworkConfig::on_qcbTcpMode_stateChanged);


Actually, I think this connection will be made automatically thanks to the naming of the slot. Qt has some auto-connect features that should handle this case.

Thanks for pointing it out. I should have at least seen the same thing done with on_qcbType_currentIndexChanged() and similar slots. :)

Krzmbrzl · 2022-07-25T09:57:34Z

src/mumble/NetworkConfig.cpp

@@ -157,6 +179,10 @@ void NetworkConfig::on_qcbType_currentIndexChanged(int v) {
 	s.ptProxyType = pt;
 }

+void NetworkConfig::on_qcbTcpMode_stateChanged(int v) {
+	qgbUdpMode->setDisabled((v == 0) ? false : true);


Suggested change

qgbUdpMode->setDisabled((v == 0) ? false : true);

qgbUdpMode->setDisabled(v != 0);

Krzmbrzl · 2022-07-25T10:01:35Z

src/mumble/MainWindow.cpp

+	if (Global::get().s.bTCPCompat) {
+		Global::get().sh->forceTcpOnlyMode(true);
+	}


Wouldn't it make more sense to let the ServerHandler check for this setting itself when it is about to establish a connection? That way we would exclude any potential weird issues arising due to bad timings where the connection has already been established but this slot has not yet executed and thus the handler still thinks it is supposed to use UDP...

Yes, since it is a crucial parameter which is best be configured once during connection establishment, as you said.

I think the best place to put it would be in the slot void ServerHandler::serverConnectionConnected();. Please let me know your opinion on that?

Yes, I would say that would be a good fit 👍

Krzmbrzl · 2022-07-25T10:04:49Z

src/mumble/Settings.h

-	bool bReconnect   = true;
-	bool bAutoConnect = false;
-	bool bQoS         = true;
+	bool bTCPCompat              = false;


I think a better name for this setting would be e.g. bUdpDisabled
That way it would be clearer that it is related to the same mechanism as the other (new) settings

Does it mean also mean that the strings like "Force TCP Mode", "net/tcponly" also must be changed to something that says "Disable UDP"?

Yeah, I think from a consistency point of view that would be best.

Note that with the new settings framework, renaming a setting is really easy. Just prepend the new name to the list here:

mumble/src/mumble/SettingsKeys.h

Line 91 in c87fb2b

const SettingsKey RESTRICT_TO_TCP_KEY = { "restrict_to_tcp" };

const SettingsKey RESTRICT_TO_TCP_KEY = { "disable_udp", "restrict_to_tcp" };

(and maybe also rename the variable's name accordingly)

The SettingsKey implementation should then automatically take care of creating a migration path from the old setting name to the new one (should be tested though).

Does it mean there won't be anything that says "Force TCP mode"? And instead now, are we completely calling everything as "UDP disabled mode"?

And also, from the new settings framework that you mentioned, I guess this is what I am expected to get in the mumble settings file right? ->

"network": { "disable_udp": true, "reconnect_automatically": false }

I see that "tcponly" is replaced by "disable_udp".

Does it mean there won't be anything that says "Force TCP mode"? And instead now, are we completely calling everything as "UDP disabled mode"?

Yes that is what I would have imagined. That seems like the most consistent choice 🤔

I see that "tcponly" is replaced by "disable_udp".

Yeah that's the expected behavior. Well actually tcponly was the key that was used in the old INI format of the settings. With the change to the JSON format, that setting should be called restrict_to_tcp and with your changes this should then be replaced by disable_udp.

irakr · 2022-07-27T16:32:01Z

My bad. I accidentally closed the PR by resetting the commits on my forked master branch. :D

Krzmbrzl · 2022-07-28T06:08:12Z

src/mumble/AudioConfigDialog.cpp

@@ -349,7 +349,7 @@ void AudioInputDialog::updateBitrate() {
 	overhead = 100 * 8 * (20 + 8 + 4 + 1 + 2 + p);

 	// TCP is 12 more bytes than UDP
-	if (NetworkConfig::TcpModeEnabled())
+	if (NetworkConfig::UdpModeDisabled())


I think technically this if should only check whether TCP is currently used for _incoming audio - whether it is used for outgoing packets doesn't matter. So either we do something like NetworkConfig::UdpModeDisabled || <UdpDisabledForIncomingTraffic> or we only check the latter but ensure that if UDP is disabled in general, we also set the variable for incoming (and outgoing) UDP mode accordingly.

(Actually, re-check that this code is about processing incoming packets and if it is actually processing outgoing ones, then adapt my statement above accordingly)

Krzmbrzl · 2022-07-28T06:12:33Z

src/mumble/ServerHandler.cpp

@@ -800,6 +800,10 @@ void ServerHandler::serverConnectionConnected() {
 	mpa.set_opus(true);
 	sendMessage(mpa);

+	if (Global::get().s.bUdpDisabled) {
+		Global::get().sh->disableUdpMode(true);


Suggested change

Global::get().sh->disableUdpMode(true);

disableUdpMode(true);

This should be applied to the current instance

Krzmbrzl · 2022-07-28T06:15:07Z

src/mumble/Settings.cpp

+	LOAD(bUdpDisabled, "net/tcponly");
+	LOAD(bUdpStreamInboundOnly, "net/udpin");
+	LOAD(bUdpStreamOutboundOnly, "net/udpout");
+	LOAD(bUdpStreamBidirectional, "net/udpboth");
+	LOAD(bUdpStreamAuto, "net/udpauto");


These are new settings, so they should not be loaded as legacy settings. This function is only needed to provide backwards compatibility for migrating the old INI format to the new JSON format. All settings that are added only after the introduction of the JSON format should not be added here to the legacy load function.

Suggested change

LOAD(bUdpDisabled, "net/tcponly");

LOAD(bUdpStreamInboundOnly, "net/udpin");

LOAD(bUdpStreamOutboundOnly, "net/udpout");

LOAD(bUdpStreamBidirectional, "net/udpboth");

LOAD(bUdpStreamAuto, "net/udpauto");

LOAD(bUdpDisabled, "net/tcponly");

Krzmbrzl · 2022-07-28T06:20:56Z

src/mumble/Settings.h

+	bool bUdpDisabled            = false;
+	bool bUdpStreamInboundOnly   = false;
+	bool bUdpStreamOutboundOnly  = false;
+	bool bUdpStreamBidirectional = false;


I'm wondering whether this might be better served by a single variable of enum type:

enum class UDPMode { Disabled, Inbound, Outbound, Bidirectional, }; UDPMode udpMode = UDPMode::Bidirectional;

or maybe even as bit-flags:

namespace UDPMode { enum Mode { Disabled = 0, Inbound = 1 << 0, Outbound = 1 << 1, Bidirectional = Inbound | Outbound, }; } UDPMode::Mode udpMode = UDPMode::Bidirectional

and then check like this:

if (udpMode & UDPMode::Inbound) { // Do stuff }

This has the advantage that above code works as expected if udpMode is Bidirectional without explicitly checking udpMode == Inbound || udpMode == Bidirectional (which is what would be needed with my first approach)

The bit-flags approach seems to be better. I will go ahead with that.

So by the namespace->enum suggestion you mean the definition of the enum is in the global scope? Not inside 'Settings' class?
And what about the UDPMode::Auto?

PROCESS(network, RESTRICT_TO_TCP_KEY, bUdpDisabled) \ PROCESS(network, UDP_INBOUND_ONLY_KEY, bUdpStreamInboundOnly) \ PROCESS(network, UDP_OUTBOUND_ONLY_KEY, bUdpStreamOutboundOnly) \ PROCESS(network, UDP_BIDIRECTIONAL_KEY, bUdpStreamBidirectional) \ PROCESS(network, UDP_AUTO_KEY, bUdpStreamAuto) \

Earlier I was using bool variable for each UDP mode so it was straight forward to map them as shown in the above code. But now with a single enum variable I have no idea how to put it there. Can you please help me find a way to do it?

So by the namespace->enum suggestion you mean the definition of the enum is in the global scope? Not inside 'Settings' class?

Yes. I'm not a big fan of having the enum definitions in the Settings namespace as they are used also (actually, mainly) outside a settings context.

And what about the UDPMode::Auto?

That's a different context I would say. While the first 4 states (Disabled, InboundOnly, OutboundOnly, Both) describe the currently used state of the UDP direction, the Auto mode determines if and how Mumble should switch between these states.
To be more concrete: If at some point we need to check whether we want to send out audio via UDP, we check if we're in OutboundOnly or Both mode. However, we don't care in the slightest whether we're in Auto mode. The latter only comes into play, once we figure that the UDP packets that we send out don't reach their destination.
Thus, we are expressing different concepts here, which is why I would not want to fuse that into a single type.

Earlier I was using bool variable for each UDP mode so it was straight forward to map them as shown in the above code. But now with a single enum variable I have no idea how to put it there. Can you please help me find a way to do it?

In order for this trick to work with enums, we require a bit more boilerplate code for the string <-> enum conversion. This is done in the EnumStringConversion namespace and you will find examples for other enums there. Once you have defined the necessary functions for the new enum, you should be able to use the usual mechanism for storing and saving the settings.

For representing bit-flags, I think we could leverage a symbolic notation like this:

#define UDP_MODES \ PROCESS(UDPMode::Mode, Disabled, "Disabled") \ PROCESS(UDPMode::Modet, InboundOny, "Inbound") \ PROCESS(UDPMode::Mode, OutboundOnly, "Outbound") \ PROCESS(UDPMode::Mode, Both, "Inbound | Outbound")

That's a different context I would say. While the first 4 states (Disabled, InboundOnly, OutboundOnly, Both) describe the currently used state of the UDP direction, the Auto mode determines if and how Mumble should switch between these states. To be more concrete: If at some point we need to check whether we want to send out audio via UDP, we check if we're in OutboundOnly or Both mode. However, we don't care in the slightest whether we're in Auto mode. The latter only comes into play, once we figure that the UDP packets that we send out don't reach their destination. Thus, we are expressing different concepts here, which is why I would not want to fuse that into a single type.

So that means the AUTO is meant to be an additional advanced feature of the UDP mode feature not part of this commit. I am guessing from your first comment on issue #5517.

Well, it could be added later in a separate commit, but if you have an idea on how to make it work, I also don't object to including it in this one. That's up to you.

Krzmbrzl · 2022-07-28T06:23:40Z

src/murmur/Server.cpp

@@ -999,7 +999,9 @@ void Server::run() {
 									handlePing(m_udpDecoder, m_udpPingEncoder, false);

 								QByteArray cache;
-								sendMessage(*u, encodedPing.data(), encodedPing.size(), cache, true);
+								// Reply to UDP ping only if UDP mode is not completely disabled for this client.


I think the server should always reply to UDP connectivity pings as the client might want the server to transmit audio via TCP but it itself might still want to send audio to the server via UDP. For that to work, it must know that the UDP connection to the server is still working as otherwise the client would have to switch to TCP sending as well.

The problem is in the "UDP Outbound Only" mode. The expectation in this setting is that the client will send audio through UDP and server will send back audio to this client using TCP.
But, in a LAN environment, since there will always be UDP connectivity between server and the client the server will switch back to UDP. And therefore, it will basically behave as "UDP Bidirectional" mode.
But I think if there is actually no UDP connectivity between the server and the client, due to NAT, it should work as expected. So do you think we should keep this behaviour?

FYI,
Since the server side only has a boolean flag for "UDP Mode Disabled" or "Forced TCP mode", I made kind of a workaround on the server side to satisfy the above issue. But yes I too think it is not a good idea.

I have created a new PR 5931 on a separate branch with the latest changes.

Krzmbrzl · 2022-07-28T06:25:21Z

src/murmur/Server.cpp

@@ -1041,7 +1043,7 @@ void Server::sendMessage(ServerUser &u, const unsigned char *data, int len, QByt
 	ZoneScoped;

 #if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
-	if ((u.aiUdpFlag.loadRelaxed() == 1 || force) && (u.sUdpSocket != INVALID_SOCKET)) {
+	if (!u.bUdpModeDisabled && (u.aiUdpFlag.loadRelaxed() == 1 || force) && (u.sUdpSocket != INVALID_SOCKET)) {


I think our new flag should probably be handled a level higher (where this function is called) 🤔

Krzmbrzl · 2022-09-08T18:16:54Z

@irakr what's the status here?

irakr · 2022-09-10T09:23:31Z

@irakr what's the status here?

Hey @Krzmbrzl. Sorry, being on a tight schedule with other work. Don't worry my eyes are on this. I will bring an update very soon, in a few days.

Krzmbrzl · 2022-09-23T13:46:22Z

Uhm... What happened here? 👀

davidebeatrici · 2022-09-23T17:41:26Z

Closed by mistake when rebasing against master probably.

@irakr Please use a dedicated branch for the next PR.

irakr · 2022-09-23T19:58:26Z

Uhm... What happened here? 👀

Sorry. Resetted to the new master HEAD, had some conflicts.

irakr · 2022-09-23T19:59:00Z

Closed by mistake when rebasing against master probably.

@irakr Please use a dedicated branch for the next PR.

Ok that would be better. Thanks.

irakr · 2022-10-16T17:40:01Z

New pull request -> 5931

My bad. Had to create a new PR but on a different branch.

irakr force-pushed the master branch from 4c3468a to 713741a Compare May 16, 2022 06:15

Krzmbrzl reviewed May 17, 2022

View reviewed changes

irakr force-pushed the master branch 3 times, most recently from f333df5 to b4ad448 Compare May 22, 2022 08:52

irakr force-pushed the master branch from b4ad448 to a281501 Compare May 23, 2022 06:37

irakr changed the title ~~FEAT(client, server): Add more control over UDP voice stream.~~ FEAT(client, server): Add more control over UDP audio stream. Jun 24, 2022

Krzmbrzl requested changes Jul 25, 2022

View reviewed changes

irakr closed this Jul 27, 2022

irakr force-pushed the master branch from a281501 to 4f50172 Compare July 27, 2022 12:58

irakr reopened this Jul 27, 2022

irakr force-pushed the master branch from eef4a69 to 8a821ea Compare July 27, 2022 16:38

irakr requested a review from Krzmbrzl July 27, 2022 20:00

Krzmbrzl requested changes Jul 28, 2022

View reviewed changes

Krzmbrzl added client server feature-request This issue or PR deals with a new feature labels Aug 31, 2022

irakr closed this Sep 23, 2022

irakr force-pushed the master branch from 8a821ea to c87fb2b Compare September 23, 2022 11:02

	qgbUdpMode->setDisabled((v == 0) ? false : true);
	qgbUdpMode->setDisabled(v != 0);

	Global::get().sh->disableUdpMode(true);
	disableUdpMode(true);

FEAT(client, server): Add more control over UDP audio stream. #5684

FEAT(client, server): Add more control over UDP audio stream. #5684

Conversation

irakr commented May 15, 2022 • edited by Krzmbrzl Loading

Checks

Krzmbrzl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irakr May 17, 2022 • edited Loading

Choose a reason for hiding this comment

Krzmbrzl commented May 23, 2022

irakr commented May 23, 2022

irakr commented Jun 5, 2022

Krzmbrzl commented Jun 5, 2022

irakr commented Jun 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irakr Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irakr Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irakr commented Jul 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irakr Jul 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irakr Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krzmbrzl commented Sep 8, 2022

irakr commented Sep 10, 2022

Krzmbrzl commented Sep 23, 2022

davidebeatrici commented Sep 23, 2022

irakr commented Sep 23, 2022 • edited Loading

irakr commented Sep 23, 2022

irakr commented Oct 16, 2022

irakr commented May 15, 2022 •

edited by Krzmbrzl

Loading

irakr May 17, 2022 •

edited

Loading

irakr Jul 26, 2022 •

edited

Loading

irakr Jul 27, 2022 •

edited

Loading

irakr Jul 30, 2022 •

edited

Loading

irakr Oct 16, 2022 •

edited

Loading

irakr commented Sep 23, 2022 •

edited

Loading