Merge pull request #4707 from daschuer/effects_refactoring_3

Effect crasher fix

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
@@ -71,14 +72,12 @@ class EffectProcessor {
             const QSet<ChannelHandleAndGroup>& activeInputChannels,
             const QSet<ChannelHandleAndGroup>& registeredOutputChannels,
             const mixxx::EngineParameters& engineParameters) = 0;
+    virtual void initializeInputChannel(
+            ChannelHandle inputChannel,
+            const mixxx::EngineParameters& engineParameters) = 0;
     virtual void loadEngineEffectParameters(
             const QMap<QString, EngineEffectParameterPointer>& parameters) = 0;
-    virtual EffectState* createState(const mixxx::EngineParameters& engineParameters) = 0;
-    virtual void deleteStatesForInputChannel(ChannelHandle inputChannel) = 0;
-
-    // Called from the audio thread
-    virtual bool loadStatesForInputChannel(ChannelHandle inputChannel,
-            const EffectStatesMap* pStatesMap) = 0;
+    virtual bool hasStatesForInputChannel(ChannelHandle inputChannel) const = 0;
 
     /// Called from the audio thread
     /// This method takes a buffer of audio samples as pInput, processes the buffer
@@ -127,25 +126,6 @@ class EffectProcessorImpl : public EffectProcessor {
         if (kEffectDebugOutput) {
             qDebug() << "~EffectProcessorImpl" << this;
         }
-        int inputChannelHandleNumber = 0;
-        for (ChannelHandleMap<EffectSpecificState*>& 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();
     };
 
     /// NOTE: Subclasses for Built-In effects must implement the following static methods for
@@ -174,7 +154,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].get();
         VERIFY_OR_DEBUG_ASSERT(pState != nullptr) {
             if (kEffectDebugOutput) {
                 qWarning() << "EffectProcessorImpl::process could not retrieve"
@@ -184,8 +165,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);
     }
@@ -196,110 +176,61 @@ class EffectProcessorImpl : public EffectProcessor {
         m_registeredOutputChannels = registeredOutputChannels;
 
         for (const ChannelHandleAndGroup& inputChannel : activeInputChannels) {
-            if (kEffectDebugOutput) {
-                qDebug() << this << "EffectProcessorImpl::initialize allocating "
-                                    "EffectStates for input"
-                         << inputChannel;
-            }
-            ChannelHandleMap<EffectSpecificState*> outputChannelMap;
-            for (const ChannelHandleAndGroup& outputChannel :
-                    std::as_const(m_registeredOutputChannels)) {
-                outputChannelMap.insert(outputChannel.handle(),
-                        createSpecificState(engineParameters));
-                if (kEffectDebugOutput) {
-                    qDebug() << this << "EffectProcessorImpl::initialize "
-                                        "registering output"
-                             << outputChannel << outputChannelMap[outputChannel.handle()];
-                }
-            }
-            m_channelStateMatrix.insert(inputChannel.handle(), outputChannelMap);
+            initializeInputChannel(inputChannel.handle(), engineParameters);
         }
     };
 
