Move fields out of Effect, into new class RealtimeEffectState...

... and simplify, using a std::atomic instead of a critical section.

(But did this before, and does this now, correctly synchronize across threads?
I defer that question.)

src/effects/Effect.cpp

@@ -146,10 +146,6 @@ Effect::Effect()
    mNumGroups = 0;
    mProgress = NULL;
 
-   mRealtimeSuspendLock.Enter();
-   mRealtimeSuspendCount = 1;    // Effects are initially suspended
-   mRealtimeSuspendLock.Leave();
-
    mUIParent = NULL;
    mUIDialog = NULL;
 
@@ -476,46 +472,41 @@ bool Effect::RealtimeFinalize()
    return false;
 }
 
+RealtimeEffectState::RealtimeEffectState( Effect &effect )
+   : mEffect{ effect }
+{
+}
+
+bool RealtimeEffectState::RealtimeSuspend()
+{
+   auto result = mEffect.RealtimeSuspend();
+   if ( result ) {
+      mRealtimeSuspendCount++;
+   }
+   return result;
+}
+
 bool Effect::RealtimeSuspend()
 {
    if (mClient)
-   {
+      return mClient->RealtimeSuspend();
-      if (mClient->RealtimeSuspend())
-      {
-         mRealtimeSuspendLock.Enter();
-         mRealtimeSuspendCount++;
-         mRealtimeSuspendLock.Leave();
-         return true;
-      }
-
-      return false;
-   }
-
-   mRealtimeSuspendLock.Enter();
-   mRealtimeSuspendCount++;
-   mRealtimeSuspendLock.Leave();
 
    return true;
 }
 
+bool RealtimeEffectState::RealtimeResume()
+{
+   auto result = mEffect.RealtimeResume();
+   if ( result ) {
+      mRealtimeSuspendCount--;
+   }
+   return result;
+}
+
 bool Effect::RealtimeResume()
 {
    if (mClient)
-   {
+      return mClient->RealtimeResume();
-      if (mClient->RealtimeResume())
-      {
-         mRealtimeSuspendLock.Enter();
-         mRealtimeSuspendCount--;
-         mRealtimeSuspendLock.Leave();
-         return true;
-      }
-
-      return false;
-   }
-
-   mRealtimeSuspendLock.Enter();
-   mRealtimeSuspendCount--;
-   mRealtimeSuspendLock.Leave();
 
    return true;
 }
@@ -2306,7 +2297,7 @@ double Effect::CalcPreviewInputLength(double previewLength)
 // RealtimeAddProcessor and RealtimeProcess use the same method of
 // determining the current processor index, so updates to one should
 // be reflected in the other.
