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

Pref > Mixer: apply & save settings only in slotApply(), fix bugs, improve UX #11527

Merged
merged 53 commits into from
Oct 16, 2023
Merged

Pref > Mixer: apply & save settings only in slotApply(), fix bugs, improve UX #11527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
f888df7
Pref Mixer, ui: set xfader button group in ui file
ronso0 Apr 25, 2023
c400306
Pref Mixer, ui: fix widget tab focus order
ronso0 Apr 25, 2023
ca18756
Pref Mixer, xFader: update settings only when touching xfader widgets
ronso0 Apr 25, 2023
f230382
Pref Mixer, xfader: don't store pointer to curve scene
ronso0 Apr 25, 2023
14f6811
Pref Mixer: read config only in slotUpdate() pt I
ronso0 Apr 25, 2023
555b02d
Pref Mixer, bypass EQ: add bool for checkbox state, apply 'only in sl…
ronso0 Apr 25, 2023
9554e84
Pref Mixer, deck EQ & QuickEffects: prevent unneeded reload during init
ronso0 Apr 25, 2023
f731e9d
Pref Mixer: improve function names, remove b prefix from bools, comments
ronso0 Apr 25, 2023
96929a0
Pref Mixer: apply settings only in slotApply(), except Main EQ
ronso0 Apr 27, 2023
770c643
Pref Mixer, main EQ: rework loading, updating & saving
ronso0 Jun 30, 2023
6b248d5
Pref Mixer, main EQ: add checkbox to apply parameter changes instantly
ronso0 Apr 29, 2023
2fdaee3
Pref Mixer, main EQ: tweak parameter slider steps and value labels
ronso0 May 1, 2023
b4c880b
Pref Mixer, EQ shelves: add checkbox to apply shelves instantly
ronso0 Apr 29, 2023
21d5f5e
Pref Mixer, EQ shelves: fix update, improve precision, avoid round tr…
ronso0 May 1, 2023
f6c4435
Pref Mixer, deck EQ & QuickEffects: rework update & save
ronso0 Apr 30, 2023
c4e4fe1
Pref Mixer, deck EQ & QuickEffects: disable non-EQ items if 'EQs only…
ronso0 Jun 24, 2023
f1e1a6a
Pref Mixer: add tooltips for deck & main EQ effects
ronso0 Jun 29, 2023
2f5fd9e
Pref Mixer: fix UX when checking 'Same EQ for all decks'
ronso0 Jun 30, 2023
0d2f732
Pref Mixer: migrate bool options from yes|no to 1|0
ronso0 May 1, 2023
82b3fc0
Pref Mixer: remove obsolete includes, some comments
ronso0 Apr 29, 2023
e82d50b
Pref Mixer: p prefix for widget pointers
ronso0 Jun 30, 2023
d3d18cb
Pref Mixer: use PollingControlProxy for EQ shelve controls
ronso0 Jul 1, 2023
41c4938
Pref Mixer, EQ shelves: instant apply on change, write to config in s…
ronso0 Jul 1, 2023
0a3c57f
Pref Mixer, main EQ: apply instantly, store to config in slotApply()
ronso0 Jul 2, 2023
37bddab
Pref Mixer: fix main EQ parameter precision
ronso0 Jul 3, 2023
742ca3d
Pref Mixer: fix EQ shelves display precision
ronso0 Jul 3, 2023
88a7f37
move EQ corner ConfigKey strings to effects/defs.h, adjust Pref Mixer
ronso0 Jul 6, 2023
21667b6
Pref Mixer: constants for ConfigKeys
ronso0 Jul 6, 2023
2f8534f
Pref Mixer: get default QuickEffect name from its effect manifest
ronso0 Jul 5, 2023
26b952c
use PollingControlProxy for EQ corner frequency controls
ronso0 Jul 5, 2023
c8d98b2
EQ utils: add TODO to update tr string after 2.4 release
ronso0 Jul 5, 2023
89acb4e
Merge remote-tracking branch 'mixxx/2.4' into pref-mixer-apply
ronso0 Jul 12, 2023
0aea69e
Merge remote-tracking branch 'mixxx/2.4' into pref-mixer-apply
ronso0 Aug 12, 2023
579792f
Merge remote-tracking branch 'mixxx/2.4' into pref-mixer-apply
ronso0 Aug 13, 2023
930e42a
Pref Mixer: polish comments, remove obsolete TODOs, add DEBUG_ASESERT
ronso0 Aug 13, 2023
ce925d9
EffectsManager: add getQuickEffectChain() helper
ronso0 Jun 30, 2023
d3ca182
Pref Mixer: use EffectsManager::getQuickEffectChain()
ronso0 Aug 13, 2023
0437f34
Pref Mixer: a few DEBUG_ASSERT instead of VERIFY_OR_...
ronso0 Aug 13, 2023
5298b85
EQ effects tooltip: point to new Mixer page for adjusting the EQ shelves
ronso0 Aug 14, 2023
f2abc89
Merge remote-tracking branch 'mixxx/2.4' into pref-mixer-apply
ronso0 Sep 6, 2023
0ca9701
Pref Mixer: constants for xfader ConfigKeys
ronso0 Sep 7, 2023
c439051
EQ effects: remove empty destructors
ronso0 Sep 13, 2023
e07c3c2
Pref Mixer: use parented_ptr for xfader QGraphicsScene, reuse scene
ronso0 Sep 13, 2023
20bd4fa
Pref Mixer: polish xfader graph, smooth lines, style it like it's 202…
ronso0 Sep 14, 2023
6f20086
Merge remote-tracking branch 'mixxx/2.4' into pref-mixer-apply
ronso0 Sep 18, 2023
17422ed
Pref Mixer: don't use indexOf(QComboBox*) in for loops
ronso0 Sep 19, 2023
5aa6b32
Pref Mixer: use parented_ptr for all widgets created on request
ronso0 Sep 19, 2023
84ddc77
Pref Mixer: contruct combobox tooltips with string args
ronso0 Sep 19, 2023
4c1e867
Pref Mixer: qAsConst -> std::as_const
ronso0 Sep 19, 2023
c222ed4
Pref Mixer: auto* + simpler combobox item deactivation
ronso0 Sep 21, 2023
8e571ee
Pref Mixer: more efficient EQ list splitting EQs / non-EQs
ronso0 Sep 21, 2023
2132b97
Pref Mixer: show effects' display name, separate EQs / non-EQs
ronso0 Sep 21, 2023
268e278
Merge remote-tracking branch 'mixxx/2.4' into pref-mixer-apply
ronso0 Oct 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 6 additions & 10 deletions src/effects/backends/builtin/bessel4lvmixeqeffect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "effects/backends/builtin/bessel4lvmixeqeffect.h"

