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

(fix) Sound preferences: don't set m_settingsModified in update slots #13450

Merged
merged 2 commits into from
Jul 19, 2024
Merged

(fix) Sound preferences: don't set m_settingsModified in update slots #13450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1ae0b8
Pref Sound: use PollingControlProxy and parented_ptr
ronso0 Jul 8, 2024
5321f91
(fix) Sound preferences: don't set m_settingsModified in update slots
ronso0 Jul 7, 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
105 changes: 56 additions & 49 deletions src/preferences/dialog/dlgprefsound.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@
namespace {

const QString kAppGroup = QStringLiteral("[App]");
const QString kMasterGroup = QStringLiteral("[Master]");
const ConfigKey kKeylockEngingeCfgkey =
ConfigKey(kAppGroup, QStringLiteral("keylock_engine"));
const ConfigKey kKeylockMultiThreadingCfgkey =
ConfigKey(kAppGroup, QStringLiteral("keylock_multithreading"));

bool soundItemAlreadyExists(const AudioPath& output, const QWidget& widget) {
for (const QObject* pObj : widget.children()) {
Expand Down Expand Up @@ -69,6 +74,12 @@ DlgPrefSound::DlgPrefSound(QWidget* pParent,
m_pSoundManager(pSoundManager),
m_pSettings(pSettings),
m_config(pSoundManager.get()),
m_pLatencyCompensation(kMasterGroup, QStringLiteral("microphoneLatencyCompensation")),
m_pMainDelay(kMasterGroup, QStringLiteral("delay")),
m_pHeadDelay(kMasterGroup, QStringLiteral("headDelay")),
m_pBoothDelay(kMasterGroup, QStringLiteral("boothDelay")),
m_pMicMonitorMode(kMasterGroup, QStringLiteral("talkover_mix")),
m_pKeylockEngine(kKeylockEngingeCfgkey),
m_settingsModified(false),
m_bLatencyChanged(false),
m_bSkipConfigClear(true),
Expand Down Expand Up @@ -137,16 +148,11 @@ DlgPrefSound::DlgPrefSound(QWidget* pParent,
}
}

m_pLatencyCompensation = new ControlProxy("[Master]", "microphoneLatencyCompensation", this);
m_pMainDelay = new ControlProxy("[Master]", "delay", this);
m_pHeadDelay = new ControlProxy("[Master]", "headDelay", this);
m_pBoothDelay = new ControlProxy("[Master]", "boothDelay", this);

latencyCompensationSpinBox->setValue(m_pLatencyCompensation->get());
latencyCompensationSpinBox->setValue(m_pLatencyCompensation.get());
latencyCompensationWarningLabel->setWordWrap(true);
mainDelaySpinBox->setValue(m_pMainDelay->get());
headDelaySpinBox->setValue(m_pHeadDelay->get());
boothDelaySpinBox->setValue(m_pBoothDelay->get());
mainDelaySpinBox->setValue(m_pMainDelay.get());
headDelaySpinBox->setValue(m_pHeadDelay.get());
boothDelaySpinBox->setValue(m_pBoothDelay.get());

// TODO These settings are applied immediately via ControlProxies.
// While this is handy for testing the delays, it breaks the rule to
Expand All @@ -168,15 +174,14 @@ DlgPrefSound::DlgPrefSound(QWidget* pParent,
this,
&DlgPrefSound::boothDelaySpinboxChanged);

m_pMicMonitorMode = new ControlProxy("[Master]", "talkover_mix", this);
micMonitorModeComboBox->addItem(tr("Main output only"),
QVariant(static_cast<int>(EngineMixer::MicMonitorMode::Main)));
micMonitorModeComboBox->addItem(tr("Main and booth outputs"),
QVariant(static_cast<int>(EngineMixer::MicMonitorMode::MainAndBooth)));
micMonitorModeComboBox->addItem(tr("Direct monitor (recording and broadcasting only)"),
QVariant(static_cast<int>(EngineMixer::MicMonitorMode::DirectMonitor)));
int modeIndex = micMonitorModeComboBox->findData(
static_cast<int>(m_pMicMonitorMode->get()));
static_cast<int>(m_pMicMonitorMode.get()));
micMonitorModeComboBox->setCurrentIndex(modeIndex);
micMonitorModeComboBoxChanged(modeIndex);
connect(micMonitorModeComboBox,
Expand Down Expand Up @@ -211,6 +216,10 @@ DlgPrefSound::DlgPrefSound(QWidget* pParent,
QOverload<int>::of(&QComboBox::currentIndexChanged),
this,
&DlgPrefSound::settingChanged);
connect(keylockComboBox,
QOverload<int>::of(&QComboBox::currentIndexChanged),
this,
&DlgPrefSound::updateKeylockDualThreadingCheckbox);
#ifdef __RUBBERBAND__
connect(keylockDualthreadedCheckBox,
&QCheckBox::clicked,
Expand Down Expand Up @@ -240,16 +249,18 @@ DlgPrefSound::DlgPrefSound(QWidget* pParent,
loadSettings();
});

m_pAudioLatencyOverloadCount =
new ControlProxy(kAppGroup, QStringLiteral("audio_latency_overload_count"), this);
m_pAudioLatencyOverloadCount = make_parented<ControlProxy>(
kAppGroup, QStringLiteral("audio_latency_overload_count"), this);
m_pAudioLatencyOverloadCount->connectValueChanged(this, &DlgPrefSound::bufferUnderflow);

m_pOutputLatencyMs = new ControlProxy(kAppGroup, QStringLiteral("output_latency_ms"), this);
m_pOutputLatencyMs = make_parented<ControlProxy>(
kAppGroup, QStringLiteral("output_latency_ms"), this);
m_pOutputLatencyMs->connectValueChanged(this, &DlgPrefSound::outputLatencyChanged);

// TODO: remove this option by automatically disabling/enabling the main mix
// when recording, broadcasting, headphone, and main outputs are enabled/disabled
m_pMainEnabled = new ControlProxy("[Master]", "enabled", this);
m_pMainEnabled =
make_parented<ControlProxy>(kMasterGroup, QStringLiteral("enabled"), this);
mainMixComboBox->addItem(tr("Disabled"));
mainMixComboBox->addItem(tr("Enabled"));
mainMixComboBox->setCurrentIndex(m_pMainEnabled->toBool() ? 1 : 0);
Expand All @@ -259,7 +270,8 @@ DlgPrefSound::DlgPrefSound(QWidget* pParent,
&DlgPrefSound::mainMixChanged);
m_pMainEnabled->connectValueChanged(this, &DlgPrefSound::mainEnabledChanged);

m_pMainMonoMixdown = new ControlProxy("[Master]", "mono_mixdown", this);
m_pMainMonoMixdown =
make_parented<ControlProxy>(kMasterGroup, QStringLiteral("mono_mixdown"), this);
mainOutputModeComboBox->addItem(tr("Stereo"));
mainOutputModeComboBox->addItem(tr("Mono"));
mainOutputModeComboBox->setCurrentIndex(m_pMainMonoMixdown->toBool() ? 1 : 0);
Expand All @@ -269,9 +281,6 @@ DlgPrefSound::DlgPrefSound(QWidget* pParent,
&DlgPrefSound::mainOutputModeComboBoxChanged);
m_pMainMonoMixdown->connectValueChanged(this, &DlgPrefSound::mainMonoMixdownChanged);

m_pKeylockEngine =
new ControlProxy(kAppGroup, QStringLiteral("keylock_engine"), this);

#ifdef __LINUX__
qDebug() << "RLimit Cur " << RLimit::getCurRtPrio();
qDebug() << "RLimit Max " << RLimit::getMaxRtPrio();
Expand Down Expand Up @@ -306,7 +315,6 @@ DlgPrefSound::DlgPrefSound(QWidget* pParent,
void DlgPrefSound::slotUpdate() {
m_bSkipConfigClear = true;
loadSettings();
settingChanged();
checkLatencyCompensation();
m_bSkipConfigClear = false;
}
Expand All @@ -326,14 +334,14 @@ void DlgPrefSound::slotApply() {
ScopedWaitCursor cursor;
const auto keylockEngine =
keylockComboBox->currentData().value<EngineBuffer::KeylockEngine>();
m_pKeylockEngine->set(static_cast<double>(keylockEngine));
m_pSettings->set(ConfigKey("[Master]", "keylock_engine"),
m_pKeylockEngine.set(static_cast<double>(keylockEngine));
m_pSettings->set(kKeylockEngingeCfgkey,
ConfigValue(static_cast<int>(keylockEngine)));

#ifdef __RUBBERBAND__
bool keylockMultithreading = m_pSettings->getValue(
ConfigKey(kAppGroup, "keylock_multithreading"), false);
m_pSettings->setValue(ConfigKey(kAppGroup, "keylock_multithreading"),
kKeylockMultiThreadingCfgkey, false);
m_pSettings->setValue(kKeylockMultiThreadingCfgkey,
keylockDualthreadedCheckBox->isChecked() &&
keylockDualthreadedCheckBox->isEnabled());
if (keylockMultithreading !=
Expand All @@ -357,6 +365,7 @@ void DlgPrefSound::slotApply() {
m_bSkipConfigClear = true;
loadSettings(); // in case SM decided to change anything it didn't like
checkLatencyCompensation();
updateKeylockDualThreadingCheckbox();
m_bSkipConfigClear = false;
}

Expand Down Expand Up @@ -527,7 +536,7 @@ void DlgPrefSound::loadSettings(const SoundManagerConfig& config) {

// Default keylock engine is Rubberband Faster (v2)
const auto keylockEngine = static_cast<EngineBuffer::KeylockEngine>(
m_pSettings->getValue(ConfigKey("[Master]", "keylock_engine"),
m_pSettings->getValue(kKeylockEngingeCfgkey,
static_cast<int>(EngineBuffer::defaultKeylockEngine())));
const auto keylockEngineVariant = QVariant::fromValue(keylockEngine);
const int index = keylockComboBox->findData(keylockEngineVariant);
Expand All @@ -542,7 +551,7 @@ void DlgPrefSound::loadSettings(const SoundManagerConfig& config) {
#ifdef __RUBBERBAND__
// Default is no multi threading on keylock
keylockDualthreadedCheckBox->setChecked(m_pSettings->getValue(
ConfigKey(kAppGroup, QStringLiteral("keylock_multithreading")),
kKeylockMultiThreadingCfgkey,
false));
#endif

Expand Down Expand Up @@ -742,6 +751,9 @@ void DlgPrefSound::settingChanged() {
m_settingsModified = true;

#ifdef __RUBBERBAND__
}

void DlgPrefSound::updateKeylockDualThreadingCheckbox() {
Comment on lines 753 to +756
Copy link
Member

Choose a reason for hiding this comment

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

This #ifdef looks suspicious, does it work as intended? I'm getting some errors building for iOS (without Rubberband)...

Copy link
Member

Choose a reason for hiding this comment

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

Seems to work, but the brace placement makes this confusing. I've fixed this and some related issues in

bool supportedScaler = keylockComboBox->currentData()
.value<EngineBuffer::KeylockEngine>() !=
EngineBuffer::KeylockEngine::SoundTouch;
Expand Down Expand Up @@ -776,6 +788,8 @@ void DlgPrefSound::updateKeylockMultithreading(bool enabled) {
msg.setDefaultButton(pNoBtn);
msg.exec();
keylockDualthreadedCheckBox->setChecked(msg.clickedButton() == pYesBtn);

updateKeylockDualThreadingCheckbox();
#endif
}

Expand All @@ -784,7 +798,7 @@ void DlgPrefSound::updateKeylockMultithreading(bool enabled) {
/// opens this page to allow users to make adjustments in case configured
/// devices are busy/missing.
/// The issue is that the visual state (combobox(es) with 'None') does not match
/// the untouched config state. This set the modified flag so slotApply() will
/// the untouched config state. This sets the modified flag so slotApply() will
/// apply the (seemingly) unchanged configuration if users simply click Apply/Okay
/// because they are okay to continue without these devices.
void DlgPrefSound::configuredDeviceNotFound() {
Expand Down Expand Up @@ -871,22 +885,22 @@ void DlgPrefSound::slotResetToDefaults() {
if (index >= 0) {
keylockComboBox->setCurrentIndex(index);
}
m_pKeylockEngine->set(static_cast<double>(keylockEngine));
m_pKeylockEngine.set(static_cast<double>(keylockEngine));

mainMixComboBox->setCurrentIndex(1);
m_pMainEnabled->set(1.0);

mainDelaySpinBox->setValue(0.0);
m_pMainDelay->set(0.0);
m_pMainDelay.set(0.0);

headDelaySpinBox->setValue(0.0);
m_pHeadDelay->set(0.0);
m_pHeadDelay.set(0.0);

boothDelaySpinBox->setValue(0.0);
m_pBoothDelay->set(0.0);
m_pBoothDelay.set(0.0);

// Enable talkover main output
m_pMicMonitorMode->set(
m_pMicMonitorMode.set(
static_cast<double>(
static_cast<int>(EngineMixer::MicMonitorMode::Main)));
micMonitorModeComboBox->setCurrentIndex(
Expand All @@ -895,7 +909,8 @@ void DlgPrefSound::slotResetToDefaults() {

latencyCompensationSpinBox->setValue(latencyCompensationSpinBox->minimum());

settingChanged(); // force the apply button to enable
settingChanged();
updateKeylockDualThreadingCheckbox();
}

void DlgPrefSound::bufferUnderflow(double count) {
Expand All @@ -909,20 +924,20 @@ void DlgPrefSound::outputLatencyChanged(double latency) {
}

void DlgPrefSound::latencyCompensationSpinboxChanged(double value) {
m_pLatencyCompensation->set(value);
m_pLatencyCompensation.set(value);
checkLatencyCompensation();
}

void DlgPrefSound::mainDelaySpinboxChanged(double value) {
m_pMainDelay->set(value);
m_pMainDelay.set(value);
}

void DlgPrefSound::headDelaySpinboxChanged(double value) {
m_pHeadDelay->set(value);
m_pHeadDelay.set(value);
}

void DlgPrefSound::boothDelaySpinboxChanged(double value) {
m_pBoothDelay->set(value);
m_pBoothDelay.set(value);
}

void DlgPrefSound::mainMixChanged(int value) {
Expand All @@ -938,15 +953,7 @@ void DlgPrefSound::mainOutputModeComboBoxChanged(int value) {
m_pMainMonoMixdown->set(static_cast<double>(value));

#ifdef __RUBBERBAND__
bool supportedScaler = keylockComboBox->currentData()
.value<EngineBuffer::KeylockEngine>() !=
EngineBuffer::KeylockEngine::SoundTouch;
keylockDualthreadedCheckBox->setEnabled(!value && supportedScaler);
keylockDualthreadedCheckBox->setToolTip(
value ? kKeylockMultiThreadedUnavailableMono
: (supportedScaler
? kKeylockMultiThreadedAvailable
: kKeylockMultiThreadedUnavailableRubberband));
updateKeylockDualThreadingCheckbox();
#endif
}

Expand All @@ -960,15 +967,15 @@ void DlgPrefSound::micMonitorModeComboBoxChanged(int value) {
static_cast<EngineMixer::MicMonitorMode>(
micMonitorModeComboBox->itemData(value).toInt());

m_pMicMonitorMode->set(static_cast<double>(newMode));
m_pMicMonitorMode.set(static_cast<double>(newMode));

checkLatencyCompensation();
}

void DlgPrefSound::checkLatencyCompensation() {
EngineMixer::MicMonitorMode configuredMicMonitorMode =
static_cast<EngineMixer::MicMonitorMode>(
static_cast<int>(m_pMicMonitorMode->get()));
static_cast<int>(m_pMicMonitorMode.get()));

// Do not clear the SoundManagerConfig on startup, from slotApply, or from slotUpdate
if (!m_bSkipConfigClear) {
Expand All @@ -984,7 +991,7 @@ void DlgPrefSound::checkLatencyCompensation() {
latencyCompensationSpinBox->setEnabled(true);
QString lineBreak("<br/>");
// TODO(Be): Make the "User Manual" text link to the manual.
if (m_pLatencyCompensation->get() == 0.0) {
if (m_pLatencyCompensation.get() == 0.0) {
latencyCompensationWarningLabel->setText(kWarningIconHtmlString +
tr("Microphone inputs are out of time in the record & "
"broadcast signal compared to what you hear.") +
Expand Down
26 changes: 16 additions & 10 deletions src/preferences/dialog/dlgprefsound.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@

#include <memory>

#include "control/pollingcontrolproxy.h"
#include "defs_urls.h"
#include "preferences/dialog/dlgpreferencepage.h"
#include "preferences/dialog/ui_dlgprefsounddlg.h"
#include "preferences/usersettings.h"
#include "soundio/sounddevice.h"
#include "soundio/sounddevicestatus.h"
#include "soundio/soundmanagerconfig.h"
#include "util/parented_ptr.h"

class SoundManager;
class PlayerManager;
Expand Down Expand Up @@ -73,6 +75,7 @@ class DlgPrefSound : public DlgPreferencePage, public Ui::DlgPrefSoundDlg {
void configuredDeviceNotFound();
void queryClicked();
#ifdef __RUBBERBAND__
void updateKeylockDualThreadingCheckbox();
void updateKeylockMultithreading(bool enabled);
#endif

Expand All @@ -86,16 +89,19 @@ class DlgPrefSound : public DlgPreferencePage, public Ui::DlgPrefSoundDlg {
std::shared_ptr<SoundManager> m_pSoundManager;
UserSettingsPointer m_pSettings;
SoundManagerConfig m_config;
ControlProxy* m_pAudioLatencyOverloadCount;
ControlProxy* m_pOutputLatencyMs;
ControlProxy* m_pHeadDelay;
ControlProxy* m_pMainDelay;
ControlProxy* m_pBoothDelay;
ControlProxy* m_pLatencyCompensation;
ControlProxy* m_pKeylockEngine;
ControlProxy* m_pMainEnabled;
ControlProxy* m_pMainMonoMixdown;
ControlProxy* m_pMicMonitorMode;

PollingControlProxy m_pLatencyCompensation;
PollingControlProxy m_pMainDelay;
PollingControlProxy m_pHeadDelay;
PollingControlProxy m_pBoothDelay;
PollingControlProxy m_pMicMonitorMode;
PollingControlProxy m_pKeylockEngine;

parented_ptr<ControlProxy> m_pAudioLatencyOverloadCount;
parented_ptr<ControlProxy> m_pOutputLatencyMs;
parented_ptr<ControlProxy> m_pMainEnabled;
parented_ptr<ControlProxy> m_pMainMonoMixdown;

QList<SoundDevicePointer> m_inputDevices;
QList<SoundDevicePointer> m_outputDevices;
QHash<DlgPrefSoundItem*, QPair<SoundDeviceId, int>> m_selectedOutputChannelIndices;
Expand Down
Loading