From 29fa03b31b36db4744326f35153f48b15b4fbed3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Tue, 19 Oct 2021 23:16:33 +0200 Subject: [PATCH 01/21] Added a TODO for a probably data race in effectprocessor. --- src/effects/backends/effectprocessor.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/effects/backends/effectprocessor.h b/src/effects/backends/effectprocessor.h index 837da2b9104..ce68fa03e1d 100644 --- a/src/effects/backends/effectprocessor.h +++ b/src/effects/backends/effectprocessor.h @@ -157,6 +157,11 @@ class EffectProcessorImpl : public EffectProcessor { const EffectEnableState enableState, const GroupFeatureState& groupFeatures) final { EffectSpecificState* pState = m_channelStateMatrix[inputHandle][outputHandle]; + // TODO: The state can be null if we are in the deleteStatesForInputChannel() loop. + // A protection against this is missing. Since it can happen the assertion is incorrect + // The memory will be leaked. + // Idea: Skip the processing here? + // Probably related: https://bugs.launchpad.net/mixxx/+bug/1775497 VERIFY_OR_DEBUG_ASSERT(pState != nullptr) { if (kEffectDebugOutput) { qWarning() << "EffectProcessorImpl::process could not retrieve" From 36fbe826ae9391a74a938ae62efa1d925277e755 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Tue, 19 Oct 2021 23:45:40 +0200 Subject: [PATCH 02/21] Added a TODO for another probably data race in effectprocessor. --- src/effects/backends/effectprocessor.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/effects/backends/effectprocessor.h b/src/effects/backends/effectprocessor.h index ce68fa03e1d..add23a3f964 100644 --- a/src/effects/backends/effectprocessor.h +++ b/src/effects/backends/effectprocessor.h @@ -273,6 +273,11 @@ class EffectProcessorImpl : public EffectProcessor { // m_channelStateMatrix may be accessed concurrently in the audio // engine thread in loadStatesForInputChannel. + // TODO: How is ensure that the statMap is not accessed during this loop? + // This is called in responds to DISABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL + // and it looks like that the objects pointed by m_channelStateMatrix + // may be still in use. + // Probably related: https://bugs.launchpad.net/mixxx/+bug/1775497 ChannelHandleMap& stateMap = m_channelStateMatrix[inputChannel]; for (EffectSpecificState* pState : stateMap) { From 027ddee4dcc20afcbd5d0ee6c0b4ccab37ffa7d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 16 Mar 2022 11:04:52 +0100 Subject: [PATCH 03/21] Remove unused function EnginEffectChain::getChannelStatus() --- src/engine/effects/engineeffectchain.cpp | 7 ------- src/engine/effects/engineeffectchain.h | 5 ----- 2 files changed, 12 deletions(-) diff --git a/src/engine/effects/engineeffectchain.cpp b/src/engine/effects/engineeffectchain.cpp index c3101ecb5ab..75ea0327b90 100644 --- a/src/engine/effects/engineeffectchain.cpp +++ b/src/engine/effects/engineeffectchain.cpp @@ -220,13 +220,6 @@ void EngineEffectChain::deleteStatesForInputChannel(const ChannelHandle inputCha } } -EngineEffectChain::ChannelStatus& EngineEffectChain::getChannelStatus( - const ChannelHandle& inputHandle, - const ChannelHandle& outputHandle) { - ChannelStatus& status = m_chainStatusForChannelMatrix[inputHandle][outputHandle]; - return status; -} - bool EngineEffectChain::process(const ChannelHandle& inputHandle, const ChannelHandle& outputHandle, CSAMPLE* pIn, diff --git a/src/engine/effects/engineeffectchain.h b/src/engine/effects/engineeffectchain.h index 9bb5aa6ac35..7effa29f897 100644 --- a/src/engine/effects/engineeffectchain.h +++ b/src/engine/effects/engineeffectchain.h @@ -68,11 +68,6 @@ class EngineEffectChain final : public EffectsRequestHandler { EffectStatesMapArray* statesForEffectsInChain); bool disableForInputChannel(ChannelHandle inputHandle); - // Gets or creates a ChannelStatus entry in m_channelStatus for the provided - // handle. - ChannelStatus& getChannelStatus(const ChannelHandle& inputHandle, - const ChannelHandle& outputHandle); - QString m_group; EffectEnableState m_enableState; EffectChainMixMode::Type m_mixMode; From fd60cf9408a77d1f6a41f77c5d7f0430a3aeaf64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 16 Mar 2022 11:07:29 +0100 Subject: [PATCH 04/21] Avoid write access via reference --- src/engine/effects/engineeffectchain.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/engine/effects/engineeffectchain.cpp b/src/engine/effects/engineeffectchain.cpp index 75ea0327b90..66ab71f22d7 100644 --- a/src/engine/effects/engineeffectchain.cpp +++ b/src/engine/effects/engineeffectchain.cpp @@ -342,11 +342,10 @@ bool EngineEffectChain::process(const ChannelHandle& inputHandle, // enabling/disabling state, set the channel state or chain state // to the fully enabled/disabled state for the next engine callback. - EffectEnableState& chainOnChannelEnableState = channelStatus.enableState; - if (chainOnChannelEnableState == EffectEnableState::Disabling) { - chainOnChannelEnableState = EffectEnableState::Disabled; - } else if (chainOnChannelEnableState == EffectEnableState::Enabling) { - chainOnChannelEnableState = EffectEnableState::Enabled; + if (channelStatus.enableState == EffectEnableState::Disabling) { + channelStatus.enableState = EffectEnableState::Disabled; + } else if (channelStatus.enableState == EffectEnableState::Enabling) { + channelStatus.enableState = EffectEnableState::Enabled; } if (m_enableState == EffectEnableState::Disabling) { From e379844a1ca0bb195a1f319664456f2b72432305 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 16 Mar 2022 21:25:00 +0100 Subject: [PATCH 05/21] Add ChannelHandleMap::isEmpty() --- src/engine/channelhandle.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/engine/channelhandle.h b/src/engine/channelhandle.h index e20a9e1583e..e8335c1fc3e 100644 --- a/src/engine/channelhandle.h +++ b/src/engine/channelhandle.h @@ -207,6 +207,10 @@ class ChannelHandleMap { m_data.clear(); } + bool isEmpty() { + return m_data.isEmpty(); + } + typename container_type::iterator begin() { return m_data.begin(); } From b019fa7ff130e4083b830a36f2c685329a4a3d9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Thu, 17 Mar 2022 21:31:42 +0100 Subject: [PATCH 06/21] Pass ChannelHandleAndGroup instead of just the group name to avoid handle lookup --- src/effects/chains/equalizereffectchain.cpp | 14 +++++++------ src/effects/chains/equalizereffectchain.h | 3 ++- src/effects/chains/pergroupeffectchain.cpp | 18 ++++------------- src/effects/chains/pergroupeffectchain.h | 3 ++- src/effects/chains/quickeffectchain.cpp | 10 ++++++---- src/effects/chains/quickeffectchain.h | 3 ++- src/effects/effectsmanager.cpp | 22 ++++++++++----------- src/effects/effectsmanager.h | 6 +++--- src/mixer/playermanager.cpp | 2 +- 9 files changed, 39 insertions(+), 42 deletions(-) diff --git a/src/effects/chains/equalizereffectchain.cpp b/src/effects/chains/equalizereffectchain.cpp index 4b5e7ce3476..cabb617fc64 100644 --- a/src/effects/chains/equalizereffectchain.cpp +++ b/src/effects/chains/equalizereffectchain.cpp @@ -2,22 +2,24 @@ #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())); m_effectSlots[0]->setEnabled(true); // DlgPrefEq loads the Effect with loadEffectToGroup - setupLegacyAliasesForGroup(group); + setupLegacyAliasesForGroup(handleAndGroup.name()); } void EqualizerEffectChain::setFilterWaveform(bool state) { diff --git a/src/effects/chains/equalizereffectchain.h b/src/effects/chains/equalizereffectchain.h index 98a15973838..706c0e6f7e8 100644 --- a/src/effects/chains/equalizereffectchain.h +++ b/src/effects/chains/equalizereffectchain.h @@ -11,7 +11,8 @@ class EqualizerEffectChain : public PerGroupEffectChain { Q_OBJECT public: - EqualizerEffectChain(const QString& group, + EqualizerEffectChain( + const ChannelHandleAndGroup& handleAndGroup, EffectsManager* pEffectsManager, EffectsMessengerPointer pEffectsMessenger); diff --git a/src/effects/chains/pergroupeffectchain.cpp b/src/effects/chains/pergroupeffectchain.cpp index 2ec903bbb4c..1630f99dcf6 100644 --- a/src/effects/chains/pergroupeffectchain.cpp +++ b/src/effects/chains/pergroupeffectchain.cpp @@ -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, @@ -15,18 +16,7 @@ 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); + enableForInputChannel(handleAndGroup); } diff --git a/src/effects/chains/pergroupeffectchain.h b/src/effects/chains/pergroupeffectchain.h index acf10c3743c..dc510a930bb 100644 --- a/src/effects/chains/pergroupeffectchain.h +++ b/src/effects/chains/pergroupeffectchain.h @@ -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, diff --git a/src/effects/chains/quickeffectchain.cpp b/src/effects/chains/quickeffectchain.cpp index 617080f968a..fb816448d1f 100644 --- a/src/effects/chains/quickeffectchain.cpp +++ b/src/effects/chains/quickeffectchain.cpp @@ -3,16 +3,18 @@ #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); } // The base EffectChain class constructor connects to the signal for the list of StandardEffectChain presets, diff --git a/src/effects/chains/quickeffectchain.h b/src/effects/chains/quickeffectchain.h index 34c747281f7..eecd76cc776 100644 --- a/src/effects/chains/quickeffectchain.h +++ b/src/effects/chains/quickeffectchain.h @@ -10,7 +10,8 @@ class QuickEffectChain : public PerGroupEffectChain { Q_OBJECT public: - QuickEffectChain(const QString& group, + QuickEffectChain( + const ChannelHandleAndGroup& handleAndGroup, EffectsManager* pEffectsManager, EffectsMessengerPointer pEffectsMessenger); diff --git a/src/effects/effectsmanager.cpp b/src/effects/effectsmanager.cpp index 36c107e344d..23c20cf83be 100644 --- a/src/effects/effectsmanager.cpp +++ b/src/effects/effectsmanager.cpp @@ -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); } diff --git a/src/effects/effectsmanager.h b/src/effects/effectsmanager.h index 2e45b256405..d6efe32f8e4 100644 --- a/src/effects/effectsmanager.h +++ b/src/effects/effectsmanager.h @@ -26,7 +26,7 @@ class EffectsManager { virtual ~EffectsManager(); void setup(); - void addDeck(const QString& deckGroupName); + void addDeck(const ChannelHandleAndGroup& deckHandleGroup); EffectChainPointer getEffectChain(const QString& group) const; EqualizerEffectChainPointer getEqualizerEffectChain( @@ -75,8 +75,8 @@ class EffectsManager { void addStandardEffectChains(); void addOutputEffectChain(); - void addEqualizerEffectChain(const QString& deckGroupName); - void addQuickEffectChain(const QString& deckGroupName); + void addEqualizerEffectChain(const ChannelHandleAndGroup& deckHandleGroup); + void addQuickEffectChain(const ChannelHandleAndGroup& deckHandleGroup); void readEffectsXml(); void saveEffectsXml(); diff --git a/src/mixer/playermanager.cpp b/src/mixer/playermanager.cpp index 0f018bd0e4e..725e7500a5c 100644 --- a/src/mixer/playermanager.cpp +++ b/src/mixer/playermanager.cpp @@ -436,7 +436,7 @@ void PlayerManager::addDeckInner() { AudioInput(AudioInput::VINYLCONTROL, 0, 2, deckIndex), pEngineDeck); // Setup equalizer and QuickEffect chain for this deck. - m_pEffectsManager->addDeck(handleGroup.m_name); + m_pEffectsManager->addDeck(handleGroup); } void PlayerManager::loadSamplers() { From f5bb9a1b04ba819c5f6d7caf894509fe3888c99d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 19 Mar 2022 09:08:52 +0100 Subject: [PATCH 07/21] Move the skin warning before the assertion --- src/widget/weffectpushbutton.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/widget/weffectpushbutton.cpp b/src/widget/weffectpushbutton.cpp index a733e51dabe..f979962502c 100644 --- a/src/widget/weffectpushbutton.cpp +++ b/src/widget/weffectpushbutton.cpp @@ -22,6 +22,10 @@ void WEffectPushButton::setup(const QDomNode& node, const SkinContext& context) auto pEffectSlot = EffectWidgetUtils::getEffectSlotFromNode(node, context, pChainSlot); m_pEffectParameterSlot = EffectWidgetUtils::getButtonParameterSlotFromNode( node, context, pEffectSlot); + if (!m_pEffectParameterSlot) { + SKIN_WARNING(node, context) << "Could not find effect parameter slot"; + DEBUG_ASSERT(false); + } connect(m_pEffectParameterSlot.data(), &EffectParameterSlotBase::updated, this, @@ -33,9 +37,6 @@ void WEffectPushButton::setup(const QDomNode& node, const SkinContext& context) this, &WEffectPushButton::slotActionChosen); parameterUpdated(); - VERIFY_OR_DEBUG_ASSERT(m_pEffectParameterSlot) { - SKIN_WARNING(node, context) << "Could not find effect parameter slot"; - } } void WEffectPushButton::onConnectedControlChanged(double dParameter, double dValue) { From 5663ae242e01546c15f9f8033512aea5cf6b987f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 19 Mar 2022 09:17:12 +0100 Subject: [PATCH 08/21] Enable the effects AFTER we have added effect slots --- src/effects/chains/equalizereffectchain.cpp | 1 + src/effects/chains/pergroupeffectchain.cpp | 1 - src/effects/chains/quickeffectchain.cpp | 1 + 3 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/effects/chains/equalizereffectchain.cpp b/src/effects/chains/equalizereffectchain.cpp index cabb617fc64..320406a8935 100644 --- a/src/effects/chains/equalizereffectchain.cpp +++ b/src/effects/chains/equalizereffectchain.cpp @@ -16,6 +16,7 @@ EqualizerEffectChain::EqualizerEffectChain( new ControlObject(ConfigKey(handleAndGroup.name(), "filterWaveformEnable"))) { // Add a single effect slot addEffectSlot(formatEffectSlotGroup(handleAndGroup.name())); + enableForInputChannel(handleAndGroup); m_effectSlots[0]->setEnabled(true); // DlgPrefEq loads the Effect with loadEffectToGroup diff --git a/src/effects/chains/pergroupeffectchain.cpp b/src/effects/chains/pergroupeffectchain.cpp index 1630f99dcf6..745a3920794 100644 --- a/src/effects/chains/pergroupeffectchain.cpp +++ b/src/effects/chains/pergroupeffectchain.cpp @@ -18,5 +18,4 @@ PerGroupEffectChain::PerGroupEffectChain( // Register this channel alone with the chain slot. registerInputChannel(handleAndGroup); - enableForInputChannel(handleAndGroup); } diff --git a/src/effects/chains/quickeffectchain.cpp b/src/effects/chains/quickeffectchain.cpp index fb816448d1f..7df93c380ba 100644 --- a/src/effects/chains/quickeffectchain.cpp +++ b/src/effects/chains/quickeffectchain.cpp @@ -17,6 +17,7 @@ QuickEffectChain::QuickEffectChain( 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(), From 895a7e8d6bf796b80dcd8c12aa05e10c669d8407 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 23 Mar 2022 22:28:09 +0100 Subject: [PATCH 09/21] extend scope for a better readability --- src/engine/effects/engineeffectsmanager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/effects/engineeffectsmanager.cpp b/src/engine/effects/engineeffectsmanager.cpp index 5dae585c537..9b5590e4eca 100644 --- a/src/engine/effects/engineeffectsmanager.cpp +++ b/src/engine/effects/engineeffectsmanager.cpp @@ -45,7 +45,6 @@ void EngineEffectsManager::onCallbackStart() { response.status = EffectsResponse::NO_SUCH_CHAIN; break; } - } processed = request->pTargetChain->processEffectsRequest( *request, m_pResponsePipe.data()); if (processed) { @@ -64,6 +63,7 @@ void EngineEffectsManager::onCallbackStart() { response.status = EffectsResponse::INVALID_REQUEST; } break; + } case EffectsRequest::SET_EFFECT_PARAMETERS: case EffectsRequest::SET_PARAMETER_PARAMETERS: VERIFY_OR_DEBUG_ASSERT(m_effects.contains(request->pTargetEffect)) { From 2d2bb6c6226d1cb9931fd6863bdd90b330efb89c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 26 Mar 2022 14:49:27 +0100 Subject: [PATCH 10/21] Introduce an updateActiveState() function to be able to clean up buffers after a channel has been disabled. --- src/engine/channels/engineaux.cpp | 16 ++++---- src/engine/channels/engineaux.h | 3 +- src/engine/channels/enginechannel.cpp | 1 + src/engine/channels/enginechannel.h | 13 ++++++- src/engine/channels/enginedeck.cpp | 18 +++++---- src/engine/channels/enginedeck.h | 3 +- src/engine/channels/enginemicrophone.cpp | 16 ++++---- src/engine/channels/enginemicrophone.h | 4 +- src/engine/effects/engineeffectchain.cpp | 14 ++++++- src/engine/enginemaster.cpp | 12 +++++- src/test/enginemastertest.cpp | 49 ++++++++++++------------ 11 files changed, 92 insertions(+), 57 deletions(-) diff --git a/src/engine/channels/engineaux.cpp b/src/engine/channels/engineaux.cpp index 99ff35631e4..b555ed770f3 100644 --- a/src/engine/channels/engineaux.cpp +++ b/src/engine/channels/engineaux.cpp @@ -15,8 +15,7 @@ EngineAux::EngineAux(const ChannelHandleAndGroup& handleGroup, EffectsManager* p /*isTalkoverChannel*/ false, /*isPrimaryDeck*/ false), m_pInputConfigured(new ControlObject(ConfigKey(getGroup(), "input_configured"))), - m_pPregain(new ControlAudioTaperPot(ConfigKey(getGroup(), "pregain"), -12, 12, 0.5)), - m_wasActive(false) { + m_pPregain(new ControlAudioTaperPot(ConfigKey(getGroup(), "pregain"), -12, 12, 0.5)) { // Make input_configured read-only. m_pInputConfigured->setReadOnly(); ControlDoublePrivate::insertAlias(ConfigKey(getGroup(), "enabled"), @@ -32,15 +31,18 @@ EngineAux::~EngineAux() { delete m_pPregain; } -bool EngineAux::isActive() { +EngineChannel::ActiveState EngineAux::updateActiveState() { bool enabled = m_pInputConfigured->toBool(); if (enabled && m_sampleBuffer) { - m_wasActive = true; - } else if (m_wasActive) { + m_active = true; + return ActiveState::Active; + } + if (m_active) { m_vuMeter.reset(); - m_wasActive = false; + m_active = false; + return ActiveState::WasActive; } - return m_wasActive; + return ActiveState::Inactive; } void EngineAux::onInputConfigured(const AudioInput& input) { diff --git a/src/engine/channels/engineaux.h b/src/engine/channels/engineaux.h index aa6c8fe72e7..5934ca5fec0 100644 --- a/src/engine/channels/engineaux.h +++ b/src/engine/channels/engineaux.h @@ -18,7 +18,7 @@ class EngineAux : public EngineChannel, public AudioDestination { EngineAux(const ChannelHandleAndGroup& handleGroup, EffectsManager* pEffectsManager); virtual ~EngineAux(); - bool isActive(); + ActiveState updateActiveState() override; /// Called by EngineMaster whenever is requesting a new buffer of audio. virtual void process(CSAMPLE* pOutput, const int iBufferSize); @@ -45,5 +45,4 @@ class EngineAux : public EngineChannel, public AudioDestination { private: QScopedPointer m_pInputConfigured; ControlAudioTaperPot* m_pPregain; - bool m_wasActive; }; diff --git a/src/engine/channels/enginechannel.cpp b/src/engine/channels/enginechannel.cpp index 3756f341c44..941a0c7921c 100644 --- a/src/engine/channels/enginechannel.cpp +++ b/src/engine/channels/enginechannel.cpp @@ -15,6 +15,7 @@ EngineChannel::EngineChannel(const ChannelHandleAndGroup& handleGroup, m_pSampleRate(new ControlProxy("[Master]", "samplerate")), m_sampleBuffer(nullptr), m_bIsPrimaryDeck(isPrimaryDeck), + m_active(false), m_bIsTalkoverChannel(isTalkoverChannel), m_channelIndex(-1) { m_pPFL = new ControlPushButton(ConfigKey(getGroup(), "pfl")); diff --git a/src/engine/channels/enginechannel.h b/src/engine/channels/enginechannel.h index 6771447c158..6f17da014c1 100644 --- a/src/engine/channels/enginechannel.h +++ b/src/engine/channels/enginechannel.h @@ -21,6 +21,12 @@ class EngineChannel : public EngineObject { RIGHT, }; + enum class ActiveState { + Inactive = 0, + Active, + WasActive + }; + EngineChannel(const ChannelHandleAndGroup& handleGroup, ChannelOrientation defaultOrientation, EffectsManager* pEffectsManager, @@ -38,7 +44,11 @@ class EngineChannel : public EngineObject { return m_group.name(); } - virtual bool isActive() = 0; + virtual ActiveState updateActiveState() = 0; + bool isActive() { + return m_active; + } + void setPfl(bool enabled); virtual bool isPflEnabled() const; void setMaster(bool enabled); @@ -76,6 +86,7 @@ class EngineChannel : public EngineObject { // If set to true, this engine channel represents one of the primary playback decks. // It is used to check for valid bpm targets by the sync code. const bool m_bIsPrimaryDeck; + bool m_active; private slots: void slotOrientationLeft(double v); diff --git a/src/engine/channels/enginedeck.cpp b/src/engine/channels/enginedeck.cpp index de05794bca5..67a3e8f3894 100644 --- a/src/engine/channels/enginedeck.cpp +++ b/src/engine/channels/enginedeck.cpp @@ -22,10 +22,7 @@ EngineDeck::EngineDeck( primaryDeck), m_pConfig(pConfig), m_pInputConfigured(new ControlObject(ConfigKey(getGroup(), "input_configured"))), - m_pPassing(new ControlPushButton(ConfigKey(getGroup(), "passthrough"))), - // Need a +1 here because the CircularBuffer only allows its size-1 - // items to be held at once (it keeps a blank spot open persistently) - m_wasActive(false) { + m_pPassing(new ControlPushButton(ConfigKey(getGroup(), "passthrough"))) { m_pInputConfigured->setReadOnly(); // Set up passthrough utilities and fields m_pPassing->setButtonMode(ControlPushButton::POWERWINDOW); @@ -101,7 +98,7 @@ EngineBuffer* EngineDeck::getEngineBuffer() { return m_pBuffer; } -bool EngineDeck::isActive() { +EngineChannel::ActiveState EngineDeck::updateActiveState() { bool active = false; if (m_bPassthroughWasActive && !m_bPassthroughIsActive) { active = true; @@ -109,11 +106,16 @@ bool EngineDeck::isActive() { active = m_pBuffer->isTrackLoaded() || isPassthroughActive(); } - if (!active && m_wasActive) { + if (active) { + m_active = true; + return ActiveState::Active; + } + if (m_active) { m_vuMeter.reset(); + m_active = false; + return ActiveState::WasActive; } - m_wasActive = active; - return active; + return ActiveState::Inactive; } void EngineDeck::receiveBuffer( diff --git a/src/engine/channels/enginedeck.h b/src/engine/channels/enginedeck.h index a6bb982144c..50f61ec8622 100644 --- a/src/engine/channels/enginedeck.h +++ b/src/engine/channels/enginedeck.h @@ -38,7 +38,7 @@ class EngineDeck : public EngineChannel, public AudioDestination { // TODO(XXX) This hack needs to be removed. virtual EngineBuffer* getEngineBuffer(); - virtual bool isActive(); + EngineChannel::ActiveState updateActiveState() override; // This is called by SoundManager whenever there are new samples from the // configured input to be processed. This is run in the callback thread of @@ -77,5 +77,4 @@ class EngineDeck : public EngineChannel, public AudioDestination { ControlPushButton* m_pPassing; bool m_bPassthroughIsActive; bool m_bPassthroughWasActive; - bool m_wasActive; }; diff --git a/src/engine/channels/enginemicrophone.cpp b/src/engine/channels/enginemicrophone.cpp index c7d41ac9750..943eb7b8eb3 100644 --- a/src/engine/channels/enginemicrophone.cpp +++ b/src/engine/channels/enginemicrophone.cpp @@ -16,8 +16,7 @@ EngineMicrophone::EngineMicrophone(const ChannelHandleAndGroup& handleGroup, /*isTalkoverChannel*/ true, /*isPrimaryDeck*/ false), m_pInputConfigured(new ControlObject(ConfigKey(getGroup(), "input_configured"))), - m_pPregain(new ControlAudioTaperPot(ConfigKey(getGroup(), "pregain"), -12, 12, 0.5)), - m_wasActive(false) { + m_pPregain(new ControlAudioTaperPot(ConfigKey(getGroup(), "pregain"), -12, 12, 0.5)) { // Make input_configured read-only. m_pInputConfigured->setReadOnly(); ControlDoublePrivate::insertAlias(ConfigKey(getGroup(), "enabled"), @@ -30,15 +29,18 @@ EngineMicrophone::~EngineMicrophone() { delete m_pPregain; } -bool EngineMicrophone::isActive() { +EngineChannel::ActiveState EngineMicrophone::updateActiveState() { bool enabled = m_pInputConfigured->toBool(); if (enabled && m_sampleBuffer) { - m_wasActive = true; - } else if (m_wasActive) { + m_active = true; + return ActiveState::Active; + } + if (m_active) { m_vuMeter.reset(); - m_wasActive = false; + m_active = false; + return ActiveState::WasActive; } - return m_wasActive; + return ActiveState::Inactive; } void EngineMicrophone::onInputConfigured(const AudioInput& input) { diff --git a/src/engine/channels/enginemicrophone.h b/src/engine/channels/enginemicrophone.h index 282407d3575..789be15cf13 100644 --- a/src/engine/channels/enginemicrophone.h +++ b/src/engine/channels/enginemicrophone.h @@ -22,7 +22,7 @@ class EngineMicrophone : public EngineChannel, public AudioDestination { EffectsManager* pEffectsManager); virtual ~EngineMicrophone(); - bool isActive(); + ActiveState updateActiveState(); // Called by EngineMaster whenever is requesting a new buffer of audio. virtual void process(CSAMPLE* pOutput, const int iBufferSize); @@ -52,6 +52,4 @@ class EngineMicrophone : public EngineChannel, public AudioDestination { private: QScopedPointer m_pInputConfigured; ControlAudioTaperPot* m_pPregain; - - bool m_wasActive; }; diff --git a/src/engine/effects/engineeffectchain.cpp b/src/engine/effects/engineeffectchain.cpp index 66ab71f22d7..dc7f007b073 100644 --- a/src/engine/effects/engineeffectchain.cpp +++ b/src/engine/effects/engineeffectchain.cpp @@ -183,7 +183,14 @@ bool EngineEffectChain::enableForInputChannel(ChannelHandle inputHandle, bool EngineEffectChain::disableForInputChannel(ChannelHandle inputHandle) { auto& outputMap = m_chainStatusForChannelMatrix[inputHandle]; for (auto&& outputChannelStatus : outputMap) { - if (outputChannelStatus.enableState != EffectEnableState::Disabled) { + qDebug() << "EngineEffectChain::disableForInputChannel" << inputHandle + << &outputChannelStatus + << (int)outputChannelStatus.enableState; + if (outputChannelStatus.enableState == EffectEnableState::Enabling) { + // Channel has never been processed and can be disabled immediately + outputChannelStatus.enableState = EffectEnableState::Disabled; + } else if (outputChannelStatus.enableState == EffectEnableState::Enabled) { + // Channel was enabled, fade effect out via Disabling state outputChannelStatus.enableState = EffectEnableState::Disabling; } } @@ -211,7 +218,10 @@ void EngineEffectChain::deleteStatesForInputChannel(const ChannelHandle inputCha // enableForInputChannel(), or disableForInputChannel(). auto& outputMap = m_chainStatusForChannelMatrix[inputChannel]; for (auto&& outputChannelStatus : outputMap) { - outputChannelStatus.enableState = EffectEnableState::Disabled; + qDebug() << "EngineEffectChain::deleteStatesForInputChannel" + << inputChannel << &outputChannelStatus + << (int)outputChannelStatus.enableState; + //DEBUG_ASSERT(outputChannelStatus.enableState == EffectEnableState::Disabled); } for (EngineEffect* pEffect : qAsConst(m_effects)) { if (pEffect != nullptr) { diff --git a/src/engine/enginemaster.cpp b/src/engine/enginemaster.cpp index 31a9dc4c054..c503990cd41 100644 --- a/src/engine/enginemaster.cpp +++ b/src/engine/enginemaster.cpp @@ -291,7 +291,17 @@ void EngineMaster::processChannels(int iBufferSize) { EngineChannel* pChannel = pChannelInfo->m_pChannel; // Skip inactive channels. - if (!pChannel || !pChannel->isActive()) { + VERIFY_OR_DEBUG_ASSERT(pChannel) { + continue; + } + + EngineChannel::ActiveState activeState = pChannel->updateActiveState(); + if (activeState == EngineChannel::ActiveState::Inactive) { + continue; + } + + if (activeState == EngineChannel::ActiveState::WasActive) { + // TODO() Clean up effect buffers. continue; } diff --git a/src/test/enginemastertest.cpp b/src/test/enginemastertest.cpp index d43f6f0ebcf..740ce484e15 100644 --- a/src/test/enginemastertest.cpp +++ b/src/test/enginemastertest.cpp @@ -30,6 +30,7 @@ class EngineChannelMock : public EngineChannel { Q_UNUSED(iBufferSize); } + MOCK_METHOD0(updateActiveState, ActiveState()); MOCK_METHOD0(isActive, bool()); MOCK_CONST_METHOD0(isMasterEnabled, bool()); MOCK_CONST_METHOD0(isPflEnabled, bool()); @@ -64,9 +65,9 @@ TEST_F(EngineMasterTest, SingleChannelOutputWorks) { SampleUtil::fill(pChannelBuffer, 0.1f, MAX_BUFFER_LEN); // Instruct the mock to claim it is active, master and not PFL. - EXPECT_CALL(*pChannel, isActive()) + EXPECT_CALL(*pChannel, updateActiveState()) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(EngineChannel::ActiveState::Active)); EXPECT_CALL(*pChannel, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -101,9 +102,9 @@ TEST_F(EngineMasterTest, SingleChannelPFLOutputWorks) { SampleUtil::fill(pChannelBuffer, 0.1f, MAX_BUFFER_LEN); // Instruct the mock to claim it is active, not master and PFL - EXPECT_CALL(*pChannel, isActive()) + EXPECT_CALL(*pChannel, updateActiveState()) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(EngineChannel::ActiveState::Active)); EXPECT_CALL(*pChannel, isMasterEnabled()) .Times(1) .WillOnce(Return(false)); @@ -144,9 +145,9 @@ TEST_F(EngineMasterTest, TwoChannelOutputWorks) { SampleUtil::fill(pChannel2Buffer, 0.2f, MAX_BUFFER_LEN); // Instruct channel 1 to claim it is active, master and not PFL. - EXPECT_CALL(*pChannel1, isActive()) + EXPECT_CALL(*pChannel1, updateActiveState()) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(EngineChannel::ActiveState::Active)); EXPECT_CALL(*pChannel1, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -155,9 +156,9 @@ TEST_F(EngineMasterTest, TwoChannelOutputWorks) { .WillOnce(Return(false)); // Instruct channel 2 to claim it is active, master and not PFL. - EXPECT_CALL(*pChannel2, isActive()) + EXPECT_CALL(*pChannel2, updateActiveState()) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(EngineChannel::ActiveState::Active)); EXPECT_CALL(*pChannel2, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -201,9 +202,9 @@ TEST_F(EngineMasterTest, TwoChannelPFLOutputWorks) { SampleUtil::fill(pChannel2Buffer, 0.2f, MAX_BUFFER_LEN); // Instruct channel 1 to claim it is active, master and PFL. - EXPECT_CALL(*pChannel1, isActive()) + EXPECT_CALL(*pChannel1, updateActiveState()) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(EngineChannel::ActiveState::Active)); EXPECT_CALL(*pChannel1, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -212,9 +213,9 @@ TEST_F(EngineMasterTest, TwoChannelPFLOutputWorks) { .WillOnce(Return(true)); // Instruct channel 2 to claim it is active, master and PFL. - EXPECT_CALL(*pChannel2, isActive()) + EXPECT_CALL(*pChannel2, updateActiveState()) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(EngineChannel::ActiveState::Active)); EXPECT_CALL(*pChannel2, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -263,9 +264,9 @@ TEST_F(EngineMasterTest, ThreeChannelOutputWorks) { SampleUtil::fill(pChannel3Buffer, 0.3f, MAX_BUFFER_LEN); // Instruct channel 1 to claim it is active, master and not PFL. - EXPECT_CALL(*pChannel1, isActive()) + EXPECT_CALL(*pChannel1, updateActiveState()) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(EngineChannel::ActiveState::Active)); EXPECT_CALL(*pChannel1, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -274,9 +275,9 @@ TEST_F(EngineMasterTest, ThreeChannelOutputWorks) { .WillOnce(Return(false)); // Instruct channel 2 to claim it is active, master and not PFL. - EXPECT_CALL(*pChannel2, isActive()) + EXPECT_CALL(*pChannel2, updateActiveState()) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(EngineChannel::ActiveState::Active)); EXPECT_CALL(*pChannel2, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -285,9 +286,9 @@ TEST_F(EngineMasterTest, ThreeChannelOutputWorks) { .WillOnce(Return(false)); // Instruct channel 3 to claim it is active, master and not PFL. - EXPECT_CALL(*pChannel3, isActive()) + EXPECT_CALL(*pChannel3, updateActiveState()) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(EngineChannel::ActiveState::Active)); EXPECT_CALL(*pChannel3, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -339,9 +340,9 @@ TEST_F(EngineMasterTest, ThreeChannelPFLOutputWorks) { SampleUtil::fill(pChannel3Buffer, 0.3f, MAX_BUFFER_LEN); // Instruct channel 1 to claim it is active, master and not PFL. - EXPECT_CALL(*pChannel1, isActive()) + EXPECT_CALL(*pChannel1, updateActiveState()) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(EngineChannel::ActiveState::Active)); EXPECT_CALL(*pChannel1, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -350,9 +351,9 @@ TEST_F(EngineMasterTest, ThreeChannelPFLOutputWorks) { .WillOnce(Return(true)); // Instruct channel 2 to claim it is active, master and not PFL. - EXPECT_CALL(*pChannel2, isActive()) + EXPECT_CALL(*pChannel2, updateActiveState()) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(EngineChannel::ActiveState::Active)); EXPECT_CALL(*pChannel2, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -361,9 +362,9 @@ TEST_F(EngineMasterTest, ThreeChannelPFLOutputWorks) { .WillOnce(Return(true)); // Instruct channel 3 to claim it is active, master and not PFL. - EXPECT_CALL(*pChannel3, isActive()) + EXPECT_CALL(*pChannel3, updateActiveState()) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(EngineChannel::ActiveState::Active)); EXPECT_CALL(*pChannel3, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); From dff90d2c295f95c4a368bac32060707d728136f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 26 Mar 2022 23:55:48 +0100 Subject: [PATCH 11/21] process one more buffer after eject --- src/engine/cachingreader/cachingreader.cpp | 4 ++- src/engine/enginebuffer.cpp | 31 ++++++++++++---------- src/engine/enginemaster.cpp | 5 ---- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/engine/cachingreader/cachingreader.cpp b/src/engine/cachingreader/cachingreader.cpp index 2402a226376..85415a0ec07 100644 --- a/src/engine/cachingreader/cachingreader.cpp +++ b/src/engine/cachingreader/cachingreader.cpp @@ -282,7 +282,9 @@ void CachingReader::process() { // track is already loading! In this case the TRACK_LOADED will // be the very next status update. if (!m_state.testAndSetRelease(STATE_TRACK_UNLOADING, STATE_IDLE)) { - DEBUG_ASSERT(atomicLoadRelaxed(m_state) == STATE_TRACK_LOADING); + DEBUG_ASSERT( + atomicLoadRelaxed(m_state) == STATE_TRACK_LOADING || + atomicLoadRelaxed(m_state) == STATE_IDLE); } } } diff --git a/src/engine/enginebuffer.cpp b/src/engine/enginebuffer.cpp index 34bbf41ee61..0b2255f7597 100644 --- a/src/engine/enginebuffer.cpp +++ b/src/engine/enginebuffer.cpp @@ -1000,18 +1000,23 @@ void EngineBuffer::processTrackLocked( rate = m_rate_old; } - const mixxx::audio::FramePos trackEndPosition = getTrackEndPosition(); - bool atEnd = m_playPosition >= trackEndPosition; - bool backwards = rate < 0; - bool bCurBufferPaused = false; - if (atEnd && !backwards) { - // do not play past end - bCurBufferPaused = true; - } else if (rate == 0 && !is_scratching) { - // do not process samples if have no transport - // the linear scaler supports ramping down to 0 - // this is used for pause by scratching only + bool atEnd = false; + bool backwards = rate < 0; + const mixxx::audio::FramePos trackEndPosition = getTrackEndPosition(); + if (trackEndPosition.isValid()) { + atEnd = m_playPosition >= trackEndPosition; + if (atEnd && !backwards) { + // do not play past end + bCurBufferPaused = true; + } else if (rate == 0 && !is_scratching) { + // do not process samples if have no transport + // the linear scaler supports ramping down to 0 + // this is used for pause by scratching only + bCurBufferPaused = true; + } + } else { + // Track has already been ejected. bCurBufferPaused = true; } @@ -1072,12 +1077,10 @@ void EngineBuffer::processTrackLocked( // Handle repeat mode const bool atStart = m_playPosition <= mixxx::audio::kStartFramePos; - atEnd = m_playPosition >= trackEndPosition; bool repeat_enabled = m_pRepeat->toBool(); - bool end_of_track = //(at_start && backwards) || - (atEnd && !backwards); + bool end_of_track = atEnd && !backwards; // If playbutton is pressed, check if we are at start or end of track if ((m_playButton->toBool() || (m_fwdButton->toBool() || m_backButton->toBool())) diff --git a/src/engine/enginemaster.cpp b/src/engine/enginemaster.cpp index c503990cd41..542ba6d25bf 100644 --- a/src/engine/enginemaster.cpp +++ b/src/engine/enginemaster.cpp @@ -300,11 +300,6 @@ void EngineMaster::processChannels(int iBufferSize) { continue; } - if (activeState == EngineChannel::ActiveState::WasActive) { - // TODO() Clean up effect buffers. - continue; - } - if (pChannel->isTalkoverEnabled() && !pChannelInfo->m_pMuteControl->toBool()) { // talkover is an exclusive channel From bf1af79be73a82736c62323bad678f1717dcdb8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Mon, 28 Mar 2022 21:57:41 +0200 Subject: [PATCH 12/21] Use the fadeout flag to fade down and initialize the effect buffers when a stream is paused. Fixes lp1966607 --- src/effects/backends/effectprocessor.h | 10 +++-- src/engine/channelmixer.cpp | 16 ++++++-- src/engine/channels/enginechannel.h | 2 +- src/engine/effects/engineeffectchain.cpp | 15 ++++++- src/engine/effects/engineeffectchain.h | 3 +- src/engine/effects/engineeffectsmanager.cpp | 43 ++++++++++++--------- src/engine/effects/engineeffectsmanager.h | 31 ++++++++------- src/engine/enginemaster.cpp | 25 +++++++++--- src/test/enginemastertest.cpp | 36 +++++++++++++++++ 9 files changed, 133 insertions(+), 48 deletions(-) diff --git a/src/effects/backends/effectprocessor.h b/src/effects/backends/effectprocessor.h index add23a3f964..cd49897620d 100644 --- a/src/effects/backends/effectprocessor.h +++ b/src/effects/backends/effectprocessor.h @@ -194,9 +194,11 @@ class EffectProcessorImpl : public EffectProcessor { outputChannelMap.insert(outputChannel.handle(), createSpecificState(engineParameters)); if (kEffectDebugOutput) { - qDebug() << this << "EffectProcessorImpl::initialize " - "registering output" - << outputChannel << outputChannelMap[outputChannel.handle()]; + qDebug() << this + << "EffectProcessorImpl::initialize " + "registering output" + << outputChannel << outputChannel.handle() + << outputChannelMap[outputChannel.handle()]; } } m_channelStateMatrix.insert(inputChannel.handle(), outputChannelMap); @@ -286,7 +288,7 @@ class EffectProcessorImpl : public EffectProcessor { } if (kEffectDebugOutput) { qDebug() << "EffectProcessorImpl::deleteStatesForInputChannel" - << this << "deleting state" << pState; + << inputChannel << this << "deleting state" << pState; } delete pState; } diff --git a/src/engine/channelmixer.cpp b/src/engine/channelmixer.cpp index 9a76a2c5a48..664e87e24e7 100644 --- a/src/engine/channelmixer.cpp +++ b/src/engine/channelmixer.cpp @@ -27,7 +27,10 @@ void ChannelMixer::applyEffectsAndMixChannels(const EngineMaster::GainCalculator EngineMaster::GainCache& gainCache = (*channelGainCache)[pChannelInfo->m_index]; CSAMPLE_GAIN oldGain = gainCache.m_gain; CSAMPLE_GAIN newGain; - if (gainCache.m_fadeout) { + bool fadeout = gainCache.m_fadeout || + (pChannelInfo->m_pChannel && + !pChannelInfo->m_pChannel->isActive()); + if (fadeout) { newGain = 0; gainCache.m_fadeout = false; } else { @@ -42,7 +45,8 @@ void ChannelMixer::applyEffectsAndMixChannels(const EngineMaster::GainCalculator iSampleRate, pChannelInfo->m_features, oldGain, - newGain); + newGain, + fadeout); } } @@ -69,7 +73,10 @@ void ChannelMixer::applyEffectsInPlaceAndMixChannels( EngineMaster::GainCache& gainCache = (*channelGainCache)[pChannelInfo->m_index]; CSAMPLE_GAIN oldGain = gainCache.m_gain; CSAMPLE_GAIN newGain; - if (gainCache.m_fadeout) { + bool fadeout = gainCache.m_fadeout || + (pChannelInfo->m_pChannel && + !pChannelInfo->m_pChannel->isActive()); + if (fadeout) { newGain = 0; gainCache.m_fadeout = false; } else { @@ -83,7 +90,8 @@ void ChannelMixer::applyEffectsInPlaceAndMixChannels( iSampleRate, pChannelInfo->m_features, oldGain, - newGain); + newGain, + fadeout); SampleUtil::add(pOutput, pChannelInfo->m_pBuffer, iBufferSize); } } diff --git a/src/engine/channels/enginechannel.h b/src/engine/channels/enginechannel.h index 6f17da014c1..6ea7c910406 100644 --- a/src/engine/channels/enginechannel.h +++ b/src/engine/channels/enginechannel.h @@ -45,7 +45,7 @@ class EngineChannel : public EngineObject { } virtual ActiveState updateActiveState() = 0; - bool isActive() { + virtual bool isActive() { return m_active; } diff --git a/src/engine/effects/engineeffectchain.cpp b/src/engine/effects/engineeffectchain.cpp index dc7f007b073..7277dc7f8f0 100644 --- a/src/engine/effects/engineeffectchain.cpp +++ b/src/engine/effects/engineeffectchain.cpp @@ -236,7 +236,8 @@ bool EngineEffectChain::process(const ChannelHandle& inputHandle, CSAMPLE* pOut, const unsigned int numSamples, const unsigned int sampleRate, - const GroupFeatureState& groupFeatures) { + const GroupFeatureState& groupFeatures, + bool fadeout) { // Compute the effective enable state from the channel input routing switch and // the chain's enable state. When either of these are turned on/off, send the // effects the intermediate enabling/disabling signal. @@ -248,6 +249,13 @@ bool EngineEffectChain::process(const ChannelHandle& inputHandle, ChannelStatus& channelStatus = m_chainStatusForChannelMatrix[inputHandle][outputHandle]; EffectEnableState effectiveChainEnableState = channelStatus.enableState; + if (fadeout && channelStatus.enableState == EffectEnableState::Enabled) { + // This is the last callback before pause + // It can start again without further notice + // make use the effect is paused + effectiveChainEnableState = EffectEnableState::Disabling; + } + // If the channel is fully disabled, do not let intermediate // enabling/disabing signals from the chain's enable switch override // the channel's state. @@ -358,6 +366,11 @@ bool EngineEffectChain::process(const ChannelHandle& inputHandle, channelStatus.enableState = EffectEnableState::Enabled; } + if (fadeout && channelStatus.enableState == EffectEnableState::Enabled) { + // Effect is paused now, ramp up next callback which may happen later + channelStatus.enableState = EffectEnableState::Enabling; + } + if (m_enableState == EffectEnableState::Disabling) { m_enableState = EffectEnableState::Disabled; } else if (m_enableState == EffectEnableState::Enabling) { diff --git a/src/engine/effects/engineeffectchain.h b/src/engine/effects/engineeffectchain.h index 7effa29f897..ed5ea4469a3 100644 --- a/src/engine/effects/engineeffectchain.h +++ b/src/engine/effects/engineeffectchain.h @@ -42,7 +42,8 @@ class EngineEffectChain final : public EffectsRequestHandler { CSAMPLE* pOut, const unsigned int numSamples, const unsigned int sampleRate, - const GroupFeatureState& groupFeatures); + const GroupFeatureState& groupFeatures, + bool fadeout); /// called from main thread void deleteStatesForInputChannel(const ChannelHandle channel); diff --git a/src/engine/effects/engineeffectsmanager.cpp b/src/engine/effects/engineeffectsmanager.cpp index 9b5590e4eca..8f65bc228f3 100644 --- a/src/engine/effects/engineeffectsmanager.cpp +++ b/src/engine/effects/engineeffectsmanager.cpp @@ -97,8 +97,8 @@ void EngineEffectsManager::onCallbackStart() { void EngineEffectsManager::processPreFaderInPlace(const ChannelHandle& inputHandle, const ChannelHandle& outputHandle, CSAMPLE* pInOut, - const unsigned int numSamples, - const unsigned int sampleRate) { + unsigned int numSamples, + unsigned int sampleRate) { // Feature state is gathered after prefader effects processing. // This is okay because the equalizer effects do not make use of it. GroupFeatureState featureState; @@ -116,11 +116,12 @@ void EngineEffectsManager::processPostFaderInPlace( const ChannelHandle& inputHandle, const ChannelHandle& outputHandle, CSAMPLE* pInOut, - const unsigned int numSamples, - const unsigned int sampleRate, + unsigned int numSamples, + unsigned int sampleRate, const GroupFeatureState& groupFeatures, - const CSAMPLE_GAIN oldGain, - const CSAMPLE_GAIN newGain) { + CSAMPLE_GAIN oldGain, + CSAMPLE_GAIN newGain, + bool fadeout) { processInner(SignalProcessingStage::Postfader, inputHandle, outputHandle, @@ -130,7 +131,8 @@ void EngineEffectsManager::processPostFaderInPlace( sampleRate, groupFeatures, oldGain, - newGain); + newGain, + fadeout); } void EngineEffectsManager::processPostFaderAndMix( @@ -138,11 +140,12 @@ void EngineEffectsManager::processPostFaderAndMix( const ChannelHandle& outputHandle, CSAMPLE* pIn, CSAMPLE* pOut, - const unsigned int numSamples, - const unsigned int sampleRate, + unsigned int numSamples, + unsigned int sampleRate, const GroupFeatureState& groupFeatures, - const CSAMPLE_GAIN oldGain, - const CSAMPLE_GAIN newGain) { + CSAMPLE_GAIN oldGain, + CSAMPLE_GAIN newGain, + bool fadeout) { processInner(SignalProcessingStage::Postfader, inputHandle, outputHandle, @@ -152,7 +155,8 @@ void EngineEffectsManager::processPostFaderAndMix( sampleRate, groupFeatures, oldGain, - newGain); + newGain, + fadeout); } void EngineEffectsManager::processInner( @@ -161,11 +165,12 @@ void EngineEffectsManager::processInner( const ChannelHandle& outputHandle, CSAMPLE* pIn, CSAMPLE* pOut, - const unsigned int numSamples, - const unsigned int sampleRate, + unsigned int numSamples, + unsigned int sampleRate, const GroupFeatureState& groupFeatures, - const CSAMPLE_GAIN oldGain, - const CSAMPLE_GAIN newGain) { + CSAMPLE_GAIN oldGain, + CSAMPLE_GAIN newGain, + bool fadeout) { const QList& chains = m_chainsByStage.value(stage); if (pIn == pOut) { @@ -180,7 +185,8 @@ void EngineEffectsManager::processInner( pOut, numSamples, sampleRate, - groupFeatures)) { + groupFeatures, + fadeout)) { } } } @@ -217,7 +223,8 @@ void EngineEffectsManager::processInner( pIntermediateOutput, numSamples, sampleRate, - groupFeatures)) { + groupFeatures, + fadeout)) { // Output of this chain becomes the input of the next chain. pIntermediateInput = pIntermediateOutput; } diff --git a/src/engine/effects/engineeffectsmanager.h b/src/engine/effects/engineeffectsmanager.h index 9eacda043c6..287c691b717 100644 --- a/src/engine/effects/engineeffectsmanager.h +++ b/src/engine/effects/engineeffectsmanager.h @@ -33,8 +33,8 @@ class EngineEffectsManager final : public EffectsRequestHandler { const ChannelHandle& inputHandle, const ChannelHandle& outputHandle, CSAMPLE* pInOut, - const unsigned int numSamples, - const unsigned int sampleRate); + unsigned int numSamples, + unsigned int sampleRate); /// Process the postfader EngineEffectChains on the pInOut buffer, modifying /// the contents of the input buffer. @@ -42,11 +42,12 @@ class EngineEffectsManager final : public EffectsRequestHandler { const ChannelHandle& inputHandle, const ChannelHandle& outputHandle, CSAMPLE* pInOut, - const unsigned int numSamples, - const unsigned int sampleRate, + unsigned int numSamples, + unsigned int sampleRate, const GroupFeatureState& groupFeatures, - const CSAMPLE_GAIN oldGain = CSAMPLE_GAIN_ONE, - const CSAMPLE_GAIN newGain = CSAMPLE_GAIN_ONE); + CSAMPLE_GAIN oldGain = CSAMPLE_GAIN_ONE, + CSAMPLE_GAIN newGain = CSAMPLE_GAIN_ONE, + bool fadeout = false); /// Process the postfader EngineEffectChains, leaving the pIn buffer unmodified /// and mixing the output into the pOut buffer. Using EngineEffectsManager's @@ -58,11 +59,12 @@ class EngineEffectsManager final : public EffectsRequestHandler { const ChannelHandle& outputHandle, CSAMPLE* pIn, CSAMPLE* pOut, - const unsigned int numSamples, - const unsigned int sampleRate, + unsigned int numSamples, + unsigned int sampleRate, const GroupFeatureState& groupFeatures, - const CSAMPLE_GAIN oldGain = CSAMPLE_GAIN_ONE, - const CSAMPLE_GAIN newGain = CSAMPLE_GAIN_ONE); + CSAMPLE_GAIN oldGain = CSAMPLE_GAIN_ONE, + CSAMPLE_GAIN newGain = CSAMPLE_GAIN_ONE, + bool fadeout = false); bool processEffectsRequest( EffectsRequest& message, @@ -88,11 +90,12 @@ class EngineEffectsManager final : public EffectsRequestHandler { const ChannelHandle& outputHandle, CSAMPLE* pIn, CSAMPLE* pOut, - const unsigned int numSamples, - const unsigned int sampleRate, + unsigned int numSamples, + unsigned int sampleRate, const GroupFeatureState& groupFeatures, - const CSAMPLE_GAIN oldGain = CSAMPLE_GAIN_ONE, - const CSAMPLE_GAIN newGain = CSAMPLE_GAIN_ONE); + CSAMPLE_GAIN oldGain = CSAMPLE_GAIN_ONE, + CSAMPLE_GAIN newGain = CSAMPLE_GAIN_ONE, + bool fadeout = false); QScopedPointer m_pResponsePipe; QHash> m_chainsByStage; diff --git a/src/engine/enginemaster.cpp b/src/engine/enginemaster.cpp index 542ba6d25bf..acfdce15f72 100644 --- a/src/engine/enginemaster.cpp +++ b/src/engine/enginemaster.cpp @@ -487,7 +487,10 @@ void EngineMaster::process(const int iBufferSize) { m_pTalkover, m_iBufferSize, static_cast(m_sampleRate.value()), - busFeatures); + busFeatures, + CSAMPLE_GAIN_ONE, + CSAMPLE_GAIN_ONE, + false); } switch (m_pTalkoverDucking->getMode()) { @@ -542,21 +545,30 @@ void EngineMaster::process(const int iBufferSize) { m_pOutputBusBuffers[EngineChannel::LEFT], m_iBufferSize, static_cast(m_sampleRate.value()), - busFeatures); + busFeatures, + CSAMPLE_GAIN_ONE, + CSAMPLE_GAIN_ONE, + false); m_pEngineEffectsManager->processPostFaderInPlace( m_busCrossfaderCenterHandle.handle(), m_masterHandle.handle(), m_pOutputBusBuffers[EngineChannel::CENTER], m_iBufferSize, static_cast(m_sampleRate.value()), - busFeatures); + busFeatures, + CSAMPLE_GAIN_ONE, + CSAMPLE_GAIN_ONE, + false); m_pEngineEffectsManager->processPostFaderInPlace( m_busCrossfaderRightHandle.handle(), m_masterHandle.handle(), m_pOutputBusBuffers[EngineChannel::RIGHT], m_iBufferSize, static_cast(m_sampleRate.value()), - busFeatures); + busFeatures, + CSAMPLE_GAIN_ONE, + CSAMPLE_GAIN_ONE, + false); } if (masterEnabled) { @@ -787,7 +799,10 @@ void EngineMaster::applyMasterEffects() { m_pMaster, m_iBufferSize, static_cast(m_sampleRate.value()), - masterFeatures); + masterFeatures, + CSAMPLE_GAIN_ONE, + CSAMPLE_GAIN_ONE, + false); } } diff --git a/src/test/enginemastertest.cpp b/src/test/enginemastertest.cpp index 740ce484e15..51bd1bf00b9 100644 --- a/src/test/enginemastertest.cpp +++ b/src/test/enginemastertest.cpp @@ -68,6 +68,9 @@ TEST_F(EngineMasterTest, SingleChannelOutputWorks) { EXPECT_CALL(*pChannel, updateActiveState()) .Times(1) .WillOnce(Return(EngineChannel::ActiveState::Active)); + EXPECT_CALL(*pChannel, isActive()) + .Times(1) + .WillOnce(Return(true)); EXPECT_CALL(*pChannel, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -105,6 +108,9 @@ TEST_F(EngineMasterTest, SingleChannelPFLOutputWorks) { EXPECT_CALL(*pChannel, updateActiveState()) .Times(1) .WillOnce(Return(EngineChannel::ActiveState::Active)); + EXPECT_CALL(*pChannel, isActive()) + .Times(1) + .WillOnce(Return(true)); EXPECT_CALL(*pChannel, isMasterEnabled()) .Times(1) .WillOnce(Return(false)); @@ -148,6 +154,9 @@ TEST_F(EngineMasterTest, TwoChannelOutputWorks) { EXPECT_CALL(*pChannel1, updateActiveState()) .Times(1) .WillOnce(Return(EngineChannel::ActiveState::Active)); + EXPECT_CALL(*pChannel1, isActive()) + .Times(1) + .WillOnce(Return(true)); EXPECT_CALL(*pChannel1, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -159,6 +168,9 @@ TEST_F(EngineMasterTest, TwoChannelOutputWorks) { EXPECT_CALL(*pChannel2, updateActiveState()) .Times(1) .WillOnce(Return(EngineChannel::ActiveState::Active)); + EXPECT_CALL(*pChannel2, isActive()) + .Times(1) + .WillOnce(Return(true)); EXPECT_CALL(*pChannel2, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -205,6 +217,9 @@ TEST_F(EngineMasterTest, TwoChannelPFLOutputWorks) { EXPECT_CALL(*pChannel1, updateActiveState()) .Times(1) .WillOnce(Return(EngineChannel::ActiveState::Active)); + EXPECT_CALL(*pChannel1, isActive()) + .Times(2) + .WillRepeatedly(Return(true)); EXPECT_CALL(*pChannel1, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -216,6 +231,9 @@ TEST_F(EngineMasterTest, TwoChannelPFLOutputWorks) { EXPECT_CALL(*pChannel2, updateActiveState()) .Times(1) .WillOnce(Return(EngineChannel::ActiveState::Active)); + EXPECT_CALL(*pChannel2, isActive()) + .Times(2) + .WillRepeatedly(Return(true)); EXPECT_CALL(*pChannel2, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -267,6 +285,9 @@ TEST_F(EngineMasterTest, ThreeChannelOutputWorks) { EXPECT_CALL(*pChannel1, updateActiveState()) .Times(1) .WillOnce(Return(EngineChannel::ActiveState::Active)); + EXPECT_CALL(*pChannel1, isActive()) + .Times(1) + .WillOnce(Return(true)); EXPECT_CALL(*pChannel1, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -278,6 +299,9 @@ TEST_F(EngineMasterTest, ThreeChannelOutputWorks) { EXPECT_CALL(*pChannel2, updateActiveState()) .Times(1) .WillOnce(Return(EngineChannel::ActiveState::Active)); + EXPECT_CALL(*pChannel2, isActive()) + .Times(1) + .WillOnce(Return(true)); EXPECT_CALL(*pChannel2, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -289,6 +313,9 @@ TEST_F(EngineMasterTest, ThreeChannelOutputWorks) { EXPECT_CALL(*pChannel3, updateActiveState()) .Times(1) .WillOnce(Return(EngineChannel::ActiveState::Active)); + EXPECT_CALL(*pChannel3, isActive()) + .Times(1) + .WillOnce(Return(true)); EXPECT_CALL(*pChannel3, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -343,6 +370,9 @@ TEST_F(EngineMasterTest, ThreeChannelPFLOutputWorks) { EXPECT_CALL(*pChannel1, updateActiveState()) .Times(1) .WillOnce(Return(EngineChannel::ActiveState::Active)); + EXPECT_CALL(*pChannel1, isActive()) + .Times(2) + .WillRepeatedly(Return(true)); EXPECT_CALL(*pChannel1, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -354,6 +384,9 @@ TEST_F(EngineMasterTest, ThreeChannelPFLOutputWorks) { EXPECT_CALL(*pChannel2, updateActiveState()) .Times(1) .WillOnce(Return(EngineChannel::ActiveState::Active)); + EXPECT_CALL(*pChannel2, isActive()) + .Times(2) + .WillRepeatedly(Return(true)); EXPECT_CALL(*pChannel2, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); @@ -365,6 +398,9 @@ TEST_F(EngineMasterTest, ThreeChannelPFLOutputWorks) { EXPECT_CALL(*pChannel3, updateActiveState()) .Times(1) .WillOnce(Return(EngineChannel::ActiveState::Active)); + EXPECT_CALL(*pChannel3, isActive()) + .Times(2) + .WillRepeatedly(Return(true)); EXPECT_CALL(*pChannel3, isMasterEnabled()) .Times(1) .WillOnce(Return(true)); From 313d4ab7a8a7b04050ad09f7f974e3ce3be6dcca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Tue, 29 Mar 2022 08:53:17 +0200 Subject: [PATCH 13/21] EffectProcessor: split out initalizeChannelHandle --- src/effects/backends/effectprocessor.h | 42 +++++++++++++++----------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/src/effects/backends/effectprocessor.h b/src/effects/backends/effectprocessor.h index cd49897620d..6c4681bc857 100644 --- a/src/effects/backends/effectprocessor.h +++ b/src/effects/backends/effectprocessor.h @@ -71,6 +71,9 @@ class EffectProcessor { const QSet& activeInputChannels, const QSet& registeredOutputChannels, const mixxx::EngineParameters& engineParameters) = 0; + virtual void initializeInputChannel( + ChannelHandle inputChannel, + const mixxx::EngineParameters& engineParameters) = 0; virtual void loadEngineEffectParameters( const QMap& parameters) = 0; virtual EffectState* createState(const mixxx::EngineParameters& engineParameters) = 0; @@ -183,26 +186,31 @@ class EffectProcessorImpl : public EffectProcessor { m_registeredOutputChannels = registeredOutputChannels; for (const ChannelHandleAndGroup& inputChannel : activeInputChannels) { + initializeInputChannel(inputChannel.handle(), engineParameters); + } + }; + + void initializeInputChannel(ChannelHandle inputChannel, + const mixxx::EngineParameters& engineParameters) final { + if (kEffectDebugOutput) { + qDebug() << this << "EffectProcessorImpl::initialize allocating " + "EffectStates for input" + << inputChannel; + } + ChannelHandleMap outputChannelMap; + for (const ChannelHandleAndGroup& outputChannel : + std::as_const(m_registeredOutputChannels)) { + outputChannelMap.insert(outputChannel.handle(), + createSpecificState(engineParameters)); if (kEffectDebugOutput) { - qDebug() << this << "EffectProcessorImpl::initialize allocating " - "EffectStates for input" - << inputChannel; - } - ChannelHandleMap 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 << outputChannel.handle() - << outputChannelMap[outputChannel.handle()]; - } + qDebug() << this + << "EffectProcessorImpl::initialize " + "registering output" + << outputChannel << outputChannel.handle() + << outputChannelMap[outputChannel.handle()]; } - m_channelStateMatrix.insert(inputChannel.handle(), outputChannelMap); } + m_channelStateMatrix.insert(inputChannel, outputChannelMap); }; EffectState* createState(const mixxx::EngineParameters& engineParameters) final { From c4b081a37f38e88c6bc9b894ecd1b58817a0ef8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Tue, 29 Mar 2022 13:14:25 +0200 Subject: [PATCH 14/21] Don't delete effect state object when chain is disabled. From the user perspective it is no difference between disabling an effect or a chain. --- src/effects/backends/effectprocessor.h | 107 ++--------------------- src/effects/effectchain.cpp | 20 +---- src/effects/effectslot.cpp | 23 +---- src/effects/effectslot.h | 2 +- src/effects/effectsmessenger.cpp | 8 -- src/engine/channelhandle.h | 4 + src/engine/effects/engineeffect.cpp | 24 ++--- src/engine/effects/engineeffect.h | 10 +-- src/engine/effects/engineeffectchain.cpp | 65 +------------- src/engine/effects/engineeffectchain.h | 6 +- src/engine/effects/message.h | 15 ---- 11 files changed, 36 insertions(+), 248 deletions(-) diff --git a/src/effects/backends/effectprocessor.h b/src/effects/backends/effectprocessor.h index 6c4681bc857..80fd6a0904c 100644 --- a/src/effects/backends/effectprocessor.h +++ b/src/effects/backends/effectprocessor.h @@ -76,12 +76,7 @@ class EffectProcessor { const mixxx::EngineParameters& engineParameters) = 0; virtual void loadEngineEffectParameters( const QMap& 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 @@ -160,11 +155,6 @@ class EffectProcessorImpl : public EffectProcessor { const EffectEnableState enableState, const GroupFeatureState& groupFeatures) final { EffectSpecificState* pState = m_channelStateMatrix[inputHandle][outputHandle]; - // TODO: The state can be null if we are in the deleteStatesForInputChannel() loop. - // A protection against this is missing. Since it can happen the assertion is incorrect - // The memory will be leaked. - // Idea: Skip the processing here? - // Probably related: https://bugs.launchpad.net/mixxx/+bug/1775497 VERIFY_OR_DEBUG_ASSERT(pState != nullptr) { if (kEffectDebugOutput) { qWarning() << "EffectProcessorImpl::process could not retrieve" @@ -213,95 +203,16 @@ class EffectProcessorImpl : public EffectProcessor { m_channelStateMatrix.insert(inputChannel, outputChannelMap); }; - EffectState* createState(const mixxx::EngineParameters& engineParameters) final { - return createSpecificState(engineParameters); - }; - - bool loadStatesForInputChannel(ChannelHandle inputChannel, - const EffectStatesMap* pStatesMap) 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& 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; - } - } - - QSet receivedOutputChannels = m_registeredOutputChannels; - for (const ChannelHandleAndGroup& outputChannel : - std::as_const(m_registeredOutputChannels)) { - if (kEffectDebugOutput) { - qDebug() << "EffectProcessorImpl::loadStatesForInputChannel" - << this << "output" << outputChannel; - } - - auto pState = dynamic_cast( - 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; - }; - - /// 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. - - // TODO: How is ensure that the statMap is not accessed during this loop? - // This is called in responds to DISABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL - // and it looks like that the objects pointed by m_channelStateMatrix - // may be still in use. - // Probably related: https://bugs.launchpad.net/mixxx/+bug/1775497 - ChannelHandleMap& stateMap = - m_channelStateMatrix[inputChannel]; - for (EffectSpecificState* pState : stateMap) { - VERIFY_OR_DEBUG_ASSERT(pState != nullptr) { - continue; - } - if (kEffectDebugOutput) { - qDebug() << "EffectProcessorImpl::deleteStatesForInputChannel" - << inputChannel << 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 diff --git a/src/effects/effectchain.cpp b/src/effects/effectchain.cpp index 14e052d1ab5..16edcaaf1ce 100644 --- a/src/effects/effectchain.cpp +++ b/src/effects/effectchain.cpp @@ -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); diff --git a/src/effects/effectslot.cpp b/src/effects/effectslot.cpp index 8d6083d9d55..d25f4a20027 100644 --- a/src/effects/effectslot.cpp +++ b/src/effects/effectslot.cpp @@ -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 { diff --git a/src/effects/effectslot.h b/src/effects/effectslot.h index 9fc0a5a6bce..ce2f3175eab 100644 --- a/src/effects/effectslot.h +++ b/src/effects/effectslot.h @@ -123,7 +123,7 @@ class EffectSlot : public QObject { return m_group; } - void fillEffectStatesMap(EffectStatesMap* pStatesMap) const; + void initalizeInputChannel(ChannelHandle inputChannel); EffectManifestPointer getManifest() const; diff --git a/src/effects/effectsmessenger.cpp b/src/effects/effectsmessenger.cpp index 2eec71ef615..200278be754 100644 --- a/src/effects/effectsmessenger.cpp +++ b/src/effects/effectsmessenger.cpp @@ -90,13 +90,5 @@ void EffectsMessenger::collectGarbage(const EffectsRequest* pRequest) { qDebug() << debugString() << "delete" << pRequest->RemoveEffectChain.pChain; } delete pRequest->RemoveEffectChain.pChain; - } else if (pRequest->type == EffectsRequest::DISABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL) { - if (kEffectDebugOutput) { - qDebug() << debugString() << "deleting states for input channel" - << pRequest->DisableInputChannelForChain.channelHandle - << "for EngineEffectChain" << pRequest->pTargetChain; - } - pRequest->pTargetChain->deleteStatesForInputChannel( - pRequest->DisableInputChannelForChain.channelHandle); } } diff --git a/src/engine/channelhandle.h b/src/engine/channelhandle.h index e8335c1fc3e..76662259635 100644 --- a/src/engine/channelhandle.h +++ b/src/engine/channelhandle.h @@ -207,6 +207,10 @@ class ChannelHandleMap { m_data.clear(); } + int size() const { + return m_data.size(); + } + bool isEmpty() { return m_data.isEmpty(); } diff --git a/src/engine/effects/engineeffect.cpp b/src/engine/effects/engineeffect.cpp index 20e9b30e370..2f9d2190b35 100644 --- a/src/engine/effects/engineeffect.cpp +++ b/src/engine/effects/engineeffect.cpp @@ -46,24 +46,16 @@ EngineEffect::~EngineEffect() { m_parameters.clear(); } -EffectState* EngineEffect::createState(const mixxx::EngineParameters& engineParameters) { - VERIFY_OR_DEBUG_ASSERT(m_pProcessor) { - return new EffectState(engineParameters); +void EngineEffect::initalizeInputChannel(ChannelHandle inputChannel) { + if (m_pProcessor->hasStatesForInputChannel(inputChannel)) { + // already initialized for this input channel } - return m_pProcessor->createState(engineParameters); -} - -void EngineEffect::loadStatesForInputChannel(ChannelHandle inputChannel, - EffectStatesMap* pStatesMap) { - if (kEffectDebugOutput) { - qDebug() << "EngineEffect::loadStatesForInputChannel" << this - << "loading states for input" << inputChannel; - } - m_pProcessor->loadStatesForInputChannel(inputChannel, pStatesMap); -} -void EngineEffect::deleteStatesForInputChannel(ChannelHandle inputChannel) { - m_pProcessor->deleteStatesForInputChannel(inputChannel); + //TODO: get actual configuration of engine + const mixxx::EngineParameters engineParameters( + mixxx::audio::SampleRate(96000), + MAX_BUFFER_LEN / mixxx::kEngineChannelCount); + m_pProcessor->initializeInputChannel(inputChannel, engineParameters); } bool EngineEffect::processEffectsRequest(EffectsRequest& message, diff --git a/src/engine/effects/engineeffect.h b/src/engine/effects/engineeffect.h index ea83a6fbc15..7c46105e852 100644 --- a/src/engine/effects/engineeffect.h +++ b/src/engine/effects/engineeffect.h @@ -31,14 +31,8 @@ class EngineEffect final : public EffectsRequestHandler { /// Called in main thread by EffectSlot ~EngineEffect(); - /// Called in main thread to allocate an EffectState - EffectState* createState(const mixxx::EngineParameters& engineParameters); - - /// Called in audio thread to load EffectStates received from the main thread - void loadStatesForInputChannel(ChannelHandle inputChannel, - EffectStatesMap* pStatesMap); - /// Called from the main thread for garbage collection after an input channel is disabled - void deleteStatesForInputChannel(ChannelHandle inputChannel); + /// Called from the main thread to make sure that the channel already has states + void initalizeInputChannel(ChannelHandle inputChannel); /// Called in audio thread bool processEffectsRequest( diff --git a/src/engine/effects/engineeffectchain.cpp b/src/engine/effects/engineeffectchain.cpp index 7277dc7f8f0..d4f868ef1d6 100644 --- a/src/engine/effects/engineeffectchain.cpp +++ b/src/engine/effects/engineeffectchain.cpp @@ -126,8 +126,7 @@ bool EngineEffectChain::processEffectsRequest(EffectsRequest& message, << message.EnableInputChannelForChain.channelHandle; } response.success = enableForInputChannel( - message.EnableInputChannelForChain.channelHandle, - message.EnableInputChannelForChain.pEffectStatesMapArray); + message.EnableInputChannelForChain.channelHandle); break; case EffectsRequest::DISABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL: if (kEffectDebugOutput) { @@ -146,46 +145,21 @@ bool EngineEffectChain::processEffectsRequest(EffectsRequest& message, return true; } -bool EngineEffectChain::enableForInputChannel(ChannelHandle inputHandle, - EffectStatesMapArray* statesForEffectsInChain) { +bool EngineEffectChain::enableForInputChannel(ChannelHandle inputHandle) { if (kEffectDebugOutput) { qDebug() << "EngineEffectChain::enableForInputChannel" << this << inputHandle; } auto& outputMap = m_chainStatusForChannelMatrix[inputHandle]; for (auto&& outputChannelStatus : outputMap) { - VERIFY_OR_DEBUG_ASSERT(outputChannelStatus.enableState != - EffectEnableState::Enabled) { - for (auto&& pStatesMap : *statesForEffectsInChain) { - for (auto&& pState : pStatesMap) { - delete pState; - } - } - return false; - } + DEBUG_ASSERT(outputChannelStatus.enableState != EffectEnableState::Enabled); outputChannelStatus.enableState = EffectEnableState::Enabling; } - for (int i = 0; i < m_effects.size(); ++i) { - if (m_effects[i] != nullptr) { - if (kEffectDebugOutput) { - qDebug() << "EngineEffectChain::enableForInputChannel" << this - << "loading states for effect" << i; - } - EffectStatesMap* pStatesMap = &(*statesForEffectsInChain)[i]; - VERIFY_OR_DEBUG_ASSERT(pStatesMap) { - return false; - } - m_effects[i]->loadStatesForInputChannel(inputHandle, pStatesMap); - } - } return true; } bool EngineEffectChain::disableForInputChannel(ChannelHandle inputHandle) { auto& outputMap = m_chainStatusForChannelMatrix[inputHandle]; for (auto&& outputChannelStatus : outputMap) { - qDebug() << "EngineEffectChain::disableForInputChannel" << inputHandle - << &outputChannelStatus - << (int)outputChannelStatus.enableState; if (outputChannelStatus.enableState == EffectEnableState::Enabling) { // Channel has never been processed and can be disabled immediately outputChannelStatus.enableState = EffectEnableState::Disabled; @@ -194,42 +168,9 @@ bool EngineEffectChain::disableForInputChannel(ChannelHandle inputHandle) { outputChannelStatus.enableState = EffectEnableState::Disabling; } } - // Do not call deleteStatesForInputChannel here because the EngineEffects' - // process() method needs to run one last time before deleting the states. - // deleteStatesForInputChannel needs to be called from the main thread after - // the successful EffectsResponse is returned by the MessagePipe FIFO. return true; } -// Called from the main thread for garbage collection after an input channel is disabled -void EngineEffectChain::deleteStatesForInputChannel(const ChannelHandle inputChannel) { - // If an output channel is not presently being processed, for example when - // PFL is not active, then process() cannot be relied upon to set this - // chain's EffectEnableState from Disabling to Disabled. This must be done - // before the next time process() is called for that output channel, - // otherwise, if any EngineEffects are Enabled, - // EffectProcessorImpl::processChannel will try to run - // with an EffectState that has already been deleted and cause a crash. - // Refer to https://bugs.launchpad.net/mixxx/+bug/1741213 - // NOTE: ChannelHandleMap is like a map in that it associates an object with - // a ChannelHandle key, but it actually backed by a QVarLengthArray, not a - // QMap. So it is okay that m_chainStatusForChannelMatrix may be - // accessed concurrently in the audio engine thread in process(), - // enableForInputChannel(), or disableForInputChannel(). - auto& outputMap = m_chainStatusForChannelMatrix[inputChannel]; - for (auto&& outputChannelStatus : outputMap) { - qDebug() << "EngineEffectChain::deleteStatesForInputChannel" - << inputChannel << &outputChannelStatus - << (int)outputChannelStatus.enableState; - //DEBUG_ASSERT(outputChannelStatus.enableState == EffectEnableState::Disabled); - } - for (EngineEffect* pEffect : qAsConst(m_effects)) { - if (pEffect != nullptr) { - pEffect->deleteStatesForInputChannel(inputChannel); - } - } -} - bool EngineEffectChain::process(const ChannelHandle& inputHandle, const ChannelHandle& outputHandle, CSAMPLE* pIn, diff --git a/src/engine/effects/engineeffectchain.h b/src/engine/effects/engineeffectchain.h index ed5ea4469a3..36007a0918b 100644 --- a/src/engine/effects/engineeffectchain.h +++ b/src/engine/effects/engineeffectchain.h @@ -45,9 +45,6 @@ class EngineEffectChain final : public EffectsRequestHandler { const GroupFeatureState& groupFeatures, bool fadeout); - /// called from main thread - void deleteStatesForInputChannel(const ChannelHandle channel); - private: struct ChannelStatus { ChannelStatus() @@ -65,8 +62,7 @@ class EngineEffectChain final : public EffectsRequestHandler { bool updateParameters(const EffectsRequest& message); bool addEffect(EngineEffect* pEffect, int iIndex); bool removeEffect(EngineEffect* pEffect, int iIndex); - bool enableForInputChannel(ChannelHandle inputHandle, - EffectStatesMapArray* statesForEffectsInChain); + bool enableForInputChannel(ChannelHandle inputHandle); bool disableForInputChannel(ChannelHandle inputHandle); QString m_group; diff --git a/src/engine/effects/message.h b/src/engine/effects/message.h index 880ea48f623..b78549567aa 100644 --- a/src/engine/effects/message.h +++ b/src/engine/effects/message.h @@ -44,20 +44,6 @@ struct EffectsRequest { pTargetEffect = nullptr; } - // This is called from the main thread by EffectsManager after receiving a - // response from EngineEffectsManager in the audio engine thread. - ~EffectsRequest() { - if (type == ENABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL) { - VERIFY_OR_DEBUG_ASSERT(EnableInputChannelForChain.pEffectStatesMapArray != nullptr) { - return; - } - // This only deletes the container used to passed the EffectStates - // to EffectProcessorImpl. The EffectStates are managed by - // EffectProcessorImpl. - delete EnableInputChannelForChain.pEffectStatesMapArray; - } - } - MessageType type; qint64 request_id; @@ -86,7 +72,6 @@ struct EffectsRequest { SignalProcessingStage signalProcessingStage; } RemoveEffectChain; struct { - EffectStatesMapArray* pEffectStatesMapArray; ChannelHandle channelHandle; } EnableInputChannelForChain; struct { From b4dc4491400ee031c105b3457c5d371166b18a02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 30 Mar 2022 01:35:38 +0200 Subject: [PATCH 15/21] Use override keyword --- src/engine/channels/engineaux.h | 18 ++++++++++-------- src/engine/channels/enginechannel.h | 6 ++---- src/engine/channels/enginedeck.h | 18 +++++++++--------- src/engine/channels/enginemicrophone.h | 20 +++++++++++--------- src/engine/enginebuffer.h | 4 ++-- src/engine/engineobject.h | 4 ++-- 6 files changed, 36 insertions(+), 34 deletions(-) diff --git a/src/engine/channels/engineaux.h b/src/engine/channels/engineaux.h index 5934ca5fec0..72a171035c6 100644 --- a/src/engine/channels/engineaux.h +++ b/src/engine/channels/engineaux.h @@ -16,31 +16,33 @@ class EngineAux : public EngineChannel, public AudioDestination { Q_OBJECT public: EngineAux(const ChannelHandleAndGroup& handleGroup, EffectsManager* pEffectsManager); - virtual ~EngineAux(); + ~EngineAux() override; ActiveState updateActiveState() override; /// Called by EngineMaster whenever is requesting a new buffer of audio. - virtual void process(CSAMPLE* pOutput, const int iBufferSize); - virtual void collectFeatures(GroupFeatureState* pGroupFeatures) const; - virtual void postProcess(const int iBufferSize) { Q_UNUSED(iBufferSize) } + void process(CSAMPLE* pOutput, const int iBufferSize) override; + void collectFeatures(GroupFeatureState* pGroupFeatures) const override; + void postProcess(const int iBufferSize) override { + Q_UNUSED(iBufferSize) + } /// This is called by SoundManager whenever there are new samples from the /// configured input to be processed. This is run in the callback thread of /// the soundcard this AudioDestination was registered for! Beware, in the /// case of multiple soundcards, this method is not re-entrant but it may be /// concurrent with EngineMaster processing. - virtual void receiveBuffer(const AudioInput& input, + void receiveBuffer(const AudioInput& input, const CSAMPLE* pBuffer, - unsigned int nFrames); + unsigned int nFrames) override; /// Called by SoundManager whenever the aux input is connected to a /// soundcard input. - virtual void onInputConfigured(const AudioInput& input); + void onInputConfigured(const AudioInput& input) override; /// Called by SoundManager whenever the aux input is disconnected from /// a soundcard input. - virtual void onInputUnconfigured(const AudioInput& input); + void onInputUnconfigured(const AudioInput& input) override; private: QScopedPointer m_pInputConfigured; diff --git a/src/engine/channels/enginechannel.h b/src/engine/channels/enginechannel.h index 6ea7c910406..9517a98907a 100644 --- a/src/engine/channels/enginechannel.h +++ b/src/engine/channels/enginechannel.h @@ -32,7 +32,7 @@ class EngineChannel : public EngineObject { EffectsManager* pEffectsManager, bool isTalkoverChannel, bool isPrimaryDeck); - virtual ~EngineChannel(); + ~EngineChannel() override; virtual ChannelOrientation getOrientation() const; @@ -66,13 +66,11 @@ class EngineChannel : public EngineObject { m_channelIndex = channelIndex; } - virtual void process(CSAMPLE* pOut, const int iBufferSize) = 0; - virtual void collectFeatures(GroupFeatureState* pGroupFeatures) const = 0; virtual void postProcess(const int iBuffersize) = 0; // TODO(XXX) This hack needs to be removed. virtual EngineBuffer* getEngineBuffer() { - return NULL; + return nullptr; } protected: diff --git a/src/engine/channels/enginedeck.h b/src/engine/channels/enginedeck.h index 50f61ec8622..3bdb13abcb4 100644 --- a/src/engine/channels/enginedeck.h +++ b/src/engine/channels/enginedeck.h @@ -29,14 +29,14 @@ class EngineDeck : public EngineChannel, public AudioDestination { EffectsManager* pEffectsManager, EngineChannel::ChannelOrientation defaultOrientation, bool primaryDeck); - virtual ~EngineDeck(); + ~EngineDeck() override; - virtual void process(CSAMPLE* pOutput, const int iBufferSize); - virtual void collectFeatures(GroupFeatureState* pGroupFeatures) const; - virtual void postProcess(const int iBufferSize); + void process(CSAMPLE* pOutput, const int iBufferSize) override; + void collectFeatures(GroupFeatureState* pGroupFeatures) const override; + void postProcess(const int iBufferSize) override; // TODO(XXX) This hack needs to be removed. - virtual EngineBuffer* getEngineBuffer(); + EngineBuffer* getEngineBuffer() override; EngineChannel::ActiveState updateActiveState() override; @@ -45,17 +45,17 @@ class EngineDeck : public EngineChannel, public AudioDestination { // the soundcard this AudioDestination was registered for! Beware, in the // case of multiple soundcards, this method is not re-entrant but it may be // concurrent with EngineMaster processing. - virtual void receiveBuffer(const AudioInput& input, + void receiveBuffer(const AudioInput& input, const CSAMPLE* pBuffer, - unsigned int nFrames); + unsigned int nFrames) override; // Called by SoundManager whenever the passthrough input is connected to a // soundcard input. - virtual void onInputConfigured(const AudioInput& input); + void onInputConfigured(const AudioInput& input) override; // Called by SoundManager whenever the passthrough input is disconnected // from a soundcard input. - virtual void onInputUnconfigured(const AudioInput& input); + void onInputUnconfigured(const AudioInput& input) override; // Return whether or not passthrough is active bool isPassthroughActive() const; diff --git a/src/engine/channels/enginemicrophone.h b/src/engine/channels/enginemicrophone.h index 789be15cf13..8cd3445d92f 100644 --- a/src/engine/channels/enginemicrophone.h +++ b/src/engine/channels/enginemicrophone.h @@ -20,31 +20,33 @@ class EngineMicrophone : public EngineChannel, public AudioDestination { public: EngineMicrophone(const ChannelHandleAndGroup& handleGroup, EffectsManager* pEffectsManager); - virtual ~EngineMicrophone(); + ~EngineMicrophone() override; - ActiveState updateActiveState(); + ActiveState updateActiveState() override; // Called by EngineMaster whenever is requesting a new buffer of audio. - virtual void process(CSAMPLE* pOutput, const int iBufferSize); - virtual void collectFeatures(GroupFeatureState* pGroupFeatures) const; - virtual void postProcess(const int iBufferSize) { Q_UNUSED(iBufferSize) } + void process(CSAMPLE* pOutput, const int iBufferSize) override; + void collectFeatures(GroupFeatureState* pGroupFeatures) const override; + void postProcess(const int iBufferSize) override { + Q_UNUSED(iBufferSize) + } // This is called by SoundManager whenever there are new samples from the // configured input to be processed. This is run in the callback thread of // the soundcard this AudioDestination was registered for! Beware, in the // case of multiple soundcards, this method is not re-entrant but it may be // concurrent with EngineMaster processing. - virtual void receiveBuffer(const AudioInput& input, + void receiveBuffer(const AudioInput& input, const CSAMPLE* pBuffer, - unsigned int iNumSamples); + unsigned int iNumSamples) override; // Called by SoundManager whenever the microphone input is connected to a // soundcard input. - virtual void onInputConfigured(const AudioInput& input); + void onInputConfigured(const AudioInput& input) override; // Called by SoundManager whenever the microphone input is disconnected from // a soundcard input. - virtual void onInputUnconfigured(const AudioInput& input); + void onInputUnconfigured(const AudioInput& input) override; bool isSolo(); double getSoloDamping(); diff --git a/src/engine/enginebuffer.h b/src/engine/enginebuffer.h index 86d0aabdcc4..f8a2d36ecb2 100644 --- a/src/engine/enginebuffer.h +++ b/src/engine/enginebuffer.h @@ -112,7 +112,7 @@ class EngineBuffer : public EngineObject { void requestClonePosition(EngineChannel* pChannel); // The process methods all run in the audio callback. - void process(CSAMPLE* pOut, const int iBufferSize); + void process(CSAMPLE* pOut, const int iBufferSize) override; void processSlip(int iBufferSize); void postProcess(const int iBufferSize); @@ -132,7 +132,7 @@ class EngineBuffer : public EngineObject { double getRateRatio() const; - void collectFeatures(GroupFeatureState* pGroupFeatures) const; + void collectFeatures(GroupFeatureState* pGroupFeatures) const override; // For dependency injection of scalers. void setScalerForTest( diff --git a/src/engine/engineobject.h b/src/engine/engineobject.h index 88de2c7e492..3c5c4c443b5 100644 --- a/src/engine/engineobject.h +++ b/src/engine/engineobject.h @@ -9,7 +9,7 @@ class EngineObject : public QObject { Q_OBJECT public: EngineObject(); - virtual ~EngineObject(); + ~EngineObject() override; virtual void process(CSAMPLE* pInOut, const int iBufferSize) = 0; @@ -24,7 +24,7 @@ class EngineObjectConstIn : public QObject { Q_OBJECT public: EngineObjectConstIn(); - virtual ~EngineObjectConstIn(); + ~EngineObjectConstIn() override; virtual void process(const CSAMPLE* pIn, CSAMPLE* pOut, const int iBufferSize) = 0; From 2250eaa1397b2aa96c146a8ebb34767c618fd186 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 2 Apr 2022 14:00:07 +0200 Subject: [PATCH 16/21] Manage the lifetime of the effect state by a std::unique_ptr --- src/effects/backends/effectprocessor.h | 62 +++++++++++++------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/src/effects/backends/effectprocessor.h b/src/effects/backends/effectprocessor.h index 80fd6a0904c..d95f8993eb3 100644 --- a/src/effects/backends/effectprocessor.h +++ b/src/effects/backends/effectprocessor.h @@ -10,6 +10,7 @@ #include "engine/effects/groupfeaturestate.h" #include "engine/effects/message.h" #include "engine/engine.h" +#include "util/sample.h" #include "util/types.h" /// Effects are implemented as two separate classes, an EffectState subclass and @@ -110,28 +111,9 @@ class EffectProcessorImpl : public EffectProcessor { /// Subclasses should not implement their own destructor. All state should /// be stored in the EffectState subclass, not the EffectProcessorImpl subclass. ~EffectProcessorImpl() { - if (kEffectDebugOutput) { - qDebug() << "~EffectProcessorImpl" << this; - } - int inputChannelHandleNumber = 0; - for (ChannelHandleMap& outputsMap : m_channelStateMatrix) { - int outputChannelHandleNumber = 0; - for (EffectSpecificState* pState : outputsMap) { - VERIFY_OR_DEBUG_ASSERT(pState != nullptr) { - continue; - } - if (kEffectDebugOutput) { - qDebug() << "~EffectProcessorImpl deleting EffectState" << pState - << "for input ChannelHandle(" << inputChannelHandleNumber << ")" - << "and output ChannelHandle(" << outputChannelHandleNumber << ")"; - } - delete pState; - outputChannelHandleNumber++; - } - outputsMap.clear(); - inputChannelHandleNumber++; - } - m_channelStateMatrix.clear(); + //if (kEffectDebugOutput) { + qDebug() << "~EffectProcessorImpl" << this; + //} }; /// NOTE: Subclasses for Built-In effects must implement the following static methods for @@ -154,7 +136,8 @@ class EffectProcessorImpl : public EffectProcessor { const mixxx::EngineParameters& engineParameters, const EffectEnableState enableState, const GroupFeatureState& groupFeatures) final { - EffectSpecificState* pState = m_channelStateMatrix[inputHandle][outputHandle]; + EffectSpecificState* pState = + m_channelStateMatrix[inputHandle][outputHandle.handle()].get(); VERIFY_OR_DEBUG_ASSERT(pState != nullptr) { if (kEffectDebugOutput) { qWarning() << "EffectProcessorImpl::process could not retrieve" @@ -164,8 +147,7 @@ class EffectProcessorImpl : public EffectProcessor { << "EffectState should have been preallocated in the" "main thread."; } - pState = createSpecificState(engineParameters); - m_channelStateMatrix[inputHandle][outputHandle] = pState; + SampleUtil::copy(pOutput, pInput, engineParameters.samplesPerBuffer()); } processChannel(pState, pInput, pOutput, engineParameters, enableState, groupFeatures); } @@ -187,25 +169,43 @@ class EffectProcessorImpl : public EffectProcessor { "EffectStates for input" << inputChannel; } - ChannelHandleMap outputChannelMap; + + int requiredVectorSize = 0; + // For fast lookups we use a vector with index = handle; + // gaps are filled with nullptr + for (const ChannelHandleAndGroup& outputChannel : + std::as_const(m_registeredOutputChannels)) { + int vectorIndex = outputChannel.handle().handle(); + if (requiredVectorSize <= vectorIndex) { + requiredVectorSize = vectorIndex + 1; + } + } + + DEBUG_ASSERT(requiredVectorSize > 0); + auto& outputChannelStates = m_channelStateMatrix[inputChannel]; + DEBUG_ASSERT(outputChannelStates.size() == 0); + outputChannelStates.reserve(requiredVectorSize); + outputChannelStates.clear(); + for (int i = 0; i < requiredVectorSize; ++i) { + outputChannelStates.push_back(std::unique_ptr()); + } for (const ChannelHandleAndGroup& outputChannel : std::as_const(m_registeredOutputChannels)) { - outputChannelMap.insert(outputChannel.handle(), + outputChannelStates[outputChannel.handle().handle()].reset( createSpecificState(engineParameters)); if (kEffectDebugOutput) { qDebug() << this << "EffectProcessorImpl::initialize " "registering output" << outputChannel << outputChannel.handle() - << outputChannelMap[outputChannel.handle()]; + << outputChannelStates[outputChannel.handle().handle()].get(); } } - m_channelStateMatrix.insert(inputChannel, outputChannelMap); }; bool hasStatesForInputChannel(ChannelHandle inputChannel) const { if (inputChannel.handle() < m_channelStateMatrix.size()) { - for (EffectSpecificState* pState : m_channelStateMatrix.at(inputChannel)) { + for (const auto& pState : m_channelStateMatrix.at(inputChannel)) { if (pState) { return true; } @@ -228,5 +228,5 @@ class EffectProcessorImpl : public EffectProcessor { private: QSet m_registeredOutputChannels; - ChannelHandleMap> m_channelStateMatrix; + ChannelHandleMap>> m_channelStateMatrix; }; From cba84c1cd0e3bf7c1efce035b0caa24ff06f81c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Fri, 22 Jul 2022 06:47:39 +0200 Subject: [PATCH 17/21] Added missing null check in CueControl::hintReader() This is a regression since https://github.com/mixxxdj/mixxx/pull/4771 --- src/engine/controls/cuecontrol.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/engine/controls/cuecontrol.cpp b/src/engine/controls/cuecontrol.cpp index 09c6dd6a4b2..9d5fa90f5b7 100644 --- a/src/engine/controls/cuecontrol.cpp +++ b/src/engine/controls/cuecontrol.cpp @@ -1192,13 +1192,15 @@ void CueControl::hintReader(gsl::not_null pHintList) { appendCueHint(pHintList, pControl->getPosition(), Hint::Type::HotCue); } - CuePointer pAudibleSound = - m_pLoadedTrack->findCueByType(mixxx::CueType::AudibleSound); - if (pAudibleSound) { - const mixxx::audio::FramePos frame = pAudibleSound->getPosition(); - appendCueHint(pHintList, frame, Hint::Type::FirstSound); + TrackPointer pLoadedTrack = m_pLoadedTrack; + if (pLoadedTrack) { + CuePointer pAudibleSound = + pLoadedTrack->findCueByType(mixxx::CueType::AudibleSound); + if (pAudibleSound) { + const mixxx::audio::FramePos frame = pAudibleSound->getPosition(); + appendCueHint(pHintList, frame, Hint::Type::FirstSound); + } } - appendCueHint(pHintList, m_pIntroStartPosition->get(), Hint::Type::IntroStart); appendCueHint(pHintList, m_pIntroEndPosition->get(), Hint::Type::IntroEnd); appendCueHint(pHintList, m_pOutroStartPosition->get(), Hint::Type::OutroStart); From 5d63ac33a5bd4cb74fce61126ac32ac7bd52792a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Fri, 28 Oct 2022 09:35:47 +0200 Subject: [PATCH 18/21] re-enabble debug guard --- src/effects/backends/effectprocessor.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/effects/backends/effectprocessor.h b/src/effects/backends/effectprocessor.h index d95f8993eb3..a7e2b21c005 100644 --- a/src/effects/backends/effectprocessor.h +++ b/src/effects/backends/effectprocessor.h @@ -111,9 +111,9 @@ class EffectProcessorImpl : public EffectProcessor { /// Subclasses should not implement their own destructor. All state should /// be stored in the EffectState subclass, not the EffectProcessorImpl subclass. ~EffectProcessorImpl() { - //if (kEffectDebugOutput) { - qDebug() << "~EffectProcessorImpl" << this; - //} + if (kEffectDebugOutput) { + qDebug() << "~EffectProcessorImpl" << this; + } }; /// NOTE: Subclasses for Built-In effects must implement the following static methods for From b504c61569a73ee977b0078c21cb4bef23368a21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Fri, 28 Oct 2022 20:30:11 +0200 Subject: [PATCH 19/21] Introduce kInitalSamplerRate to use before the SoundDevice is set up --- src/engine/effects/engineeffect.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/engine/effects/engineeffect.cpp b/src/engine/effects/engineeffect.cpp index a0ef1ad5ac7..bd34525b9dd 100644 --- a/src/engine/effects/engineeffect.cpp +++ b/src/engine/effects/engineeffect.cpp @@ -4,6 +4,13 @@ #include "util/defs.h" #include "util/sample.h" +namespace { + +// Used during initialization where the SoundSevice is not set up +constexpr auto kInitalSampleRate = mixxx::audio::SampleRate(96000); + +} // namespace + EngineEffect::EngineEffect(EffectManifestPointer pManifest, EffectsBackendManagerPointer pBackendManager, const QSet& activeInputChannels, @@ -30,9 +37,9 @@ EngineEffect::EngineEffect(EffectManifestPointer pManifest, m_pProcessor->loadEngineEffectParameters(m_parametersById); - //TODO: get actual configuration of engine + // At this point the SoundDevice is not set up so we use the kInitalSampleRate. const mixxx::EngineParameters engineParameters( - mixxx::audio::SampleRate(96000), + kInitalSampleRate, MAX_BUFFER_LEN / mixxx::kEngineChannelCount); m_pProcessor->initialize(activeInputChannels, registeredOutputChannels, engineParameters); m_effectRampsFromDry = pManifest->effectRampsFromDry(); @@ -51,9 +58,9 @@ void EngineEffect::initalizeInputChannel(ChannelHandle inputChannel) { // already initialized for this input channel } - //TODO: get actual configuration of engine + // At this point the SoundDevice is not set up so we use the kInitalSampleRate. const mixxx::EngineParameters engineParameters( - mixxx::audio::SampleRate(96000), + kInitalSampleRate, MAX_BUFFER_LEN / mixxx::kEngineChannelCount); m_pProcessor->initializeInputChannel(inputChannel, engineParameters); } From 3528e4c5247d853b0a42c21bf87bf696c8ebe897 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Fri, 28 Oct 2022 20:31:19 +0200 Subject: [PATCH 20/21] Avoid confusing handle().handle() code --- src/effects/backends/effectprocessor.h | 8 ++++---- src/engine/channelhandle.h | 20 +++++++++++++------- src/test/channelhandle_test.cpp | 12 ++++++------ 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/effects/backends/effectprocessor.h b/src/effects/backends/effectprocessor.h index a7e2b21c005..9a37b002690 100644 --- a/src/effects/backends/effectprocessor.h +++ b/src/effects/backends/effectprocessor.h @@ -137,7 +137,7 @@ class EffectProcessorImpl : public EffectProcessor { const EffectEnableState enableState, const GroupFeatureState& groupFeatures) final { EffectSpecificState* pState = - m_channelStateMatrix[inputHandle][outputHandle.handle()].get(); + m_channelStateMatrix[inputHandle][outputHandle].get(); VERIFY_OR_DEBUG_ASSERT(pState != nullptr) { if (kEffectDebugOutput) { qWarning() << "EffectProcessorImpl::process could not retrieve" @@ -175,7 +175,7 @@ class EffectProcessorImpl : public EffectProcessor { // gaps are filled with nullptr for (const ChannelHandleAndGroup& outputChannel : std::as_const(m_registeredOutputChannels)) { - int vectorIndex = outputChannel.handle().handle(); + int vectorIndex = outputChannel.handle(); if (requiredVectorSize <= vectorIndex) { requiredVectorSize = vectorIndex + 1; } @@ -191,14 +191,14 @@ class EffectProcessorImpl : public EffectProcessor { } for (const ChannelHandleAndGroup& outputChannel : std::as_const(m_registeredOutputChannels)) { - outputChannelStates[outputChannel.handle().handle()].reset( + outputChannelStates[outputChannel.handle()].reset( createSpecificState(engineParameters)); if (kEffectDebugOutput) { qDebug() << this << "EffectProcessorImpl::initialize " "registering output" << outputChannel << outputChannel.handle() - << outputChannelStates[outputChannel.handle().handle()].get(); + << outputChannelStates[outputChannel.handle()].get(); } } }; diff --git a/src/engine/channelhandle.h b/src/engine/channelhandle.h index 76662259635..b6f48670b30 100644 --- a/src/engine/channelhandle.h +++ b/src/engine/channelhandle.h @@ -41,6 +41,10 @@ class ChannelHandle { return m_iHandle; } + operator int() const { + return m_iHandle; + } + private: ChannelHandle(int iHandle) : m_iHandle(iHandle) { @@ -67,6 +71,10 @@ inline bool operator==(const ChannelHandle& h1, const ChannelHandle& h2) { return h1.handle() == h2.handle(); } +inline bool operator==(const int& i, const ChannelHandle& h2) { + return i == h2.handle(); +} + inline bool operator!=(const ChannelHandle& h1, const ChannelHandle& h2) { return h1.handle() != h2.handle(); } @@ -181,7 +189,7 @@ class ChannelHandleMap { if (!handle.valid()) { return m_dummy; } - return m_data.at(handle.handle()); + return m_data.at(handle); } void insert(const ChannelHandle& handle, const T& value) { @@ -189,18 +197,16 @@ class ChannelHandleMap { return; } - int iHandle = handle.handle(); - maybeExpand(iHandle + 1); - m_data[iHandle] = value; + maybeExpand(static_cast(handle) + 1); + m_data[handle] = value; } T& operator[](const ChannelHandle& handle) { if (!handle.valid()) { return m_dummy; } - int iHandle = handle.handle(); - maybeExpand(iHandle + 1); - return m_data[iHandle]; + maybeExpand(static_cast(handle) + 1); + return m_data[handle]; } void clear() { diff --git a/src/test/channelhandle_test.cpp b/src/test/channelhandle_test.cpp index 4d85fbde9ec..69977a7238f 100644 --- a/src/test/channelhandle_test.cpp +++ b/src/test/channelhandle_test.cpp @@ -17,21 +17,21 @@ TEST(ChannelHandleTest, BasicUsage) { EXPECT_FALSE(factory.handleForGroup(group).valid()); // The ChannelHandleFactory constructor creates handles for [Master] and [Headphone] - EXPECT_EQ(0, factory.getOrCreateHandle(group).handle()); - EXPECT_EQ(0, factory.getOrCreateHandle(group).handle()); + EXPECT_EQ(0, factory.getOrCreateHandle(group)); + EXPECT_EQ(0, factory.getOrCreateHandle(group)); ChannelHandle testHandle = factory.handleForGroup(group); EXPECT_TRUE(testHandle.valid()); - EXPECT_EQ(0, testHandle.handle()); + EXPECT_EQ(0, testHandle); EXPECT_QSTRING_EQ(group, factory.groupForHandle(testHandle)); EXPECT_NE(nullHandle, testHandle); EXPECT_EQ(testHandle, testHandle); EXPECT_FALSE(factory.handleForGroup(group2).valid()); - EXPECT_EQ(1, factory.getOrCreateHandle(group2).handle()); - EXPECT_EQ(1, factory.getOrCreateHandle(group2).handle()); + EXPECT_EQ(1, factory.getOrCreateHandle(group2)); + EXPECT_EQ(1, factory.getOrCreateHandle(group2)); ChannelHandle testHandle2 = factory.handleForGroup(group2); EXPECT_TRUE(testHandle2.valid()); - EXPECT_EQ(1, testHandle2.handle()); + EXPECT_EQ(1, testHandle2); EXPECT_QSTRING_EQ(group2, factory.groupForHandle(testHandle2)); EXPECT_NE(nullHandle, testHandle2); EXPECT_EQ(testHandle2, testHandle2); From 8d133e665d530ac4735583b2986aa3c4db3de32c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Fri, 28 Oct 2022 23:40:03 +0200 Subject: [PATCH 21/21] Added missing final keyword --- src/effects/backends/effectprocessor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/effects/backends/effectprocessor.h b/src/effects/backends/effectprocessor.h index 9a37b002690..36d09f5f5fe 100644 --- a/src/effects/backends/effectprocessor.h +++ b/src/effects/backends/effectprocessor.h @@ -203,7 +203,7 @@ class EffectProcessorImpl : public EffectProcessor { } }; - bool hasStatesForInputChannel(ChannelHandle inputChannel) const { + bool hasStatesForInputChannel(ChannelHandle inputChannel) const final { if (inputChannel.handle() < m_channelStateMatrix.size()) { for (const auto& pState : m_channelStateMatrix.at(inputChannel)) { if (pState) {