#include "effects/backends/builtin/equalizer_util.h"
#include "effects/defs.h"
#include "util/math.h"

// static
Expand Down Expand Up @@ -28,9 +29,9 @@ EffectManifestPointer Bessel4LVMixEQEffect::getManifest() {
return pManifest;
}

Bessel4LVMixEQEffect::Bessel4LVMixEQEffect() {
m_pLoFreqCorner = new ControlProxy("[Mixer Profile]", "LoEQFrequency");
m_pHiFreqCorner = new ControlProxy("[Mixer Profile]", "HiEQFrequency");
Bessel4LVMixEQEffect::Bessel4LVMixEQEffect()
: m_pLoFreqCorner(kMixerProfile, kLowEqFrequency),
m_pHiFreqCorner(kMixerProfile, kHighEqFrequency) {
}

void Bessel4LVMixEQEffect::loadEngineEffectParameters(
Expand All @@ -43,11 +44,6 @@ void Bessel4LVMixEQEffect::loadEngineEffectParameters(
m_pKillHigh = parameters.value("killHigh");
}

Bessel4LVMixEQEffect::~Bessel4LVMixEQEffect() {
delete m_pLoFreqCorner;
delete m_pHiFreqCorner;
}

void Bessel4LVMixEQEffect::processChannel(
Bessel4LVMixEQEffectGroupState* pState,
const CSAMPLE* pInput,
Expand Down Expand Up @@ -86,7 +82,7 @@ void Bessel4LVMixEQEffect::processChannel(
fLow,
fMid,
fHigh,
m_pLoFreqCorner->get(),
m_pHiFreqCorner->get());
m_pLoFreqCorner.get(),
m_pHiFreqCorner.get());
}
}
8 changes: 4 additions & 4 deletions src/effects/backends/builtin/bessel4lvmixeqeffect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#pragma once
#include <QMap>