-    EffectState* createState(const mixxx::EngineParameters& engineParameters) final {
-        return createSpecificState(engineParameters);
-    };
-
-    bool loadStatesForInputChannel(ChannelHandle inputChannel,
-            const EffectStatesMap* pStatesMap) final {
+    void initializeInputChannel(ChannelHandle inputChannel,
+            const mixxx::EngineParameters& engineParameters) final {
         if (kEffectDebugOutput) {
-            qDebug() << "EffectProcessorImpl::loadStatesForInputChannel" << this
-                     << "input" << inputChannel;
+            qDebug() << this << "EffectProcessorImpl::initialize allocating "
+                                "EffectStates for input"
+                     << inputChannel;
         }
 
-        // NOTE: ChannelHandleMap is like a map in that it associates an
-        // object with a ChannelHandle key, but it is actually backed by a
-        // QVarLengthArray, not a QMap. So it is okay that
-        // m_channelStateMatrix may be accessed concurrently in the main
-        // thread in deleteStatesForInputChannel.
-
-        // Can't directly cast a ChannelHandleMap from containing the base
-        // EffectState* type to EffectSpecificState* type, so iterate through
-        // pStatesMap to build a new ChannelHandleMap with
-        // dynamic_cast'ed states.
-        ChannelHandleMap<EffectSpecificState*>& effectSpecificStatesMap =
-                m_channelStateMatrix[inputChannel];
-
-        // deleteStatesForInputChannel should have been called before a new
-        // map of EffectStates was sent to this function, or this is the first
-        // time states are being loaded for this input channel, so
-        // effectSpecificStatesMap should be empty and this loop should
-        // not go through any iterations.
-        for (EffectSpecificState* pState : effectSpecificStatesMap) {
-            VERIFY_OR_DEBUG_ASSERT(pState == nullptr) {
-                delete pState;
+        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();
+            if (requiredVectorSize <= vectorIndex) {
+                requiredVectorSize = vectorIndex + 1;
             }
         }
 
-        QSet<ChannelHandleAndGroup> receivedOutputChannels = m_registeredOutputChannels;
+        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<EffectSpecificState>());
+        }
         for (const ChannelHandleAndGroup& outputChannel :
                 std::as_const(m_registeredOutputChannels)) {
+            outputChannelStates[outputChannel.handle()].reset(
+                    createSpecificState(engineParameters));
             if (kEffectDebugOutput) {
-                qDebug() << "EffectProcessorImpl::loadStatesForInputChannel"
-                         << this << "output" << outputChannel;
+                qDebug() << this
+                         << "EffectProcessorImpl::initialize "
+                            "registering output"
+                         << outputChannel << outputChannel.handle()
+                         << outputChannelStates[outputChannel.handle()].get();
             }
-
-            auto pState = dynamic_cast<EffectSpecificState*>(
-                    pStatesMap->at(outputChannel.handle()));
-            VERIFY_OR_DEBUG_ASSERT(pState != nullptr) {
-                return false;
-            }
-            effectSpecificStatesMap.insert(outputChannel.handle(), pState);
-            receivedOutputChannels.insert(outputChannel);
         }
-        // Output channels are hardcoded in EngineMaster and should not
-        // be registered after Mixxx initializes.
-        DEBUG_ASSERT(receivedOutputChannels == m_registeredOutputChannels);
-        return true;
     };
 
