FEAT(client, server): Implement loopback while still sending to others #6445

davidebeatrici · 2024-06-01T18:36:52Z

No description provided.

…the data The function that already existed took the index instead of the data, making it susceptible to errors because the data could be passed instead of the index. Also, with this new iteration of loadComboBox() we can reorder the items shown to the user as desired.

Hartmnt · 2024-06-02T11:58:47Z

src/mumble/AudioInput.cpp

@@ -992,7 +992,8 @@ void AudioInput::encodeAudioFrame(AudioChunk chunk) {

 	ClientUser *p          = ClientUser::get(Global::get().uiSession);
 	bool bTalkingWhenMuted = false;
-	if (Global::get().s.bMute || ((Global::get().s.lmLoopMode != Settings::Local) && p && (p->bMute || p->bSuppress))
+	if (Global::get().s.bMute
+		|| ((Global::get().s.lmLoopMode != Settings::LocalOnly) && p && (p->bMute || p->bSuppress))


I find this if now even harder to comprehend. Let's maybe create some bools before the if and compare that.

Hartmnt · 2024-06-02T11:59:38Z

src/mumble/AudioInput.cpp

+			break;
+		case Settings::LocalRegular:
+			LoopUser::lpLoopy.addFrame(audioData);
+			[[fallthrough]];


[[fallthrough]] is apparently a C++17 thing and therefore the Windows and OSX CI build fails (?)

src/MumbleProtocol.h

Hartmnt · 2024-06-02T12:48:21Z

src/murmur/Server.cpp

 		}
-	} else if (u->qmTargets.contains(static_cast< int >(audioData.targetOrContext))) { // Whisper/Shout


What happens when clients implementing this feature are connected to old servers? When they are using "server (send to others)" their packets will be dropped entirely, correct? Or will they be whisper/shout to some channel or user?

They are requesting the audio to be sent to a non-registered whisper-target, which should lead to the packet just being dropped.

The new loopback mode should probably only be used/offered to the user whilst connected to a server that actually supports this.

I was thinking about only showing the option in the combobox when available (i.e. the server supports it), but the idea implies that:

It would not appear when not connected to a server.

It would be confusing as the user may think their client version doesn't provide the feature.

I think we should show a warning in the log box explaining that the selected loopback method is not supported due to the server not implementing it.

I think hiding the option if the server doesn't support it makes sense. Only issue would be that it'd be confusing for users who configure loopback while disconnected and once they connect their chosen option is gone.

I am not sure hiding would be a good UX. Either change the text of the combobox entries to add (unsupported by server) or use the log

The log will be overlooked in like 90% of cases.

Agreed, but hiding will create us a dozen bug reports xD

True :)
We could make use of the mute-cue in cases where the user has selected loopback+broadcast but the server doesn't support it - because as far as other clients are concerned, you are muted (more or less). So maybe mute-cue + indicating in the settings window that the server doesn't support the selected loopback+broadcast would be an idea?

Krzmbrzl · 2024-06-02T14:11:57Z

src/MumbleProtocol.h

-		constexpr unsigned int REGULAR_SPEECH  = 0;
-		constexpr unsigned int SERVER_LOOPBACK = 31;
+		constexpr unsigned int REGULAR_SPEECH          = 0;
+		constexpr unsigned int SERVER_LOOPBACK_REGULAR = 30;


I think the naming "regular" is not very helpful in this case. Maybe SERVER_LOOPBACK_ADDITIONAL or something in that direction makes things a bit more expressive? 🤔

SERVER_LOOPBACK_BROADCAST?

SERVER_LOOPBACK_AND_BROADCAST <- that would nicely describe what's going on

I actually thought about using "broadcast", but ended up with "regular" because shorter. I agree that the former would be clearer.

Krzmbrzl · 2024-06-02T14:14:45Z

src/mumble/AudioConfigDialog.cpp

+	qcbLoopback->addItem(tr("Local (don't send to others)"), Settings::LocalOnly);
+	qcbLoopback->addItem(tr("Local (send to others)"), Settings::LocalRegular);
+	qcbLoopback->addItem(tr("Server (don't send to others)"), Settings::ServerOnly);
+	qcbLoopback->addItem(tr("Server (send to others)"), Settings::ServerRegular);


Suggested change

qcbLoopback->addItem(tr("Local (don't send to others)"), Settings::LocalOnly);

qcbLoopback->addItem(tr("Local (send to others)"), Settings::LocalRegular);

qcbLoopback->addItem(tr("Server (don't send to others)"), Settings::ServerOnly);

qcbLoopback->addItem(tr("Server (send to others)"), Settings::ServerRegular);

qcbLoopback->addItem(tr("Local only"), Settings::LocalOnly);

qcbLoopback->addItem(tr("Local + Broadcast"), Settings::LocalRegular);

qcbLoopback->addItem(tr("Server only"), Settings::ServerOnly);

qcbLoopback->addItem(tr("Server + Broadcast"), Settings::ServerRegular);

I feel like my suggestion can still be improved upon 🤔

Either way, I think we need tooltips for all of these that explain in more detail what is meant by that

Yes, absolutely.

Krzmbrzl · 2024-06-02T14:16:06Z

src/mumble/AudioInput.cpp

+		default:
+			break;


We should not include default cases when switching over enums as that will prevent compiler warnings once new enum values are added.

Krzmbrzl · 2024-06-02T14:16:53Z

src/mumble/AudioInput.cpp

+		case Settings::LocalRegular:
+			LoopUser::lpLoopy.addFrame(audioData);
+			[[fallthrough]];
+		default: {


Same as above

Krzmbrzl · 2024-06-02T14:18:16Z

src/mumble/ConfigWidget.cpp

+	if (c->currentIndex() != index) {
+		c->setCurrentIndex(index);
+	} else {
+		connect(this, SIGNAL(intSignal(int)), c, SIGNAL(currentIndexChanged(int)));


Old signal/slot syntax should no longer be used - prefer the version with explicit function pointers where the compiler can actually do type-checking for us.

I only did this for consistency with the rest of the file.

I don't think consistency is important for this 🤷

Krzmbrzl · 2024-06-02T14:19:42Z

src/mumble/Settings.h

@@ -188,7 +188,7 @@ struct OverlaySettings {
 struct Settings {
 	enum AudioTransmit { Continuous, VAD, PushToTalk };
 	enum VADSource { Amplitude, SignalToNoise };
-	enum LoopMode { None, Local, Server };
+	enum LoopMode { None, LocalOnly, ServerOnly, LocalRegular, ServerRegular };


As before, I don't think "regular" is descriptive - should be adapted in unison with the special whisper target.

Krzmbrzl · 2024-06-02T14:20:41Z

src/murmur/Server.cpp

+			break;
+		case Mumble::Protocol::ReservedTargetIDs::SERVER_LOOPBACK_REGULAR:
+			buffer.forceAddReceiver(*u, Mumble::Protocol::AudioContext::NORMAL, audioData.containsPositionalData);
+			[[fallthrough]];


Another C++17 usage

Krzmbrzl · 2024-06-02T14:22:23Z

src/murmur/Server.cpp

-			cachedListeners                 = cache.listeningTargets;
-		} else {
-			ZoneScopedN(TracyConstants::AUDIO_WHISPER_CACHE_CREATE);
+		default:


No default-cases in enum switches

Hartmnt

I just realized another important thing: We will have to take echo cancellation into account. Because we might effectively cancel the user speech entirely, if not careful.

Hartmnt · 2024-06-02T15:37:36Z

BTW, you are effectively implementing #3408

Krzmbrzl · 2024-06-02T15:54:00Z

We will have to take echo cancellation into account. Because we might effectively cancel the user speech entirely, if not careful.

I would assume this isn't an issue as the loopback variants already exist and work - the only changes of this PR are not visible to the local client.

davidebeatrici · 2024-06-02T21:43:05Z

Correct.

davidebeatrici added client server labels Jun 1, 2024

davidebeatrici requested review from Hartmnt and Krzmbrzl June 1, 2024 18:36

davidebeatrici force-pushed the server-loopback-sending-to-others branch from 2e47e0f to 1730f39 Compare June 1, 2024 22:42

davidebeatrici changed the title ~~FEAT(client, server): Implement server loopback while still sending to others~~ FEAT(client, server): Implement loopback while still sending to others Jun 1, 2024

davidebeatrici added 2 commits June 2, 2024 01:02

FEAT(client, server): Implement loopback while still sending to others

361754e

TRANSLATION: Update translation files

d4dd9c2

davidebeatrici force-pushed the server-loopback-sending-to-others branch from 1730f39 to d4dd9c2 Compare June 1, 2024 23:03

Hartmnt reviewed Jun 2, 2024

View reviewed changes

Krzmbrzl requested changes Jun 2, 2024

View reviewed changes

Hartmnt reviewed Jun 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT(client, server): Implement loopback while still sending to others #6445

FEAT(client, server): Implement loopback while still sending to others #6445

davidebeatrici commented Jun 1, 2024

Hartmnt Jun 2, 2024

davidebeatrici Jun 2, 2024

Hartmnt Jun 2, 2024

Hartmnt Jun 2, 2024

Krzmbrzl Jun 2, 2024

Krzmbrzl Jun 2, 2024

davidebeatrici Jun 2, 2024

Krzmbrzl Jun 3, 2024

Hartmnt Jun 3, 2024

Krzmbrzl Jun 3, 2024

Hartmnt Jun 3, 2024

Krzmbrzl Jun 5, 2024

Krzmbrzl Jun 2, 2024

Hartmnt Jun 2, 2024

Krzmbrzl Jun 2, 2024

davidebeatrici Jun 2, 2024

Krzmbrzl Jun 2, 2024

davidebeatrici Jun 2, 2024

Krzmbrzl Jun 2, 2024

Krzmbrzl Jun 2, 2024

Krzmbrzl Jun 2, 2024

davidebeatrici Jun 2, 2024

Krzmbrzl Jun 3, 2024

Krzmbrzl Jun 2, 2024

Krzmbrzl Jun 2, 2024

Krzmbrzl Jun 2, 2024

Hartmnt left a comment

Hartmnt commented Jun 2, 2024

Krzmbrzl commented Jun 2, 2024

davidebeatrici commented Jun 2, 2024

		}
		} else if (u->qmTargets.contains(static_cast< int >(audioData.targetOrContext))) { // Whisper/Shout

FEAT(client, server): Implement loopback while still sending to others #6445

Are you sure you want to change the base?

FEAT(client, server): Implement loopback while still sending to others #6445

Conversation

davidebeatrici commented Jun 1, 2024

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

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

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

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

Hartmnt left a comment

Choose a reason for hiding this comment

Hartmnt commented Jun 2, 2024

Krzmbrzl commented Jun 2, 2024

davidebeatrici commented Jun 2, 2024