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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b83f5ef
REFAC(client): Implement loadComboBox() that sets the index based on …
davidebeatrici Jun 1, 2024
361754e
FEAT(client, server): Implement loopback while still sending to others
davidebeatrici Jun 1, 2024
d4dd9c2
TRANSLATION: Update translation files
davidebeatrici Jun 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/MumbleProtocol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,9 @@ namespace Protocol {
};

namespace ReservedTargetIDs {
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;
Hartmnt marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SERVER_LOOPBACK_BROADCAST?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

constexpr unsigned int SERVER_LOOPBACK_ONLY = 31;
} // namespace ReservedTargetIDs

using audio_context_t = byte;
Expand Down
10 changes: 6 additions & 4 deletions src/mumble/AudioConfigDialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,8 +640,10 @@ AudioOutputDialog::AudioOutputDialog(Settings &st) : ConfigWidget(st) {
qcbSystem->setEnabled(qcbSystem->count() > 1);

qcbLoopback->addItem(tr("None"), Settings::None);
qcbLoopback->addItem(tr("Local"), Settings::Local);
qcbLoopback->addItem(tr("Server"), Settings::Server);
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);
Comment on lines +643 to +646
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, absolutely.


qcbDevice->view()->setTextElideMode(Qt::ElideRight);

Expand Down Expand Up @@ -729,7 +731,7 @@ void AudioOutputDialog::load(const Settings &r) {
enablePulseAudioAttenuationOptionsFor(AudioOutputRegistrar::current);

loadSlider(qsJitter, r.iJitterBufferSize);
loadComboBox(qcbLoopback, r.lmLoopMode);
loadComboBox(qcbLoopback, QVariant::fromValue(r.lmLoopMode));
loadSlider(qsPacketDelay, static_cast< int >(r.dMaxPacketDelay));
loadSlider(qsPacketLoss, static_cast< int >(r.dPacketLoss * 100.0f + 0.5f));
qsbMinimumDistance->setValue(r.fAudioMinDistance);
Expand All @@ -755,7 +757,7 @@ void AudioOutputDialog::save() const {
s.bAttenuateUsersOnPrioritySpeak = qcbAttenuateUsersOnPrioritySpeak->isChecked();
s.iJitterBufferSize = qsJitter->value();
s.qsAudioOutput = qcbSystem->currentText();
s.lmLoopMode = static_cast< Settings::LoopMode >(qcbLoopback->currentIndex());
s.lmLoopMode = qcbLoopback->currentData().value< Settings::LoopMode >();
s.dMaxPacketDelay = static_cast< float >(qsPacketDelay->value());
s.dPacketLoss = static_cast< float >(qsPacketLoss->value()) / 100.0f;
s.fAudioMinDistance = static_cast< float >(qsbMinimumDistance->value());
Expand Down
37 changes: 26 additions & 11 deletions src/mumble/AudioInput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

|| Global::get().bPushToMute || (voiceTargetID < 0)) {
bTalkingWhenMuted = bIsSpeech;
bIsSpeech = false;
Expand Down Expand Up @@ -1173,8 +1174,16 @@ void AudioInput::flushCheck(const QByteArray &frame, bool terminator, std::int32
// accordingly once the client whispers for the next time.
Global::get().iPrevTarget = 0;
}
if (Global::get().s.lmLoopMode == Settings::Server) {
audioData.targetOrContext = Mumble::Protocol::ReservedTargetIDs::SERVER_LOOPBACK;

switch (Global::get().s.lmLoopMode) {
case Settings::ServerOnly:
audioData.targetOrContext = Mumble::Protocol::ReservedTargetIDs::SERVER_LOOPBACK_ONLY;
break;
case Settings::ServerRegular:
audioData.targetOrContext = Mumble::Protocol::ReservedTargetIDs::SERVER_LOOPBACK_REGULAR;
break;
default:
break;
Comment on lines +1185 to +1186
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

audioData.usedCodec = m_codec;
Expand Down Expand Up @@ -1215,14 +1224,20 @@ void AudioInput::flushCheck(const QByteArray &frame, bool terminator, std::int32
}
}

if (Global::get().s.lmLoopMode == Settings::Local) {
// Only add audio data to local loop buffer
LoopUser::lpLoopy.addFrame(audioData);
} else {
// Encode audio frame and send out
gsl::span< const Mumble::Protocol::byte > encodedAudioPacket = m_udpEncoder.encodeAudioPacket(audioData);

sendAudioFrame(encodedAudioPacket);
switch (Global::get().s.lmLoopMode) {
case Settings::LocalOnly:
// Only add audio data to local loop buffer
LoopUser::lpLoopy.addFrame(audioData);
break;
case Settings::LocalRegular:
LoopUser::lpLoopy.addFrame(audioData);
[[fallthrough]];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

default: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

// Encode audio frame and send out
gsl::span< const Mumble::Protocol::byte > encodedAudioPacket = m_udpEncoder.encodeAudioPacket(audioData);

sendAudioFrame(encodedAudioPacket);
}
}

qlFrames.clear();
Expand Down
2 changes: 1 addition & 1 deletion src/mumble/AudioWizard.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ AudioWizard::AudioWizard(QWidget *p) : QWizard(p) {

updateTriggerWidgets(qrPTT->isChecked());
sOldSettings = Global::get().s;
Global::get().s.lmLoopMode = Settings::Local;
Global::get().s.lmLoopMode = Settings::LocalOnly;
Global::get().s.dPacketLoss = 0.0;
Global::get().s.dMaxPacketDelay = 0.0;
Global::get().s.bMute = true;
Expand Down
15 changes: 15 additions & 0 deletions src/mumble/ConfigWidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,18 @@ void ConfigWidget::loadComboBox(QComboBox *c, int v) {
disconnect(SIGNAL(intSignal(int)));
}
}

void ConfigWidget::loadComboBox(QComboBox *c, const QVariant &v) {
const int index = c->findData(v);
if (index == -1) {
return;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think consistency is important for this 🤷

emit intSignal(index);
disconnect(SIGNAL(intSignal(int)));
}
}
1 change: 1 addition & 0 deletions src/mumble/ConfigWidget.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class ConfigWidget : public QWidget {
void loadSlider(QSlider *, int);
void loadCheckBox(QAbstractButton *, bool);
void loadComboBox(QComboBox *, int);
void loadComboBox(QComboBox *, const QVariant &);
signals:
void intSignal(int);

Expand Down
10 changes: 6 additions & 4 deletions src/mumble/EnumStringConversions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@
PROCESS(Settings::VADSource, Amplitude, "Amplitude") \
PROCESS(Settings::VADSource, SignalToNoise, "SignalToNoise")

#define LOOP_MODE_VALUES \
PROCESS(Settings::LoopMode, None, "None") \
PROCESS(Settings::LoopMode, Local, "Local") \
PROCESS(Settings::LoopMode, Server, "Server")
#define LOOP_MODE_VALUES \
PROCESS(Settings::LoopMode, None, "None") \
PROCESS(Settings::LoopMode, LocalOnly, "LocalOnly") \
PROCESS(Settings::LoopMode, ServerOnly, "ServerOnly") \
PROCESS(Settings::LoopMode, LocalRegular, "LocalRegular") \
PROCESS(Settings::LoopMode, ServerRegular, "ServerRegular")

#define CHANNEL_EXPAND_VALUES \
PROCESS(Settings::ChannelExpand, NoChannels, "NoChannels") \
Expand Down
4 changes: 3 additions & 1 deletion src/mumble/Settings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

enum ChannelExpand { NoChannels, ChannelsWithUsers, AllChannels };
enum ChannelDrag { Ask, DoNothing, Move };
enum ServerShow { ShowPopulated, ShowReachable, ShowAll };
Expand Down Expand Up @@ -579,4 +579,6 @@ struct Settings {
QString findSettingsLocation(bool legacy = false, bool *foundExistingFile = nullptr) const;
};

Q_DECLARE_METATYPE(Settings::LoopMode)

#endif // MUMBLE_MUMBLE_SETTINGS_H_
24 changes: 16 additions & 8 deletions src/mumble/mumble_ar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1631,14 +1631,6 @@ This value allows you to set the maximum number of users allowed in the channel.
<source>None</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Local</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Server</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Audio Output</source>
<translation type="unfinished"></translation>
Expand Down Expand Up @@ -1675,6 +1667,22 @@ This value allows you to set the maximum number of users allowed in the channel.
<source>meters</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Local (don&apos;t send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Local (send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Server (don&apos;t send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Server (send to others)</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>AudioOutputSample</name>
Expand Down
24 changes: 16 additions & 8 deletions src/mumble/mumble_bg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1632,14 +1632,6 @@ This value allows you to set the maximum number of users allowed in the channel.
<source>None</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Local</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Server</source>
<translation>Сървър</translation>
</message>
<message>
<source>Audio Output</source>
<translation>Звуков изход</translation>
Expand Down Expand Up @@ -1676,6 +1668,22 @@ This value allows you to set the maximum number of users allowed in the channel.
<source>meters</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Local (don&apos;t send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Local (send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Server (don&apos;t send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Server (send to others)</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>AudioOutputSample</name>
Expand Down
24 changes: 16 additions & 8 deletions src/mumble/mumble_br.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1631,14 +1631,6 @@ This value allows you to set the maximum number of users allowed in the channel.
<source>None</source>
<translation>Hini ebet</translation>
</message>
<message>
<source>Local</source>
<translation>Lec&apos;hel</translation>
</message>
<message>
<source>Server</source>
<translation>Servijer</translation>
</message>
<message>
<source>Audio Output</source>
<translation type="unfinished"></translation>
Expand Down Expand Up @@ -1675,6 +1667,22 @@ This value allows you to set the maximum number of users allowed in the channel.
<source>meters</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Local (don&apos;t send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Local (send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Server (don&apos;t send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Server (send to others)</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>AudioOutputSample</name>
Expand Down
24 changes: 16 additions & 8 deletions src/mumble/mumble_ca.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1639,14 +1639,6 @@ Aquest valor us permet establir el nombre màxim d&apos;usuaris permesos al cana
<source>None</source>
<translation>Cap</translation>
</message>
<message>
<source>Local</source>
<translation>Local</translation>
</message>
<message>
<source>Server</source>
<translation>Servidor</translation>
</message>
<message>
<source>Audio Output</source>
<translation>Sortida d&apos;àudio</translation>
Expand Down Expand Up @@ -1683,6 +1675,22 @@ Aquest valor us permet establir el nombre màxim d&apos;usuaris permesos al cana
<source>meters</source>
<translation>metres</translation>
</message>
<message>
<source>Local (don&apos;t send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Local (send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Server (don&apos;t send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Server (send to others)</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>AudioOutputSample</name>
Expand Down
26 changes: 17 additions & 9 deletions src/mumble/mumble_cs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1639,14 +1639,6 @@ Tato hodnota Vám umožňuje nastavit maximální počet povolených uživatelů
<source>None</source>
<translation>Žádná</translation>
</message>
<message>
<source>Local</source>
<translation>Místní</translation>
</message>
<message>
<source>Server</source>
<translation>Server</translation>
</message>
<message>
<source>Audio Output</source>
<translation>Výstup Zvuku</translation>
Expand Down Expand Up @@ -1683,6 +1675,22 @@ Tato hodnota Vám umožňuje nastavit maximální počet povolených uživatelů
<source>meters</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Local (don&apos;t send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Local (send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Server (don&apos;t send to others)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Server (send to others)</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>AudioOutputSample</name>
Expand Down Expand Up @@ -3331,7 +3339,7 @@ Jste si jisti, že chcete certifikát nahradit?
</message>
<message>
<source>Server</source>
<translation type="unfinished"></translation>
<translation type="unfinished">Serveru</translation>
</message>
</context>
<context>
Expand Down
Loading
Loading