-    /// Called from main thread for garbage collection after an input channel is disabled
-    void deleteStatesForInputChannel(ChannelHandle inputChannel) final {
-        if (kEffectDebugOutput) {
-            qDebug() << "EffectProcessorImpl::deleteStatesForInputChannel"
-                     << this << inputChannel;
-        }
-
-        // NOTE: ChannelHandleMap is like a map in that it associates an
-        // object with a ChannelHandle key, but it is actually backed by a
-        // QVarLengthArray, not a QMap. So it is okay that
-        // m_channelStateMatrix may be accessed concurrently in the audio
-        // engine thread in loadStatesForInputChannel.
-
-        ChannelHandleMap<EffectSpecificState*>& stateMap =
-                m_channelStateMatrix[inputChannel];
-        for (EffectSpecificState* pState : stateMap) {
-            VERIFY_OR_DEBUG_ASSERT(pState != nullptr) {
-                continue;
-            }
-            if (kEffectDebugOutput) {
-                qDebug() << "EffectProcessorImpl::deleteStatesForInputChannel"
-                         << this << "deleting state" << pState;
+    bool hasStatesForInputChannel(ChannelHandle inputChannel) const final {
+        if (inputChannel.handle() < m_channelStateMatrix.size()) {
+            for (const auto& 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
@@ -315,5 +246,5 @@ class EffectProcessorImpl : public EffectProcessor {
 
   private:
     QSet<ChannelHandleAndGroup> m_registeredOutputChannels;
-    ChannelHandleMap<ChannelHandleMap<EffectSpecificState*>> m_channelStateMatrix;
+    ChannelHandleMap<std::vector<std::unique_ptr<EffectSpecificState>>> m_channelStateMatrix;
 };

src/effects/chains/equalizereffectchain.cpp

@@ -2,22 +2,25 @@
 
 #include "effects/effectslot.h"
 
-EqualizerEffectChain::EqualizerEffectChain(const QString& group,
+EqualizerEffectChain::EqualizerEffectChain(
+        const ChannelHandleAndGroup& handleAndGroup,
         EffectsManager* pEffectsManager,
         EffectsMessengerPointer pEffectsMessenger)
-        : PerGroupEffectChain(group,
-                  formatEffectChainGroup(group),
+        : PerGroupEffectChain(
+                  handleAndGroup,
+                  formatEffectChainGroup(handleAndGroup.name()),
                   SignalProcessingStage::Prefader,
                   pEffectsManager,
                   pEffectsMessenger),
           m_pCOFilterWaveform(
-                  new ControlObject(ConfigKey(group, "filterWaveformEnable"))) {
+                  new ControlObject(ConfigKey(handleAndGroup.name(), "filterWaveformEnable"))) {
     // Add a single effect slot
-    addEffectSlot(formatEffectSlotGroup(group));
+    addEffectSlot(formatEffectSlotGroup(handleAndGroup.name()));
+    enableForInputChannel(handleAndGroup);
     m_effectSlots[0]->setEnabled(true);
     // DlgPrefEq loads the Effect with loadEffectToGroup
 
-    setupLegacyAliasesForGroup(group);
+    setupLegacyAliasesForGroup(handleAndGroup.name());
 }
 
 void EqualizerEffectChain::setFilterWaveform(bool state) {

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);
 

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,6 @@ PerGroupEffectChain::PerGroupEffectChain(const QString& group,
     m_pControlChainMix->set(1.0);
     sendParameterUpdate();
 
-    // TODO(rryan): remove.
-    const ChannelHandleAndGroup* handleAndGroup = nullptr;
-    for (const ChannelHandleAndGroup& handle_group :
-            m_pEffectsManager->registeredInputChannels()) {
-        if (handle_group.name() == group) {
-            handleAndGroup = &handle_group;
-            break;
-        }
-    }
-    DEBUG_ASSERT(handleAndGroup != nullptr);
-
     // Register this channel alone with the chain slot.
-    registerInputChannel(*handleAndGroup);
-    enableForInputChannel(*handleAndGroup);
+    registerInputChannel(handleAndGroup);
 }

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,

src/effects/chains/quickeffectchain.cpp

@@ -3,18 +3,21 @@
 #include "effects/effectslot.h"
 #include "effects/presets/effectchainpresetmanager.h"
 
-QuickEffectChain::QuickEffectChain(const QString& group,
+QuickEffectChain::QuickEffectChain(
+        const ChannelHandleAndGroup& handleAndGroup,
         EffectsManager* pEffectsManager,
         EffectsMessengerPointer pEffectsMessenger)
-        : PerGroupEffectChain(group,
-                  formatEffectChainGroup(group),
+        : PerGroupEffectChain(
+                  handleAndGroup,
+                  formatEffectChainGroup(handleAndGroup.name()),
                   SignalProcessingStage::Postfader,
                   pEffectsManager,
                   pEffectsMessenger) {
     for (int i = 0; i < kNumEffectsPerUnit; ++i) {
-        addEffectSlot(formatEffectSlotGroup(group, i));
+        addEffectSlot(formatEffectSlotGroup(handleAndGroup.name(), i));
         m_effectSlots.at(i)->setEnabled(true);
     }
+    enableForInputChannel(handleAndGroup);
     // The base EffectChain class constructor connects to the signal for the list of StandardEffectChain presets,
     // but QuickEffectChain has a separate list, so disconnect the signal which is inappropriate for this subclass.
     disconnect(m_pChainPresetManager.data(),

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);
 

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);