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

This descibes possible causes for a rare crasher with effects. #4487

Closed
wants to merge 14 commits into from
Closed
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
123 changes: 27 additions & 96 deletions src/effects/backends/effectprocessor.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,12 @@ class EffectProcessor {
const QSet<ChannelHandleAndGroup>& activeInputChannels,
const QSet<ChannelHandleAndGroup>& registeredOutputChannels,
const mixxx::EngineParameters& engineParameters) = 0;
virtual void initializeInputChannel(
ChannelHandle inputChannel,
const mixxx::EngineParameters& engineParameters) = 0;
virtual void loadEngineEffectParameters(
const QMap<QString, EngineEffectParameterPointer>& parameters) = 0;
virtual EffectState* createState(const mixxx::EngineParameters& engineParameters) = 0;
virtual void deleteStatesForInputChannel(ChannelHandle inputChannel) = 0;

// Called from the audio thread
virtual bool loadStatesForInputChannel(ChannelHandle inputChannel,
const EffectStatesMap* pStatesMap) = 0;
virtual bool hasStatesForInputChannel(ChannelHandle inputChannel) const = 0;

/// Called from the audio thread
/// This method takes a buffer of audio samples as pInput, processes the buffer
Expand Down Expand Up @@ -178,110 +176,43 @@ class EffectProcessorImpl : public EffectProcessor {
m_registeredOutputChannels = registeredOutputChannels;

for (const ChannelHandleAndGroup& inputChannel : activeInputChannels) {
if (kEffectDebugOutput) {
qDebug() << this << "EffectProcessorImpl::initialize allocating "
"EffectStates for input"
<< inputChannel;
}
ChannelHandleMap<EffectSpecificState*> outputChannelMap;
for (const ChannelHandleAndGroup& outputChannel :
std::as_const(m_registeredOutputChannels)) {
outputChannelMap.insert(outputChannel.handle(),
createSpecificState(engineParameters));
if (kEffectDebugOutput) {
qDebug() << this << "EffectProcessorImpl::initialize "
"registering output"
<< outputChannel << outputChannelMap[outputChannel.handle()];
}
}
m_channelStateMatrix.insert(inputChannel.handle(), outputChannelMap);
initializeInputChannel(inputChannel.handle(), engineParameters);
}
};

EffectState* createState(const mixxx::EngineParameters& engineParameters) final {
return createSpecificState(engineParameters);
};

