Don't delete effect state object when chain is disabled. From the use…

…r perspective it is no difference between disabling an effect or a chain.

src/effects/backends/effectprocessor.h

@@ -76,12 +76,7 @@ class EffectProcessor {
             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
@@ -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<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;
-            }
-        }
-
-        QSet<ChannelHandleAndGroup> receivedOutputChannels = m_registeredOutputChannels;
-        for (const ChannelHandleAndGroup& outputChannel :
-                std::as_const(m_registeredOutputChannels)) {
-            if (kEffectDebugOutput) {
-                qDebug() << "EffectProcessorImpl::loadStatesForInputChannel"
-                         << this << "output" << outputChannel;
-            }
-
-            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.
-
-        // 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<EffectSpecificState*>& 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

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

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 {

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;
 

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

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

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,

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(

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,

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;