#include "control/controlproxy.h"
#include "control/pollingcontrolproxy.h"
#include "effects/backends/builtin/lvmixeqbase.h"
#include "effects/backends/effectprocessor.h"
#include "engine/effects/engineeffect.h"
Expand All @@ -22,7 +22,7 @@ class Bessel4LVMixEQEffectGroupState : public LVMixEQEffectGroupState<EngineFilt
class Bessel4LVMixEQEffect : public EffectProcessorImpl<Bessel4LVMixEQEffectGroupState> {
public:
Bessel4LVMixEQEffect();
~Bessel4LVMixEQEffect() override;
~Bessel4LVMixEQEffect() override = default;

static QString getId();
static EffectManifestPointer getManifest();
Expand Down Expand Up @@ -51,8 +51,8 @@ class Bessel4LVMixEQEffect : public EffectProcessorImpl<Bessel4LVMixEQEffectGrou
EngineEffectParameterPointer m_pKillMid;
EngineEffectParameterPointer m_pKillHigh;

ControlProxy* m_pLoFreqCorner;
ControlProxy* m_pHiFreqCorner;
PollingControlProxy m_pLoFreqCorner;
PollingControlProxy m_pHiFreqCorner;

DISALLOW_COPY_AND_ASSIGN(Bessel4LVMixEQEffect);
};
16 changes: 6 additions & 10 deletions src/effects/backends/builtin/bessel8lvmixeqeffect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "effects/backends/builtin/bessel8lvmixeqeffect.h"

#include "effects/backends/builtin/equalizer_util.h"
#include "effects/defs.h"
#include "util/math.h"

// static
Expand Down Expand Up @@ -28,9 +29,9 @@ EffectManifestPointer Bessel8LVMixEQEffect::getManifest() {
return pManifest;
}

Bessel8LVMixEQEffect::Bessel8LVMixEQEffect() {
m_pLoFreqCorner = new ControlProxy("[Mixer Profile]", "LoEQFrequency");
m_pHiFreqCorner = new ControlProxy("[Mixer Profile]", "HiEQFrequency");
Bessel8LVMixEQEffect::Bessel8LVMixEQEffect()
: m_pLoFreqCorner(kMixerProfile, kLowEqFrequency),
m_pHiFreqCorner(kMixerProfile, kHighEqFrequency) {
}

void Bessel8LVMixEQEffect::loadEngineEffectParameters(
Expand All @@ -43,11 +44,6 @@ void Bessel8LVMixEQEffect::loadEngineEffectParameters(
m_pKillHigh = parameters.value("killHigh");
}

Bessel8LVMixEQEffect::~Bessel8LVMixEQEffect() {
delete m_pLoFreqCorner;
delete m_pHiFreqCorner;
}

void Bessel8LVMixEQEffect::processChannel(
Bessel8LVMixEQEffectGroupState* pState,
const CSAMPLE* pInput,
Expand Down Expand Up @@ -86,7 +82,7 @@ void Bessel8LVMixEQEffect::processChannel(
fLow,
fMid,
fHigh,
m_pLoFreqCorner->get(),
m_pHiFreqCorner->get());
m_pLoFreqCorner.get(),
m_pHiFreqCorner.get());
}
}
8 changes: 4 additions & 4 deletions src/effects/backends/builtin/bessel8lvmixeqeffect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

#include <QMap>

#include "control/controlproxy.h"
#include "control/pollingcontrolproxy.h"
#include "effects/backends/builtin/lvmixeqbase.h"
#include "effects/backends/effectprocessor.h"
#include "engine/effects/engineeffect.h"
Expand All @@ -24,7 +24,7 @@ class Bessel8LVMixEQEffectGroupState : public LVMixEQEffectGroupState<EngineFilt
class Bessel8LVMixEQEffect : public EffectProcessorImpl<Bessel8LVMixEQEffectGroupState> {
public:
Bessel8LVMixEQEffect();
~Bessel8LVMixEQEffect() override;
~Bessel8LVMixEQEffect() override = default;

static QString getId();
static EffectManifestPointer getManifest();
Expand Down Expand Up @@ -53,8 +53,8 @@ class Bessel8LVMixEQEffect : public EffectProcessorImpl<Bessel8LVMixEQEffectGrou
EngineEffectParameterPointer m_pKillMid;
EngineEffectParameterPointer m_pKillHigh;

ControlProxy* m_pLoFreqCorner;
ControlProxy* m_pHiFreqCorner;
PollingControlProxy m_pLoFreqCorner;
PollingControlProxy m_pHiFreqCorner;

DISALLOW_COPY_AND_ASSIGN(Bessel8LVMixEQEffect);
};
19 changes: 10 additions & 9 deletions src/effects/backends/builtin/biquadfullkilleqeffect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "effects/backends/builtin/biquadfullkilleqeffect.h"

