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.

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

Optimize control code #13354

Merged
merged 13 commits into from
Aug 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1333,6 +1333,7 @@ set(MIXXX_LIB_PRECOMPILED_HEADER
src/control/control.h
src/control/controlaudiotaperpot.h
src/control/controlbehavior.h
src/control/controlbuttonmode.h
src/control/controlcompressingproxy.h
src/control/controleffectknob.h
src/control/controlencoder.h
Expand Down
2 changes: 1 addition & 1 deletion src/broadcast/broadcastmanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ BroadcastManager::BroadcastManager(SettingsManager* pSettingsManager,
const bool persist = true;
m_pBroadcastEnabled = new ControlPushButton(
ConfigKey(BROADCAST_PREF_KEY,"enabled"), persist);
m_pBroadcastEnabled->setButtonMode(ControlPushButton::TOGGLE);
m_pBroadcastEnabled->setButtonMode(mixxx::control::ButtonMode::Toggle);
connect(m_pBroadcastEnabled,
&ControlPushButton::valueChanged,
this,
Expand Down
14 changes: 7 additions & 7 deletions src/control/control.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ QWeakPointer<ControlDoublePrivate> s_pDefaultCO;
} // namespace

ControlDoublePrivate::ControlDoublePrivate()
: m_bPersistInConfiguration(false),
m_bIgnoreNops(true),
m_bTrack(false),
m_trackType(Stat::UNSPECIFIED),
: m_trackType(Stat::UNSPECIFIED),
m_trackFlags(Stat::COUNT | Stat::SUM | Stat::AVERAGE |
Stat::SAMPLE_VARIANCE | Stat::MIN | Stat::MAX),
m_bTrack(false),
// default CO is read only
m_confirmRequired(true),
m_bPersistInConfiguration(false),
m_bIgnoreNops(true),
m_kbdRepeatable(false) {
m_value.setValue(0.0);
}
Expand All @@ -51,13 +51,13 @@ ControlDoublePrivate::ControlDoublePrivate(
double defaultValue)
: m_key(key),
m_pCreatorCO(pCreatorCO),
m_bPersistInConfiguration(bPersist),
m_bIgnoreNops(bIgnoreNops),
m_bTrack(bTrack),
m_trackType(Stat::UNSPECIFIED),
m_trackFlags(Stat::COUNT | Stat::SUM | Stat::AVERAGE |
Stat::SAMPLE_VARIANCE | Stat::MIN | Stat::MAX),
m_bTrack(bTrack),
m_confirmRequired(false),
m_bPersistInConfiguration(bPersist),
m_bIgnoreNops(bIgnoreNops),
m_kbdRepeatable(false) {
initialize(defaultValue);
}
Expand Down
41 changes: 23 additions & 18 deletions src/control/control.h
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,31 @@ class ControlDoublePrivate : public QObject {

const ConfigKey m_key;

QSharedPointer<ControlNumericBehavior> m_pBehavior;

// User-visible, i18n name for what the control is.
QString m_name;

// User-visible, i18n description for what the control does.
QString m_description;

// The control value.
ControlValueAtomic<double> m_value;
// The default control value.
ControlValueAtomic<double> m_defaultValue;
Copy link
Member

Choose a reason for hiding this comment

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

How do we protect that for get cluttered again? Maybe define regions via comments?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@JoergAtGithub JoergAtGithub Jun 30, 2024

Choose a reason for hiding this comment

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

I tried -Wpadded on CI, but it does not only warn about padding inside of the data structure, but also if the overall size is not matching the byte alignment size, e.g. any class with 6 bytes would need to filled up with 2 explicit padding bytes.

I found also a currently unused macro in

#define M_PACK(statement) __pragma( pack(push, 1) ) statement __pragma( pack(pop) )

Maybe someone on Linux could try: https://linux.die.net/man/1/pahole

Copy link
Member

@Swiftb0y Swiftb0y Jun 30, 2024

Choose a reason for hiding this comment

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

Okay that makes sense I guess when the compiler expects you to nest these packed data structures. The only other potential solution I found was to generate two versions of the type, one packed (potentially with suboptimal alignment and then static_assert(sizeof(T) == sizeof(_T_packed)) (potentially with a bit of rounding to manually account for the overall size). I wouldn't know how to accomplish that without copy pasting or a macro (doing the copy-pasting automatically) though.

Copy link
Member

Choose a reason for hiding this comment

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

Let's not over engineer it. Just a comment state informs there reader that the oder of the variables has been chosen to avoid memory gaps due to alignment is probably sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's already added:

mixxx/src/control/control.h

Lines 203 to 205 in 6728c91

// Note: keep the order of the members below to not introduce gaps due to
// memory alignment in this often used class. Whether to track value changes
// with the stats framework.


QAtomicPointer<ControlObject> m_pCreatorCO;

QString m_trackKey;

JoergAtGithub marked this conversation as resolved.
Show resolved Hide resolved
// Note: keep the order of the members below to not introduce gaps due to
// memory alignment in this often used class. Whether to track value changes
// with the stats framework.
int m_trackType;
int m_trackFlags;
bool m_bTrack;
bool m_confirmRequired;

// Whether the control should persist in the Mixxx user configuration. The
// value is loaded from configuration when the control is created and
// written to the configuration when the control is deleted.
Expand All @@ -193,28 +216,10 @@ class ControlDoublePrivate : public QObject {
// Whether to ignore sets which would have no effect.
bool m_bIgnoreNops;

// Whether to track value changes with the stats framework.
bool m_bTrack;
QString m_trackKey;
int m_trackType;
int m_trackFlags;
bool m_confirmRequired;

// User-visible, i18n name for what the control is.
QString m_name;

// User-visible, i18n description for what the control does.
QString m_description;

// If true, this control will be issued repeatedly if the keyboard key is held.
bool m_kbdRepeatable;

// The control value.
ControlValueAtomic<double> m_value;
// The default control value.
ControlValueAtomic<double> m_defaultValue;

QSharedPointer<ControlNumericBehavior> m_pBehavior;
};

/// The constant ControlDoublePrivate version is used as dummy for default
Expand Down
13 changes: 7 additions & 6 deletions src/control/controlbehavior.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -342,8 +342,8 @@ double ControlLinSteppedIntPotBehavior::parameterToValue(double dParam) {
const int ControlPushButtonBehavior::kPowerWindowTimeMillis = 300;
const int ControlPushButtonBehavior::kLongPressLatchingTimeMillis = 300;

ControlPushButtonBehavior::ControlPushButtonBehavior(ButtonMode buttonMode,
int iNumStates)
ControlPushButtonBehavior::ControlPushButtonBehavior(mixxx::control::ButtonMode buttonMode,
int iNumStates)
: m_buttonMode(buttonMode),
m_iNumStates(iNumStates) {
}
Expand All @@ -364,7 +364,7 @@ void ControlPushButtonBehavior::setValueFromMidi(
}

// This block makes push-buttons act as power window buttons.
if (m_buttonMode == POWERWINDOW && m_iNumStates == 2) {
if (m_buttonMode == mixxx::control::ButtonMode::PowerWindow && m_iNumStates == 2) {
auto* timer = getTimer();
if (pressed) {
// Toggle on press
Expand All @@ -376,7 +376,8 @@ void ControlPushButtonBehavior::setValueFromMidi(
// Disable after releasing a long press
pControl->set(0., nullptr);
}
} else if (m_buttonMode == TOGGLE || m_buttonMode == LONGPRESSLATCHING) {
} else if (m_buttonMode == mixxx::control::ButtonMode::Toggle ||
m_buttonMode == mixxx::control::ButtonMode::LongPressLatching) {
// This block makes push-buttons act as toggle buttons.
if (m_iNumStates > 1) { // multistate button
if (pressed) {
Expand All @@ -387,14 +388,14 @@ void ControlPushButtonBehavior::setValueFromMidi(
double value = pControl->get();
value = (int)(value + 1.) % m_iNumStates;
pControl->set(value, nullptr);
if (m_buttonMode == LONGPRESSLATCHING) {
if (m_buttonMode == mixxx::control::ButtonMode::LongPressLatching) {
auto* timer = getTimer();
timer->setSingleShot(true);
timer->start(kLongPressLatchingTimeMillis);
}
} else {
double value = pControl->get();
if (m_buttonMode == LONGPRESSLATCHING &&
if (m_buttonMode == mixxx::control::ButtonMode::LongPressLatching &&
getTimer()->isActive() && value >= 1.) {
// revert toggle if button is released too early
value = (int)(value - 1.) % m_iNumStates;
Expand Down
17 changes: 4 additions & 13 deletions src/control/controlbehavior.h
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
#pragma once

#include <QTimer>
#include <QScopedPointer>
#include <QTimer>

#include "control/controlbuttonmode.h"
#include "controllers/midi/midimessage.h"

class ControlDoublePrivate;
Expand Down Expand Up @@ -130,17 +131,7 @@ class ControlPushButtonBehavior : public ControlNumericBehavior {
static const int kPowerWindowTimeMillis;
static const int kLongPressLatchingTimeMillis;

// TODO(XXX) Duplicated from ControlPushButton. It's complicated and
// annoying to share them so I just copied them.
enum ButtonMode {
PUSH = 0,
TOGGLE,
POWERWINDOW,
LONGPRESSLATCHING,
TRIGGER
};

ControlPushButtonBehavior(ButtonMode buttonMode, int iNumStates);
ControlPushButtonBehavior(mixxx::control::ButtonMode buttonMode, int iNumStates);
void setValueFromMidi(
MidiOpCode o, double dParam, ControlDoublePrivate* pControl)
override;
Expand All @@ -154,7 +145,7 @@ class ControlPushButtonBehavior : public ControlNumericBehavior {
}
return m_pushTimer.data();
}
ButtonMode m_buttonMode;
mixxx::control::ButtonMode m_buttonMode;
int m_iNumStates;
QScopedPointer<QTimer> m_pushTimer;
};
Expand Down
23 changes: 23 additions & 0 deletions src/control/controlbuttonmode.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#pragma once

// required for Qt-Macros
#include <qobjectdefs.h>

namespace mixxx {

namespace control {

Q_NAMESPACE

enum class ButtonMode {
Push,
Toggle,
PowerWindow,
LongPressLatching,
Trigger
};

Q_ENUM_NS(ButtonMode);

} // namespace control
} // namespace mixxx
3 changes: 3 additions & 0 deletions src/control/controlmodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ ControlModel::ControlModel(QObject* pParent)

QSet<ConfigKey> controlKeys;

// Reserve memory for m_controls, which will be used later in addControl
m_controls.reserve(controlsList.size());

for (const QSharedPointer<ControlDoublePrivate>& pControl : controlsList) {
if (!pControl) {
continue;
Expand Down
57 changes: 34 additions & 23 deletions src/control/controlpushbutton.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,39 +8,50 @@
-------- ------------------------------------------------------ */
ControlPushButton::ControlPushButton(const ConfigKey& key, bool bPersist, double defaultValue)
: ControlObject(key, false, false, bPersist, defaultValue),
m_buttonMode(PUSH),
m_buttonMode(mixxx::control::ButtonMode::Push),
m_iNoStates(2) {
if (m_pControl) {
m_pControl->setBehavior(
new ControlPushButtonBehavior(
static_cast<ControlPushButtonBehavior::ButtonMode>(m_buttonMode),
m_iNoStates));
updateBehavior();
}

ControlPushButton::~ControlPushButton() = default;

void ControlPushButton::setButtonMode(mixxx::control::ButtonMode mode) {
if (m_buttonMode != mode) {
m_buttonMode = mode;
updateBehavior();
}
}

ControlPushButton::~ControlPushButton() {
void ControlPushButton::setStates(int num_states) {
if (m_iNoStates != num_states) {
m_iNoStates = num_states;
updateBehavior();
}
}

// Tell this PushButton how to act on rising and falling edges
void ControlPushButton::setButtonMode(enum ButtonMode mode) {
//qDebug() << "Setting " << m_Key.group << m_Key.item << "as toggle";
m_buttonMode = mode;
void ControlPushButton::setBehavior(mixxx::control::ButtonMode mode, int num_states) {
bool shouldUpdate = false;
if (m_buttonMode != mode) {
m_buttonMode = mode;
shouldUpdate = true;
}
if (m_iNoStates != num_states) {
m_iNoStates = num_states;
shouldUpdate = true;
}

// If we would update unconditional, the state would be set always to the default value
if (shouldUpdate) {
updateBehavior();
}
}

// private
void ControlPushButton::updateBehavior() {
if (m_pControl) {
m_pControl->setBehavior(
new ControlPushButtonBehavior(
static_cast<ControlPushButtonBehavior::ButtonMode>(m_buttonMode),
m_buttonMode,
m_iNoStates));
}
}

void ControlPushButton::setStates(int num_states) {
m_iNoStates = num_states;

if (m_pControl) {
m_pControl->setBehavior(
new ControlPushButtonBehavior(
static_cast<ControlPushButtonBehavior::ButtonMode>(m_buttonMode),
m_iNoStates));
}
}
71 changes: 21 additions & 50 deletions src/control/controlpushbutton.h
Original file line number Diff line number Diff line change
@@ -1,69 +1,40 @@
/***************************************************************************
controlpushbutton.h - description
-------------------
begin : Wed Feb 20 2002
copyright : (C) 2002 by Tue and Ken Haste Andersen
email :
***************************************************************************/

/***************************************************************************
* *
* This program is free software; you can redistribute it and/or modify *
* it under the terms of the GNU General Public License as published by *
* the Free Software Foundation; either version 2 of the License, or *
* (at your option) any later version. *
* *
***************************************************************************/

#ifndef CONTROLPUSHBUTTON_H
#define CONTROLPUSHBUTTON_H
#pragma once

#include "control/controlbuttonmode.h"
#include "control/controlobject.h"

/**
*@author Tue and Ken Haste Andersen
*/

class ControlPushButton : public ControlObject {
Q_OBJECT
public:
enum ButtonMode {
PUSH = 0,
TOGGLE,
POWERWINDOW,
LONGPRESSLATCHING,
TRIGGER,
};

static QString buttonModeToString(int mode) {
switch(mode) {
case ControlPushButton::PUSH:
return "PUSH";
case ControlPushButton::TOGGLE:
return "TOGGLE";
case ControlPushButton::POWERWINDOW:
return "POWERWINDOW";
case ControlPushButton::LONGPRESSLATCHING:
return "LONGPRESSLATCHING";
case ControlPushButton::TRIGGER:
return "TRIGGER";
default:
return "UNKNOWN";
static QString buttonModeToString(mixxx::control::ButtonMode buttonMode) {
switch (buttonMode) {
case mixxx::control::ButtonMode::Push:
return QStringLiteral("Push");
case mixxx::control::ButtonMode::Toggle:
return QStringLiteral("Toggle");
case mixxx::control::ButtonMode::PowerWindow:
return QStringLiteral("PowerWindow");
case mixxx::control::ButtonMode::LongPressLatching:
return QStringLiteral("LongPressLatching");
case mixxx::control::ButtonMode::Trigger:
return QStringLiteral("Trigger");
}
DEBUG_ASSERT(false);
return "Unknown";
}

ControlPushButton(const ConfigKey& key, bool bPersist = false, double defaultValue = 0.0);
virtual ~ControlPushButton();

inline ButtonMode getButtonMode() const {
inline mixxx::control::ButtonMode getButtonMode() const {
return m_buttonMode;
}
void setButtonMode(enum ButtonMode mode);
void setButtonMode(mixxx::control::ButtonMode mode);
void setStates(int num_states);
void setBehavior(mixxx::control::ButtonMode mode, int num_states);

private:
enum ButtonMode m_buttonMode;
void updateBehavior();
enum mixxx::control::ButtonMode m_buttonMode;
int m_iNoStates;
};

#endif
Loading
Loading