bool loadStatesForInputChannel(ChannelHandle inputChannel,
const EffectStatesMap* pStatesMap) final {
void initializeInputChannel(ChannelHandle inputChannel,
const mixxx::EngineParameters& engineParameters) final {
if (kEffectDebugOutput) {
qDebug() << "EffectProcessorImpl::loadStatesForInputChannel" << this
<< "input" << inputChannel;
}

// NOTE: ChannelHandleMap is like a map in that it associates an
// object with a ChannelHandle key, but it is actually backed by a
// QVarLengthArray, not a QMap. So it is okay that
// m_channelStateMatrix may be accessed concurrently in the main
// thread in deleteStatesForInputChannel.

// Can't directly cast a ChannelHandleMap from containing the base
// EffectState* type to EffectSpecificState* type, so iterate through
// pStatesMap to build a new ChannelHandleMap with
// dynamic_cast'ed states.
ChannelHandleMap<EffectSpecificState*>& effectSpecificStatesMap =
m_channelStateMatrix[inputChannel];

// deleteStatesForInputChannel should have been called before a new
// map of EffectStates was sent to this function, or this is the first
// time states are being loaded for this input channel, so
// effectSpecificStatesMap should be empty and this loop should
// not go through any iterations.
for (EffectSpecificState* pState : effectSpecificStatesMap) {
VERIFY_OR_DEBUG_ASSERT(pState == nullptr) {
delete pState;
}
qDebug() << this << "EffectProcessorImpl::initialize allocating "
"EffectStates for input"
<< inputChannel;
}

QSet<ChannelHandleAndGroup> receivedOutputChannels = m_registeredOutputChannels;
ChannelHandleMap<EffectSpecificState*> outputChannelMap;
for (const ChannelHandleAndGroup& outputChannel :
std::as_const(m_registeredOutputChannels)) {
outputChannelMap.insert(outputChannel.handle(),
createSpecificState(engineParameters));
if (kEffectDebugOutput) {
qDebug() << "EffectProcessorImpl::loadStatesForInputChannel"
<< this << "output" << outputChannel;
qDebug() << this
<< "EffectProcessorImpl::initialize "
"registering output"
<< outputChannel << outputChannel.handle()
<< outputChannelMap[outputChannel.handle()];
}

auto pState = dynamic_cast<EffectSpecificState*>(
pStatesMap->at(outputChannel.handle()));
VERIFY_OR_DEBUG_ASSERT(pState != nullptr) {
return false;
}
effectSpecificStatesMap.insert(outputChannel.handle(), pState);
receivedOutputChannels.insert(outputChannel);
}
// Output channels are hardcoded in EngineMaster and should not
// be registered after Mixxx initializes.
DEBUG_ASSERT(receivedOutputChannels == m_registeredOutputChannels);
return true;
m_channelStateMatrix.insert(inputChannel, outputChannelMap);
};

/// Called from main thread for garbage collection after an input channel is disabled
void deleteStatesForInputChannel(ChannelHandle inputChannel) final {
if (kEffectDebugOutput) {
qDebug() << "EffectProcessorImpl::deleteStatesForInputChannel"
<< this << inputChannel;
}

// NOTE: ChannelHandleMap is like a map in that it associates an
// object with a ChannelHandle key, but it is actually backed by a
// QVarLengthArray, not a QMap. So it is okay that
// m_channelStateMatrix may be accessed concurrently in the audio
// engine thread in loadStatesForInputChannel.

ChannelHandleMap<EffectSpecificState*>& stateMap =
m_channelStateMatrix[inputChannel];
for (EffectSpecificState* pState : stateMap) {
VERIFY_OR_DEBUG_ASSERT(pState != nullptr) {
continue;
}
if (kEffectDebugOutput) {
qDebug() << "EffectProcessorImpl::deleteStatesForInputChannel"
<< this << "deleting state" << pState;
bool hasStatesForInputChannel(ChannelHandle inputChannel) const {
if (inputChannel.handle() < m_channelStateMatrix.size()) {
for (EffectSpecificState* pState : m_channelStateMatrix.at(inputChannel)) {
if (pState) {
return true;
}
}
delete pState;
}
stateMap.clear();
};
return false;
}

protected:
/// Subclasses for external effects plugins may reimplement this, but
Expand Down
15 changes: 9 additions & 6 deletions src/effects/chains/equalizereffectchain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,25 @@

#include "effects/effectslot.h"

EqualizerEffectChain::EqualizerEffectChain(const QString& group,
EqualizerEffectChain::EqualizerEffectChain(
const ChannelHandleAndGroup& handleAndGroup,
EffectsManager* pEffectsManager,
EffectsMessengerPointer pEffectsMessenger)
: PerGroupEffectChain(group,
formatEffectChainGroup(group),
: PerGroupEffectChain(
handleAndGroup,
formatEffectChainGroup(handleAndGroup.name()),
SignalProcessingStage::Prefader,
pEffectsManager,
pEffectsMessenger),
m_pCOFilterWaveform(
new ControlObject(ConfigKey(group, "filterWaveformEnable"))) {
new ControlObject(ConfigKey(handleAndGroup.name(), "filterWaveformEnable"))) {
// Add a single effect slot
addEffectSlot(formatEffectSlotGroup(group));
addEffectSlot(formatEffectSlotGroup(handleAndGroup.name()));
enableForInputChannel(handleAndGroup);
m_effectSlots[0]->setEnabled(true);
// DlgPrefEq loads the Effect with loadEffectToGroup

setupLegacyAliasesForGroup(group);
setupLegacyAliasesForGroup(handleAndGroup.name());
}

void EqualizerEffectChain::setFilterWaveform(bool state) {
Expand Down
3 changes: 2 additions & 1 deletion src/effects/chains/equalizereffectchain.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
class EqualizerEffectChain : public PerGroupEffectChain {
Q_OBJECT
public:
EqualizerEffectChain(const QString& group,
EqualizerEffectChain(
const ChannelHandleAndGroup& handleAndGroup,
EffectsManager* pEffectsManager,
EffectsMessengerPointer pEffectsMessenger);

Expand Down
17 changes: 3 additions & 14 deletions src/effects/chains/pergroupeffectchain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

#include "effects/effectsmanager.h"

PerGroupEffectChain::PerGroupEffectChain(const QString& group,
PerGroupEffectChain::PerGroupEffectChain(
const ChannelHandleAndGroup& handleAndGroup,
const QString& chainSlotGroup,
SignalProcessingStage stage,
EffectsManager* pEffectsManager,
Expand All @@ -15,18 +16,6 @@ PerGroupEffectChain::PerGroupEffectChain(const QString& group,
m_pControlChainMix->set(1.0);
sendParameterUpdate();

// TODO(rryan): remove.
const ChannelHandleAndGroup* handleAndGroup = nullptr;
for (const ChannelHandleAndGroup& handle_group :
m_pEffectsManager->registeredInputChannels()) {
if (handle_group.name() == group) {
handleAndGroup = &handle_group;
break;
}
}
DEBUG_ASSERT(handleAndGroup != nullptr);

// Register this channel alone with the chain slot.
registerInputChannel(*handleAndGroup);
enableForInputChannel(*handleAndGroup);
registerInputChannel(handleAndGroup);
}
3 changes: 2 additions & 1 deletion src/effects/chains/pergroupeffectchain.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
class PerGroupEffectChain : public EffectChain {
Q_OBJECT
public:
PerGroupEffectChain(const QString& group,
PerGroupEffectChain(
const ChannelHandleAndGroup& handleGroup,
const QString& chainSlotGroup,
SignalProcessingStage stage,
EffectsManager* pEffectsManager,
Expand Down
11 changes: 7 additions & 4 deletions src/effects/chains/quickeffectchain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,21 @@
#include "effects/effectslot.h"
#include "effects/presets/effectchainpresetmanager.h"

QuickEffectChain::QuickEffectChain(const QString& group,
QuickEffectChain::QuickEffectChain(
const ChannelHandleAndGroup& handleAndGroup,
EffectsManager* pEffectsManager,
EffectsMessengerPointer pEffectsMessenger)
: PerGroupEffectChain(group,
formatEffectChainGroup(group),
: PerGroupEffectChain(
handleAndGroup,
formatEffectChainGroup(handleAndGroup.name()),
SignalProcessingStage::Postfader,
pEffectsManager,
pEffectsMessenger) {
for (int i = 0; i < kNumEffectsPerUnit; ++i) {
addEffectSlot(formatEffectSlotGroup(group, i));
addEffectSlot(formatEffectSlotGroup(handleAndGroup.name(), i));
m_effectSlots.at(i)->setEnabled(true);
}
enableForInputChannel(handleAndGroup);
// The base EffectChain class constructor connects to the signal for the list of StandardEffectChain presets,
// but QuickEffectChain has a separate list, so disconnect the signal which is inappropriate for this subclass.
disconnect(m_pChainPresetManager.data(),
Expand Down
3 changes: 2 additions & 1 deletion src/effects/chains/quickeffectchain.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
class QuickEffectChain : public PerGroupEffectChain {
Q_OBJECT
public:
QuickEffectChain(const QString& group,
QuickEffectChain(
const ChannelHandleAndGroup& handleAndGroup,
EffectsManager* pEffectsManager,
EffectsMessengerPointer pEffectsMessenger);

Expand Down
20 changes: 4 additions & 16 deletions src/effects/effectchain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -396,24 +396,12 @@ void EffectChain::enableForInputChannel(const ChannelHandleAndGroup& handleGroup
request->pTargetChain = m_pEngineEffectChain;
request->EnableInputChannelForChain.channelHandle = handleGroup.handle();

// Allocate EffectStates here in the main thread to avoid allocating
// memory in the realtime audio callback thread. Pointers to the
// EffectStates are passed to the EffectRequest and the EffectProcessorImpls
// store the pointers. The containers of EffectState* pointers get deleted
// by ~EffectsRequest, but the EffectStates are managed by EffectProcessorImpl.

// The EffectStates for one EngineEffectChain must be sent all together in
// the same message using an EffectStatesMapArray. If they were separated
// into a message for each effect, there would be a chance that the
// EngineEffectChain could get activated in one cycle of the audio callback
// thread but the EffectStates for an EngineEffect would not be received by
// EngineEffectsManager until the next audio callback cycle.

auto* pEffectStatesMapArray = new EffectStatesMapArray;
// Initialize EffectStates for the input channel here in the main thread to
// avoid allocating memory in the realtime audio callback thread.

for (int i = 0; i < m_effectSlots.size(); ++i) {
m_effectSlots[i]->fillEffectStatesMap(&(*pEffectStatesMapArray)[i]);
m_effectSlots[i]->initalizeInputChannel(handleGroup.handle());
}
request->EnableInputChannelForChain.pEffectStatesMapArray = pEffectStatesMapArray;

m_pMessenger->writeRequest(request);

Expand Down
23 changes: 4 additions & 19 deletions src/effects/effectslot.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -192,26 +192,11 @@ void EffectSlot::updateEngineState() {
}
}

void EffectSlot::fillEffectStatesMap(EffectStatesMap* pStatesMap) const {
//TODO: get actual configuration of engine
const mixxx::EngineParameters engineParameters(
mixxx::audio::SampleRate(96000),
MAX_BUFFER_LEN / mixxx::kEngineChannelCount);

if (isLoaded()) {
for (const auto& outputChannel :
m_pEffectsManager->registeredOutputChannels()) {
pStatesMap->insert(outputChannel.handle(),
m_pEngineEffect->createState(engineParameters));
}
} else {
for (EffectState* pState : *pStatesMap) {
if (pState) {
delete pState;
}
}
pStatesMap->clear();
void EffectSlot::initalizeInputChannel(ChannelHandle inputChannel) {
if (!m_pEngineEffect) {
return;
}
m_pEngineEffect->initalizeInputChannel(inputChannel);
};

EffectManifestPointer EffectSlot::getManifest() const {
Expand Down
2 changes: 1 addition & 1 deletion src/effects/effectslot.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class EffectSlot : public QObject {
return m_group;
}

void fillEffectStatesMap(EffectStatesMap* pStatesMap) const;
void initalizeInputChannel(ChannelHandle inputChannel);

EffectManifestPointer getManifest() const;

Expand Down
22 changes: 11 additions & 11 deletions src/effects/effectsmanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -127,34 +127,34 @@ EffectChainPointer EffectsManager::getStandardEffectChain(int unitNumber) const
return m_standardEffectChains.at(unitNumber);
}

void EffectsManager::addDeck(const QString& deckGroupName) {
addEqualizerEffectChain(deckGroupName);
addQuickEffectChain(deckGroupName);
void EffectsManager::addDeck(const ChannelHandleAndGroup& deckHandleGroup) {
addEqualizerEffectChain(deckHandleGroup);
addQuickEffectChain(deckHandleGroup);
}

void EffectsManager::addEqualizerEffectChain(const QString& deckGroupName) {
void EffectsManager::addEqualizerEffectChain(const ChannelHandleAndGroup& deckHandleGroup) {
VERIFY_OR_DEBUG_ASSERT(!m_equalizerEffectChains.contains(
EqualizerEffectChain::formatEffectChainGroup(deckGroupName))) {
EqualizerEffectChain::formatEffectChainGroup(deckHandleGroup.name()))) {
return;
}

auto pChainSlot = EqualizerEffectChainPointer(
new EqualizerEffectChain(deckGroupName, this, m_pMessenger));
new EqualizerEffectChain(deckHandleGroup, this, m_pMessenger));

m_equalizerEffectChains.insert(deckGroupName, pChainSlot);
m_equalizerEffectChains.insert(deckHandleGroup.name(), pChainSlot);
m_effectChainSlotsByGroup.insert(pChainSlot->group(), pChainSlot);
}

void EffectsManager::addQuickEffectChain(const QString& deckGroupName) {
void EffectsManager::addQuickEffectChain(const ChannelHandleAndGroup& deckHandleGroup) {
VERIFY_OR_DEBUG_ASSERT(!m_quickEffectChains.contains(
QuickEffectChain::formatEffectChainGroup(deckGroupName))) {
QuickEffectChain::formatEffectChainGroup(deckHandleGroup.name()))) {
return;
}

auto pChainSlot = QuickEffectChainPointer(
new QuickEffectChain(deckGroupName, this, m_pMessenger));
new QuickEffectChain(deckHandleGroup, this, m_pMessenger));

m_quickEffectChains.insert(deckGroupName, pChainSlot);
m_quickEffectChains.insert(deckHandleGroup.name(), pChainSlot);
m_effectChainSlotsByGroup.insert(pChainSlot->group(), pChainSlot);
}

Expand Down
Loading