-bool Effect::RealtimeAddProcessor(int group, unsigned chans, float rate)
+bool RealtimeEffectState::RealtimeAddProcessor(int group, unsigned chans, float rate)
 {
    auto ichans = chans;
    auto ochans = chans;
@@ -2322,13 +2313,16 @@ bool Effect::RealtimeAddProcessor(int group, unsigned chans, float rate)
    // Remember the processor starting index
    mGroupProcessor.push_back(mCurrentProcessor);
 
+   const auto numAudioIn = mEffect.GetAudioInCount();
+   const auto numAudioOut = mEffect.GetAudioOutCount();
+
    // Call the client until we run out of input or output channels
    while (ichans > 0 && ochans > 0)
    {
       // If we don't have enough input channels to accomodate the client's
       // requirements, then we replicate the input channels until the
       // client's needs are met.
-      if (ichans < mNumAudioIn)
+      if (ichans < numAudioIn)
       {
          // All input channels have been consumed
          ichans = 0;
@@ -2336,16 +2330,16 @@ bool Effect::RealtimeAddProcessor(int group, unsigned chans, float rate)
       // Otherwise fullfil the client's needs with as many input channels as possible.
       // After calling the client with this set, we will loop back up to process more
       // of the input/output channels.
-      else if (ichans >= mNumAudioIn)
+      else if (ichans >= numAudioIn)
       {
-         gchans = mNumAudioIn;
+         gchans = numAudioIn;
          ichans -= gchans;
       }
 
       // If we don't have enough output channels to accomodate the client's
       // requirements, then we provide all of the output channels and fulfill
       // the client's needs with dummy buffers.  These will just get tossed.
-      if (ochans < mNumAudioOut)
+      if (ochans < numAudioOut)
       {
          // All output channels have been consumed
          ochans = 0;
@@ -2353,13 +2347,13 @@ bool Effect::RealtimeAddProcessor(int group, unsigned chans, float rate)
       // Otherwise fullfil the client's needs with as many output channels as possible.
       // After calling the client with this set, we will loop back up to process more
       // of the input/output channels.
-      else if (ochans >= mNumAudioOut)
+      else if (ochans >= numAudioOut)
       {
-         ochans -= mNumAudioOut;
+         ochans -= numAudioOut;
       }
 
       // Add a NEW processor
-      RealtimeAddProcessor(gchans, rate);
+      mEffect.RealtimeAddProcessor(gchans, rate);
 
       // Bump to next processor
       mCurrentProcessor++;
@@ -2371,7 +2365,7 @@ bool Effect::RealtimeAddProcessor(int group, unsigned chans, float rate)
 // RealtimeAddProcessor and RealtimeProcess use the same method of
 // determining the current processor group, so updates to one should
 // be reflected in the other.
-size_t Effect::RealtimeProcess(int group,
+size_t RealtimeEffectState::RealtimeProcess(int group,
                                     unsigned chans,
                                     float **inbuf,
                                     float **outbuf,
@@ -2386,8 +2380,11 @@ size_t Effect::RealtimeProcess(int group,
    // so if the number of channels we're curently processing are different
    // than what the effect expects, then we use a few methods of satisfying
    // the effects requirements.
-   float **clientIn = (float **) alloca(mNumAudioIn * sizeof(float *));
+   const auto numAudioIn = mEffect.GetAudioInCount();
-   float **clientOut = (float **) alloca(mNumAudioOut * sizeof(float *));
+   const auto numAudioOut = mEffect.GetAudioOutCount();
+
+   float **clientIn = (float **) alloca(numAudioIn * sizeof(float *));
+   float **clientOut = (float **) alloca(numAudioOut * sizeof(float *));
    float *dummybuf = (float *) alloca(numSamples * sizeof(float));
    decltype(numSamples) len = 0;
    auto ichans = chans;
@@ -2404,9 +2401,9 @@ size_t Effect::RealtimeProcess(int group,
       // If we don't have enough input channels to accomodate the client's
       // requirements, then we replicate the input channels until the
       // client's needs are met.
-      if (ichans < mNumAudioIn)
+      if (ichans < numAudioIn)
       {
-         for (size_t i = 0; i < mNumAudioIn; i++)
+         for (size_t i = 0; i < numAudioIn; i++)
          {
             if (indx == ichans)
             {
@@ -2421,10 +2418,10 @@ size_t Effect::RealtimeProcess(int group,
       // Otherwise fullfil the client's needs with as many input channels as possible.
       // After calling the client with this set, we will loop back up to process more
       // of the input/output channels.
-      else if (ichans >= mNumAudioIn)
+      else if (ichans >= numAudioIn)
       {
          gchans = 0;
-         for (size_t i = 0; i < mNumAudioIn; i++, ichans--, gchans++)
+         for (size_t i = 0; i < numAudioIn; i++, ichans--, gchans++)
          {
             clientIn[i] = inbuf[indx++];
          }
@@ -2433,9 +2430,9 @@ size_t Effect::RealtimeProcess(int group,
       // If we don't have enough output channels to accomodate the client's
       // requirements, then we provide all of the output channels and fulfill
       // the client's needs with dummy buffers.  These will just get tossed.
-      if (ochans < mNumAudioOut)
+      if (ochans < numAudioOut)
       {
-         for (size_t i = 0; i < mNumAudioOut; i++)
+         for (size_t i = 0; i < numAudioOut; i++)
          {
             if (i < ochans)
             {
@@ -2453,9 +2450,9 @@ size_t Effect::RealtimeProcess(int group,
       // Otherwise fullfil the client's needs with as many output channels as possible.
       // After calling the client with this set, we will loop back up to process more
       // of the input/output channels.
-      else if (ochans >= mNumAudioOut)
+      else if (ochans >= numAudioOut)
       {
-         for (size_t i = 0; i < mNumAudioOut; i++, ochans--)
+         for (size_t i = 0; i < numAudioOut; i++, ochans--)
          {
             clientOut[i] = outbuf[ondx++];
          }
@@ -2463,17 +2460,18 @@ size_t Effect::RealtimeProcess(int group,
 
       // Finally call the plugin to process the block
       len = 0;
-      for (decltype(numSamples) block = 0; block < numSamples; block += mBlockSize)
+      const auto blockSize = mEffect.GetBlockSize();
+      for (decltype(numSamples) block = 0; block < numSamples; block += blockSize)
       {
-         auto cnt = std::min(numSamples - block, mBlockSize);
+         auto cnt = std::min(numSamples - block, blockSize);
-         len += RealtimeProcess(processor, clientIn, clientOut, cnt);
+         len += mEffect.RealtimeProcess(processor, clientIn, clientOut, cnt);
 
-         for (size_t i = 0 ; i < mNumAudioIn; i++)
+         for (size_t i = 0 ; i < numAudioIn; i++)
          {
             clientIn[i] += cnt;
          }
 
-         for (size_t i = 0 ; i < mNumAudioOut; i++)
+         for (size_t i = 0 ; i < numAudioOut; i++)
          {
             clientOut[i] += cnt;
          }
@@ -2486,7 +2484,7 @@ size_t Effect::RealtimeProcess(int group,
    return len;
 }
 
-bool Effect::IsRealtimeActive()
+bool RealtimeEffectState::IsRealtimeActive()
 {
    return mRealtimeSuspendCount == 0;
 }

src/effects/Effect.h

@@ -16,6 +16,7 @@
 
 #include "../Experimental.h"
 
+#include <atomic>
 #include <set>
 
 #include <wx/defs.h>
@@ -262,15 +263,6 @@ class AUDACITY_DLL_API Effect /* not final */ : public wxEvtHandler,
 
    bool Delegate( Effect &delegate, wxWindow *parent, bool shouldPrompt);
 
-   // Realtime Effect Processing
-   /* not virtual */ bool RealtimeAddProcessor(int group, unsigned chans, float rate);
-   /* not virtual */ size_t RealtimeProcess(int group,
-                               unsigned chans,
-                               float **inbuf,
-                               float **outbuf,
-                               size_t numSamples);
-   /* not virtual */ bool IsRealtimeActive();
-
    virtual bool IsHidden();
 
    // Nonvirtual
@@ -544,12 +536,6 @@ private:
    size_t mBlockSize;
    unsigned mNumChannels;
 
-   std::vector<int> mGroupProcessor;
-   int mCurrentProcessor;
-
-   wxCriticalSection mRealtimeSuspendLock;
-   int mRealtimeSuspendCount;
-
    const static wxString kUserPresetIdent;
    const static wxString kFactoryPresetIdent;
    const static wxString kCurrentSettingsIdent;
@@ -561,6 +547,29 @@ private:
    friend class EffectPresetsDialog;
 };
 
+class RealtimeEffectState
+{
+public:
+   explicit RealtimeEffectState( Effect &effect );
+
+   Effect &GetEffect() const { return mEffect; }
+
+   bool RealtimeSuspend();
+   bool RealtimeResume();
+   bool RealtimeAddProcessor(int group, unsigned chans, float rate);
+   size_t RealtimeProcess(int group,
+      unsigned chans, float **inbuf, float **outbuf, size_t numSamples);
+   bool IsRealtimeActive();
+
+private:
+   Effect &mEffect;
+
+   std::vector<int> mGroupProcessor;
+   int mCurrentProcessor;
+
+   std::atomic<int> mRealtimeSuspendCount{ 1 };    // Effects are initially suspended
+};
+
 
 // FIXME:
 // FIXME:  Remove this once all effects are using the NEW dialog

src/effects/RealtimeEffectManager.cpp

@@ -42,51 +42,35 @@ void RealtimeEffectManager::RealtimeSetEffects(const EffectArray & effects)
    // Block RealtimeProcess()
    RealtimeSuspend();
 
-   // Tell any effects no longer in the chain to clean up
+   decltype( mStates ) newStates;
-   for (auto e: mRealtimeEffects)
+   auto begin = mStates.begin(), end = mStates.end();
-   {
+   for ( auto pEffect : effects ) {
-      // Scan the NEW chain for the effect
+      auto found = std::find_if( begin, end,
-      for (auto e1: effects)
+         [=]( const decltype( mStates )::value_type &state ){
-      {
+            return state && &state->GetEffect() == pEffect;
-         // Found it so we're done
-         if (e == e1)
-         {
-            e = NULL;
-            break;
          }
+      );
+      if ( found == end ) {
+         // Tell New effect to get ready
+         pEffect->RealtimeInitialize();
+         newStates.emplace_back(
+            std::make_unique< RealtimeEffectState >( *pEffect ) );
       }
-
+      else {
-      // Must not have been in the NEW chain, so tell it to cleanup
+         // Preserve state for effect that remains in the chain
-      if (e && mRealtimeActive)
+         newStates.emplace_back( std::move( *found ) );
-      {
-         e->RealtimeFinalize();
       }
    }
 
-   // Tell any NEW effects to get ready
+   // Remaining states that were not moved need to clean up
-   for (auto e : effects)
+   for ( auto &state : mStates ) {
-   {
+      if ( state )
-      // Scan the old chain for the effect
+         state->GetEffect().RealtimeFinalize();
-      for (auto e1 : mRealtimeEffects)
-      {
-         // Found it so tell effect to get ready
-         if (e == e1)
-         {
-            e = NULL;
-            break;
-         }
-      }
-
-      // Must not have been in the old chain, so tell it to initialize
-      if (e && mRealtimeActive)
-      {
-         e->RealtimeInitialize();
-      }
    }
 
    // Get rid of the old chain
    // And install the NEW one
-   mRealtimeEffects = effects;
+   mStates.swap( newStates );
 
    // Allow RealtimeProcess() to, well, process 
    RealtimeResume();
@@ -95,7 +79,7 @@ void RealtimeEffectManager::RealtimeSetEffects(const EffectArray & effects)
 
 bool RealtimeEffectManager::RealtimeIsActive()
 {
-   return mRealtimeEffects.size() != 0;
+   return mStates.size() != 0;
 }
 
 bool RealtimeEffectManager::RealtimeIsSuspended()
@@ -108,6 +92,10 @@ void RealtimeEffectManager::RealtimeAddEffect(Effect *effect)
    // Block RealtimeProcess()
    RealtimeSuspend();
 
+   // Add to list of active effects
+   mStates.emplace_back( std::make_unique< RealtimeEffectState >( *effect ) );
+   auto &state = mStates.back();
+
    // Initialize effect if realtime is already active
    if (mRealtimeActive)
    {
@@ -117,12 +105,10 @@ void RealtimeEffectManager::RealtimeAddEffect(Effect *effect)
       // Add the required processors
       for (size_t i = 0, cnt = mRealtimeChans.size(); i < cnt; i++)
       {
-         effect->RealtimeAddProcessor(i, mRealtimeChans[i], mRealtimeRates[i]);
+         state->RealtimeAddProcessor(i, mRealtimeChans[i], mRealtimeRates[i]);
       }
    }
    
-   // Add to list of active effects
-   mRealtimeEffects.push_back(effect);
 
    // Allow RealtimeProcess() to, well, process 
    RealtimeResume();
@@ -140,10 +126,14 @@ void RealtimeEffectManager::RealtimeRemoveEffect(Effect *effect)
    }
       
    // Remove from list of active effects
-   auto end = mRealtimeEffects.end();
+   auto end = mStates.end();
-   auto found = std::find(mRealtimeEffects.begin(), end, effect);
+   auto found = std::find_if( mStates.begin(), end,
+      [&](const decltype(mStates)::value_type &state){
+         return &state->GetEffect() == effect;
+      }
+   );
    if (found != end)
-      mRealtimeEffects.erase(found);
+      mStates.erase(found);
 
    // Allow RealtimeProcess() to, well, process 
    RealtimeResume();
@@ -163,9 +153,9 @@ void RealtimeEffectManager::RealtimeInitialize(double rate)
    mRealtimeActive = true;
 
    // Tell each effect to get ready for action
-   for (auto e : mRealtimeEffects) {
+   for (auto &state : mStates) {
-      e->SetSampleRate(rate);
+      state->GetEffect().SetSampleRate(rate);
-      e->RealtimeInitialize();
+      state->GetEffect().RealtimeInitialize();
    }
 
    // Get things moving
@@ -174,8 +164,8 @@ void RealtimeEffectManager::RealtimeInitialize(double rate)
 
 void RealtimeEffectManager::RealtimeAddProcessor(int group, unsigned chans, float rate)
 {
-   for (auto e : mRealtimeEffects)
+   for (auto &state : mStates)
-      e->RealtimeAddProcessor(group, chans, rate);
+      state->RealtimeAddProcessor(group, chans, rate);
 
    mRealtimeChans.push_back(chans);
    mRealtimeRates.push_back(rate);
@@ -190,8 +180,8 @@ void RealtimeEffectManager::RealtimeFinalize()
    mRealtimeLatency = 0;
 
    // Tell each effect to clean up as well
-   for (auto e : mRealtimeEffects)
+   for (auto &state : mStates)
-      e->RealtimeFinalize();
+      state->GetEffect().RealtimeFinalize();
 
    // Reset processor parameters
    mRealtimeChans.clear();
@@ -216,8 +206,8 @@ void RealtimeEffectManager::RealtimeSuspend()
    mRealtimeSuspended = true;
 
    // And make sure the effects don't either
-   for (auto e : mRealtimeEffects)
+   for (auto &state : mStates)
-      e->RealtimeSuspend();
+      state->RealtimeSuspend();
 
    mRealtimeLock.Leave();
 }
@@ -234,8 +224,8 @@ void RealtimeEffectManager::RealtimeResume()
    }
 
    // Tell the effects to get ready for more action
-   for (auto e : mRealtimeEffects)
+   for (auto &state : mStates)
-      e->RealtimeResume();
+      state->RealtimeResume();
 
    // And we should too
    mRealtimeSuspended = false;
@@ -255,10 +245,10 @@ void RealtimeEffectManager::RealtimeProcessStart()
    // have been suspended.
    if (!mRealtimeSuspended)
    {
-      for (auto e : mRealtimeEffects)
+      for (auto &state : mStates)
       {
-         if (e->IsRealtimeActive())
+         if (state->IsRealtimeActive())
-            e->RealtimeProcessStart();
+            state->GetEffect().RealtimeProcessStart();
       }
    }
 
@@ -275,7 +265,7 @@ size_t RealtimeEffectManager::RealtimeProcess(int group, unsigned chans, float *
 
    // Can be suspended because of the audio stream being paused or because effects
    // have been suspended, so allow the samples to pass as-is.
-   if (mRealtimeSuspended || mRealtimeEffects.empty())
+   if (mRealtimeSuspended || mStates.empty())
    {
       mRealtimeLock.Leave();
       return numSamples;
@@ -300,11 +290,11 @@ size_t RealtimeEffectManager::RealtimeProcess(int group, unsigned chans, float *
    // Now call each effect in the chain while swapping buffer pointers to feed the
    // output of one effect as the input to the next effect
    size_t called = 0;
-   for (auto e : mRealtimeEffects)
+   for (auto &state : mStates)
    {
-      if (e->IsRealtimeActive())
+      if (state->IsRealtimeActive())
       {
-         e->RealtimeProcess(group, chans, ibuf, obuf, numSamples);
+         state->RealtimeProcess(group, chans, ibuf, obuf, numSamples);
          called++;
       }
 
@@ -352,10 +342,10 @@ void RealtimeEffectManager::RealtimeProcessEnd()
    // have been suspended.
    if (!mRealtimeSuspended)
    {
-      for (auto e : mRealtimeEffects)
+      for (auto &state : mStates)
       {
-         if (e->IsRealtimeActive())
+         if (state->IsRealtimeActive())
-            e->RealtimeProcessEnd();
+            state->GetEffect().RealtimeProcessEnd();
       }
    }
 

src/effects/RealtimeEffectManager.h

@@ -17,6 +17,7 @@
 
 class Effect;
 using EffectArray = std::vector <Effect*> ;
+class RealtimeEffectState;
 
 class AUDACITY_DLL_API RealtimeEffectManager final
 {
@@ -46,7 +47,7 @@ private:
    ~RealtimeEffectManager();
 
    wxCriticalSection mRealtimeLock;
-   EffectArray mRealtimeEffects;
+   std::vector< std::unique_ptr<RealtimeEffectState> > mStates;
    int mRealtimeLatency;
    bool mRealtimeSuspended;
    bool mRealtimeActive;