#include "effects/backends/builtin/equalizer_util.h"
#include "effects/defs.h"
#include "util/math.h"

namespace {
Expand Down Expand Up @@ -142,9 +143,9 @@ void BiquadFullKillEQEffectGroupState::setFilters(
m_lvMixIso->setFilters(sampleRate, lowFreqCorner, highFreqCorner);
}

BiquadFullKillEQEffect::BiquadFullKillEQEffect() {
m_pLoFreqCorner = std::make_unique<ControlProxy>("[Mixer Profile]", "LoEQFrequency");
m_pHiFreqCorner = std::make_unique<ControlProxy>("[Mixer Profile]", "HiEQFrequency");
BiquadFullKillEQEffect::BiquadFullKillEQEffect()
: m_pLoFreqCorner(kMixerProfile, kLowEqFrequency),
m_pHiFreqCorner(kMixerProfile, kHighEqFrequency) {
}

void BiquadFullKillEQEffect::loadEngineEffectParameters(
Expand All @@ -170,10 +171,10 @@ void BiquadFullKillEQEffect::processChannel(
Q_UNUSED(groupFeatures);

if (pState->m_oldSampleRate != engineParameters.sampleRate() ||
(pState->m_loFreqCorner != m_pLoFreqCorner->get()) ||
(pState->m_highFreqCorner != m_pHiFreqCorner->get())) {
pState->m_loFreqCorner = m_pLoFreqCorner->get();
pState->m_highFreqCorner = m_pHiFreqCorner->get();
(pState->m_loFreqCorner != m_pLoFreqCorner.get()) ||
(pState->m_highFreqCorner != m_pHiFreqCorner.get())) {
pState->m_loFreqCorner = m_pLoFreqCorner.get();
pState->m_highFreqCorner = m_pHiFreqCorner.get();
pState->m_oldSampleRate = engineParameters.sampleRate();
pState->setFilters(engineParameters.sampleRate(),
pState->m_loFreqCorner,
Expand Down Expand Up @@ -407,7 +408,7 @@ void BiquadFullKillEQEffect::processChannel(
fLow,
fMid,
fHigh,
m_pLoFreqCorner->get(),
m_pHiFreqCorner->get());
m_pLoFreqCorner.get(),
m_pHiFreqCorner.get());
}
}
6 changes: 3 additions & 3 deletions src/effects/backends/builtin/biquadfullkilleqeffect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#pragma once

#include "control/controlproxy.h"
#include "control/pollingcontrolproxy.h"
#include "effects/backends/builtin/lvmixeqbase.h"
#include "effects/backends/effectprocessor.h"
#include "engine/effects/engineeffect.h"
Expand Down Expand Up @@ -96,6 +96,6 @@ class BiquadFullKillEQEffect : public EffectProcessorImpl<BiquadFullKillEQEffect
EngineEffectParameterPointer m_pKillMid;
EngineEffectParameterPointer m_pKillHigh;

std::unique_ptr<ControlProxy> m_pLoFreqCorner;
std::unique_ptr<ControlProxy> m_pHiFreqCorner;
PollingControlProxy m_pLoFreqCorner;
PollingControlProxy m_pHiFreqCorner;
};
2 changes: 1 addition & 1 deletion src/effects/backends/builtin/equalizer_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,6 @@ class EqualizerUtil {

static QString adjustFrequencyShelvesTip() {
return QObject::tr(
"To adjust frequency shelves, go to Preferences -> Equalizers.");
"To adjust frequency shelves, go to Preferences -> Mixer.");
}
};
17 changes: 8 additions & 9 deletions src/effects/backends/builtin/linkwitzriley8eqeffect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "effects/backends/builtin/linkwitzriley8eqeffect.h"

#include "effects/backends/builtin/equalizer_util.h"
#include "effects/defs.h"
#include "util/math.h"

static constexpr unsigned int kStartupSamplerate = 44100;
Expand Down Expand Up @@ -67,9 +68,9 @@ void LinkwitzRiley8EQEffectGroupState::setFilters(int sampleRate, int lowFreq, i
m_high2->setFrequencyCorners(sampleRate, highFreq);
}

LinkwitzRiley8EQEffect::LinkwitzRiley8EQEffect() {
m_pLoFreqCorner = new ControlProxy("[Mixer Profile]", "LoEQFrequency");
m_pHiFreqCorner = new ControlProxy("[Mixer Profile]", "HiEQFrequency");
LinkwitzRiley8EQEffect::LinkwitzRiley8EQEffect()
: m_pLoFreqCorner(kMixerProfile, kLowEqFrequency),
m_pHiFreqCorner(kMixerProfile, kHighEqFrequency) {
}

void LinkwitzRiley8EQEffect::loadEngineEffectParameters(
Expand All @@ -83,8 +84,6 @@ void LinkwitzRiley8EQEffect::loadEngineEffectParameters(
}

LinkwitzRiley8EQEffect::~LinkwitzRiley8EQEffect() {
delete m_pLoFreqCorner;
delete m_pHiFreqCorner;
}

void LinkwitzRiley8EQEffect::processChannel(
Expand All @@ -108,10 +107,10 @@ void LinkwitzRiley8EQEffect::processChannel(
}

if (pState->m_oldSampleRate != engineParameters.sampleRate() ||
(pState->m_loFreq != static_cast<int>(m_pLoFreqCorner->get())) ||
(pState->m_hiFreq != static_cast<int>(m_pHiFreqCorner->get()))) {
pState->m_loFreq = static_cast<int>(m_pLoFreqCorner->get());
pState->m_hiFreq = static_cast<int>(m_pHiFreqCorner->get());
(pState->m_loFreq != static_cast<int>(m_pLoFreqCorner.get())) ||
(pState->m_hiFreq != static_cast<int>(m_pHiFreqCorner.get()))) {
pState->m_loFreq = static_cast<int>(m_pLoFreqCorner.get());
pState->m_hiFreq = static_cast<int>(m_pHiFreqCorner.get());
pState->m_oldSampleRate = engineParameters.sampleRate();
pState->setFilters(engineParameters.sampleRate(), pState->m_loFreq, pState->m_hiFreq);
}
Expand Down
6 changes: 3 additions & 3 deletions src/effects/backends/builtin/linkwitzriley8eqeffect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

#include <QMap>

#include "control/controlproxy.h"
#include "control/pollingcontrolproxy.h"
#include "effects/backends/effectprocessor.h"
#include "engine/effects/engineeffect.h"
#include "engine/effects/engineeffectparameter.h"
Expand Down Expand Up @@ -69,8 +69,8 @@ class LinkwitzRiley8EQEffect : public EffectProcessorImpl<LinkwitzRiley8EQEffect
EngineEffectParameterPointer m_pKillMid;
EngineEffectParameterPointer m_pKillHigh;

ControlProxy* m_pLoFreqCorner;
ControlProxy* m_pHiFreqCorner;
PollingControlProxy m_pLoFreqCorner;
PollingControlProxy m_pHiFreqCorner;

DISALLOW_COPY_AND_ASSIGN(LinkwitzRiley8EQEffect);
};
15 changes: 8 additions & 7 deletions src/effects/backends/builtin/threebandbiquadeqeffect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "effects/backends/builtin/threebandbiquadeqeffect.h"

#include "effects/backends/builtin/equalizer_util.h"
#include "effects/defs.h"
#include "util/math.h"

namespace {
Expand Down Expand Up @@ -116,9 +117,9 @@ void ThreeBandBiquadEQEffectGroupState::setFilters(
sampleRate, highCenter / 2, kQKillShelve, m_oldHighCut);
}

ThreeBandBiquadEQEffect::ThreeBandBiquadEQEffect() {
m_pLoFreqCorner = std::make_unique<ControlProxy>("[Mixer Profile]", "LoEQFrequency");
m_pHiFreqCorner = std::make_unique<ControlProxy>("[Mixer Profile]", "HiEQFrequency");
ThreeBandBiquadEQEffect::ThreeBandBiquadEQEffect()
: m_pLoFreqCorner(kMixerProfile, kLowEqFrequency),
m_pHiFreqCorner(kMixerProfile, kHighEqFrequency) {
}

void ThreeBandBiquadEQEffect::loadEngineEffectParameters(
Expand All @@ -141,10 +142,10 @@ void ThreeBandBiquadEQEffect::processChannel(
Q_UNUSED(groupFeatures);

if (pState->m_oldSampleRate != engineParameters.sampleRate() ||
(pState->m_loFreqCorner != m_pLoFreqCorner->get()) ||
(pState->m_highFreqCorner != m_pHiFreqCorner->get())) {
pState->m_loFreqCorner = m_pLoFreqCorner->get();
pState->m_highFreqCorner = m_pHiFreqCorner->get();
(pState->m_loFreqCorner != m_pLoFreqCorner.get()) ||
(pState->m_highFreqCorner != m_pHiFreqCorner.get())) {
pState->m_loFreqCorner = m_pLoFreqCorner.get();
pState->m_highFreqCorner = m_pHiFreqCorner.get();
pState->m_oldSampleRate = engineParameters.sampleRate();
pState->setFilters(engineParameters.sampleRate(),
pState->m_loFreqCorner,
Expand Down
6 changes: 3 additions & 3 deletions src/effects/backends/builtin/threebandbiquadeqeffect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#pragma once

#include "control/controlproxy.h"
#include "control/pollingcontrolproxy.h"
#include "effects/backends/effectprocessor.h"
#include "engine/effects/engineeffect.h"
#include "engine/effects/engineeffectparameter.h"
Expand Down Expand Up @@ -77,6 +77,6 @@ class ThreeBandBiquadEQEffect : public EffectProcessorImpl<ThreeBandBiquadEQEffe
EngineEffectParameterPointer m_pKillMid;
EngineEffectParameterPointer m_pKillHigh;

std::unique_ptr<ControlProxy> m_pLoFreqCorner;
std::unique_ptr<ControlProxy> m_pHiFreqCorner;
PollingControlProxy m_pLoFreqCorner;
PollingControlProxy m_pHiFreqCorner;
};
4 changes: 4 additions & 0 deletions src/effects/defs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ constexpr int kNumEffectsPerUnit = 4;

const QString kNoEffectString = QStringLiteral("---");

const QString kMixerProfile = QStringLiteral("[Mixer Profile]");
const QString kHighEqFrequency = QStringLiteral("HiEQFrequency");
const QString kLowEqFrequency = QStringLiteral("LoEQFrequency");
Copy link
Member

Choose a reason for hiding this comment

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

The current solution is perfectly OK, but has a performance implications.

Putting a const QString into a header (internal namespace) will construct (malloc) the QString again in every cpp file that includes the header.
The memory allocation "may" take place when the QString is used the first time.

This is in our case when moved to ConfigObject

mixxx/src/preferences/configobject.h

Line 20 in dfde28d

ConfigKey(QString group, QString item)

It would be possible to share the strings across the translation units by moving this to the global (external) namespace by adding this to a defs.cpp as
extern const QString kMixerProfile = QStringLiteral("[Mixer Profile]");

and make this:
extern const QString kMixerProfile;

This way the memory allocation is only done a single time and than shared in whole mixxx binary

Maybe we can sourround it with a defs namespace or such.

We have more of these shared const QString in Mixxx, so I am not sure if it worth to target the issue here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, thanks for the info. So, like in library prefs.h/cpp (where we could enclose the defs in namespaces mixxx, library & prefs to have const ConfigKey kSyncTrackMetadataConfigKey instead of
const ConfigKey mixxx::library::prefs::kSyncTrackMetadataConfigKey)

Out of curiosity, how does this compare to #define like in src/defs_urls.h?

I wouldn't do it for these 3 strings here, and I didn't find noteworthy occurences of more than one const QString k* in header files. File a reminder if you know of more places where this could be improved?

Copy link
Member Author

@ronso0 ronso0 Sep 10, 2023 •
edited

Choose a reason for hiding this comment

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

I tried
defs.h

extern const QString kEmptyEffect;

new defs.ccp

const QString kEmptyEffect = QStringLiteral("---");

but get linker errors.

When omitting extern it builds and works well. edit: no, it doesn't work..

@daschuer I have no clue how to proceed, could you give some advice?

Copy link
Member

@Swiftb0y Swiftb0y Sep 13, 2023 •
edited

Choose a reason for hiding this comment

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

recreating the same thing in compiler explorer works with std::string (CE's Qt + Cmake integration is broken unfortunately so I can't easily investigate why this breaks for QStrings).

Copy link
Member

Choose a reason for hiding this comment

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

Putting a const QString into a header (internal namespace) will construct (malloc) the QString again in every cpp file that includes the header.

I thought the entire point of the QStringLiteral macro was that there was no allocation taking place?!? Yes, there is allocation when the string gets modified due to CoW, but thats already the place. I don't how putting this in the header would result in performance degradation. And even if, the number of allocations which would create would be tiny compared the amount of unnecessary allocations we have everywhere else.

Copy link
Member

Choose a reason for hiding this comment

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

I thought the entire point of the QStringLiteral macro was that there was no allocation taking place?!?

Oh right, my bad. We have no heap allocation here.

The uinicode string along with 20 bytes of control data is used directly from the text segment and of every compilation unit that includes and uses the string. This is almost like a constexpr with reference counting disabled.
An addition there is a pointer to that data in the bss initalized with that location.

So there is no performance benefit, but a negligible memory impact.

Sorry for he noise.

Copy link
Member

Choose a reason for hiding this comment

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

Right, moreover, what about initialization order? The initialization order of TU's is undefined. If you declare something as extern const, you need to take care that no other global variable is being initialized based on it. This reeks of a potential source of bugs. And even if the string is QStringLiteral, the constructor for that variable still needs to run in order for the variable to be usable, even if the constructor doesn't do much, right?

Copy link
Member

@daschuer daschuer Sep 19, 2023 •
edited

Choose a reason for hiding this comment

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

If you declare something as extern const, you need to take care that no other global variable is being initialized based on it.

Yes. In this case it is no issue, because the constructor boils down to a pointer initializer. No other dependencys.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ronso0 My guess is it does not work, because defs.h is not included in defs.cpp.

Aaahm, nope. I also tried namespace(s) to no avail.
The pattern you suggest works well with ConfigKey (like in library_prefs.h/cpp) but appearantly not with QString.
Btw I stopped investigating what's wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's move on, not worth to discuss it in the scope of an unrelated PR.


// NOTE: Setting this to true will enable string manipulation and calls to
// qDebug() in the audio engine thread. That may cause audio dropouts, so only
// enable this when debugging the effects system.
Expand Down
4 changes: 2 additions & 2 deletions src/effects/effectsmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ EffectsManager::EffectsManager(
std::shared_ptr<ChannelHandleFactory> pChannelHandleFactory)
: m_pConfig(pConfig),
m_pChannelHandleFactory(pChannelHandleFactory),
m_loEqFreq(ConfigKey("[Mixer Profile]", "LoEQFrequency"), 0., 22040),
m_hiEqFreq(ConfigKey("[Mixer Profile]", "HiEQFrequency"), 0., 22040) {
m_loEqFreq(ConfigKey(kMixerProfile, kLowEqFrequency), 0., 22040),
m_hiEqFreq(ConfigKey(kMixerProfile, kHighEqFrequency), 0., 22040) {
qRegisterMetaType<EffectChainMixMode>("EffectChainMixMode");

m_pBackendManager = EffectsBackendManagerPointer(new EffectsBackendManager());
Expand Down
4 changes: 4 additions & 0 deletions src/effects/effectsmanager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ class EffectsManager {
const QString& deckGroupName) const {
return m_equalizerEffectChains.value(deckGroupName);
}
QuickEffectChainPointer getQuickEffectChain(
const QString& deckGroupName) const {
return m_quickEffectChains.value(deckGroupName);
}
EffectChainPointer getStandardEffectChain(int unitNumber) const;
EffectChainPointer getOutputEffectChain() const;

Expand Down