Fix memory leak in RealtimeEffectManager

Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

src/AudioIO.cpp

@@ -635,11 +635,10 @@ struct AudioIoCallback::ScrubState : NonInterferingBase
       mStopped.store( true, std::memory_order_relaxed );
    }
 
-#if 0
+#ifdef DRAG_SCRUB
     // Needed only for the DRAG_SCRUB experiment
     // Should make mS1 atomic then?
-   double LastTrackTime() const
+    double LastTrackTime() const {
-   {
         // Needed by the main thread sometimes
         return mData.mS1.as_double() / mRate;
     }
@@ -909,14 +908,11 @@ class MidiThread final : public AudioThread {
 //
 //////////////////////////////////////////////////////////////////////
 
-void AudioIO::Init()
+void AudioIO::Init() {
-{
+    ugAudioIO.reset(new AudioIO());
-   ugAudioIO.reset(safenew AudioIO());
     Get()->mThread->Run();
-#ifdef EXPERIMENTAL_MIDI_OUT
+#if defined(EXPERIMENTAL_MIDI_OUT) && defined(USE_MIDI_THREAD)
-#ifdef USE_MIDI_THREAD
     Get()->mMidiThread->Run();
-#endif
 #endif
 
    // Make sure device prefs are initialized
@@ -990,6 +986,7 @@ AudioIO::AudioIO()
 #ifdef EXPERIMENTAL_AUTOMATED_INPUT_LEVEL_ADJUSTMENT
    mAILAActive = false;
 #endif
+
     mStreamToken = 0;
 
    mLastPaError = paNoError;
@@ -1010,15 +1007,15 @@ AudioIO::AudioIO()
     if (err != paNoError) {
         auto errStr = XO("Could not find any audio devices.\n");
         errStr += XO("You will not be able to play or record audio.\n\n");
-      wxString paErrStr = LAT1CTOWX(Pa_GetErrorText(err));
+        const wxString paErrStr = LAT1CTOWX(Pa_GetErrorText(err));
         if (!paErrStr.empty())
-         errStr += XO("Error: %s").Format( paErrStr );
+            errStr += XO("Error: %s").Format(paErrStr);
         // XXX: we are in libaudacity, popping up dialogs not allowed!  A
         // long-term solution will probably involve exceptions
         AudacityMessageBox(
             errStr,
             XO("Error Initializing Audio"),
-         wxICON_ERROR|wxOK);
+            wxICON_ERROR | wxOK);
 
       // Since PortAudio is not initialized, all calls to PortAudio
       // functions will fail.  This will give reasonable behavior, since
@@ -1033,15 +1030,15 @@ AudioIO::AudioIO()
         auto errStr =
             XO("There was an error initializing the midi i/o layer.\n");
         errStr += XO("You will not be able to play midi.\n\n");
-      wxString pmErrStr = LAT1CTOWX(Pm_GetErrorText(pmErr));
+        const wxString pmErrStr = LAT1CTOWX(Pm_GetErrorText(pmErr));
         if (!pmErrStr.empty())
-         errStr += XO("Error: %s").Format( pmErrStr );
+            errStr += XO("Error: %s").Format(pmErrStr);
         // XXX: we are in libaudacity, popping up dialogs not allowed!  A
         // long-term solution will probably involve exceptions
         AudacityMessageBox(
             errStr,
             XO("Error Initializing Midi"),
-         wxICON_ERROR|wxOK);
+            wxICON_ERROR | wxOK);
 
       // Same logic for PortMidi as described above for PortAudio
    }
@@ -2492,7 +2489,7 @@ void AudioIO::StopScrub()
       mScrubState->Stop();
 }
 
-#if 0
+#ifdef DRAG_SCRUB
 // Only for DRAG_SCRUB
 double AudioIO::GetLastScrubTime() const
 {
@@ -3843,19 +3840,18 @@ bool AudioIoCallback::FillOutputBuffers(
       return true;
    }
 
-   //Real time process section
+    if (numPlaybackTracks == 0) {
-   {
+        CallbackCheckCompletion(mCallbackReturn, 0);
-       std::unique_ptr<AudioIOBufferHelper> bufHelper = std::make_unique<AudioIOBufferHelper>(numPlaybackChannels, framesPerBuffer);
+        return false;
-       auto& em = RealtimeEffectManager::Get();
+    }
-       em.RealtimeProcessStart();
 
     bool selected = false;
     int group = 0;
     int chanCnt = 0;
+    auto& em = RealtimeEffectManager::Get();
 
     // Choose a common size to take from all ring buffers
     const auto toGet = std::min<size_t>(framesPerBuffer, GetCommonlyReadyPlayback());
-
     // The drop and dropQuickly booleans are so named for historical reasons.
     // JKC: The original code attempted to be faster by doing nothing on silenced audio.
     // This, IMHO, is 'premature optimisation'.  Instead clearer and cleaner code would
@@ -3871,55 +3867,50 @@ bool AudioIoCallback::FillOutputBuffers(
 
     bool drop = false;        // Track should become silent.
     bool dropQuickly = false; // Track has already been faded to silence.
-       for (unsigned t = 0; t < numPlaybackTracks; t++) {
-           WaveTrack* vt = mPlaybackTracks[t].get();
-           bufHelper.get()->chans[chanCnt] = vt;
 
-           // TODO: more-than-two-channels
+    decltype(framesPerBuffer) len = 0L;
-           auto nextTrack =
-               t + 1 < numPlaybackTracks
-               ? mPlaybackTracks[t + 1].get()
-               : nullptr;
 
-           // First and last channel in this group (for example left and right
+    std::unique_ptr<AudioIOBufferHelper> bufHelper = std::make_unique<AudioIOBufferHelper>(numPlaybackChannels, framesPerBuffer);
-           // channels of stereo).
+    //Real time process section
-           bool firstChannel = vt->IsLeader();
+    {
-           bool lastChannel = !nextTrack || nextTrack->IsLeader();
+        em.RealtimeProcessStart();
+
+        for (unsigned int t = 0; t < numPlaybackTracks; t++) {
+            WaveTrack* channel_one = mPlaybackTracks[t].get();
+            bufHelper.get()->chans[chanCnt] = channel_one;
+
+            const WaveTrack* channel_two = t + 1 < numPlaybackTracks ? mPlaybackTracks[t + 1].get() : nullptr;
+
+            // First and last channel in this group (for example left and right channels of stereo).
+            const bool firstChannel = channel_one->IsLeader();
+            const bool lastChannel = !channel_two || channel_two->IsLeader();
 
             if (firstChannel) {
-               selected = vt->GetSelected();
+                selected = channel_one->GetSelected();
                 // IF mono THEN clear 'the other' channel.
                 if (lastChannel && (numPlaybackChannels > 1)) {
                     // TODO: more-than-two-channels
                     memset(bufHelper.get()->tempBufs[1], 0, framesPerBuffer * sizeof(float));
                 }
-               drop = TrackShouldBeSilent(*vt);
+                dropQuickly = drop = TrackShouldBeSilent(*channel_one);
-               dropQuickly = drop;
             }
 
             if (mbMicroFades)
-               dropQuickly = dropQuickly && TrackHasBeenFadedOut(*vt);
+                dropQuickly = dropQuickly && TrackHasBeenFadedOut(*channel_one);
-
-           decltype(framesPerBuffer) len = 0;
 
             if (dropQuickly) {
                 len = mPlaybackBuffers[t]->Discard(toGet);
                 // keep going here.  
                 // we may still need to issue a paComplete.
             } else {
-               len = mPlaybackBuffers[t]->Get((samplePtr)bufHelper.get()->tempBufs[chanCnt],
+                const auto ptrToSample = (samplePtr)bufHelper.get()->tempBufs[chanCnt];
-                                              floatSample,
+
-                                              toGet);
+                len = mPlaybackBuffers[t]->Get(ptrToSample, floatSample, toGet);
-               // wxASSERT( len == toGet );
+
-               if (len < framesPerBuffer)
+                if (len < framesPerBuffer) {
-                   // This used to happen normally at the end of non-looping
+                    //Make sure we fill the output buffer even if we have insufficient length
-                   // plays, but it can also be an anomalous case where the
+                    memset(&bufHelper.get()->tempBufs[chanCnt][len], 0, (framesPerBuffer - len) * sizeof(float));
-                   // supply from FillBuffers fails to keep up with the
+                }
-                   // real-time demand in this thread (see bug 1932).  We
-                   // must supply something to the sound card, so pad it with
-                   // zeroes and not random garbage.
-                   memset((void*)&bufHelper.get()->tempBufs[chanCnt][len], 0,
-                          (framesPerBuffer - len) * sizeof(float));
                 chanCnt++;
             }
 
@@ -3932,19 +3923,19 @@ bool AudioIoCallback::FillOutputBuffers(
            // available, so maxLen ought to increase from 0 only once
            mMaxFramesOutput = std::max(mMaxFramesOutput, len);
 
-           if (!lastChannel)
+            if (lastChannel) {
-               continue;
-
                 // Last channel of a track seen now
                 len = mMaxFramesOutput;
 
                 if (!dropQuickly && selected)
                     len = em.RealtimeProcess(group, chanCnt, bufHelper.get()->tempBufs, len);
+
                 group++;
 
                 CallbackCheckCompletion(mCallbackReturn, len);
-           if (dropQuickly) // no samples to process, they've been discarded
+
-               continue;
+                if (!dropQuickly) {
+                    // no samples to process, they've been discarded
 
                     // Our channels aren't silent.  We need to pass their data on.
                     //
@@ -3956,30 +3947,30 @@ bool AudioIoCallback::FillOutputBuffers(
                     //
                     // Each channel in the tracks can output to more than one channel on the device.
                     // For example mono channels output to both left and right output channels.
-           if (len > 0) for (int c = 0; c < chanCnt; c++) {
+                    const bool len_in_bounds = len > 0;
-               vt = bufHelper.get()->chans[c];
 
-               if (vt->GetChannelIgnoringPan() == Track::LeftChannel || vt->GetChannelIgnoringPan() == Track::MonoChannel)
+                    for (int c = 0; c < chanCnt && len_in_bounds; c++) {
-                   AddToOutputChannel(0, outputMeterFloats, outputFloats, bufHelper.get()->tempBufs[c], drop, len, vt);
+                        const auto channel = bufHelper.get()->chans[c];
 
-               if (vt->GetChannelIgnoringPan() == Track::RightChannel || vt->GetChannelIgnoringPan() == Track::MonoChannel)
+                        const bool playLeftChannel = channel->GetChannelIgnoringPan() == Track::LeftChannel || channel->GetChannelIgnoringPan() == Track::MonoChannel;
-                   AddToOutputChannel(1, outputMeterFloats, outputFloats, bufHelper.get()->tempBufs[c], drop, len, vt);
+                        const bool playRightChannel = channel->GetChannelIgnoringPan() == Track::RightChannel || channel->GetChannelIgnoringPan() == Track::MonoChannel;
+
+                        if (playLeftChannel)
+                            AddToOutputChannel(0, outputMeterFloats, outputFloats, bufHelper.get()->tempBufs[c], drop, len, channel);
+
+                        if (playRightChannel)
+                            AddToOutputChannel(1, outputMeterFloats, outputFloats, bufHelper.get()->tempBufs[c], drop, len, channel);
                     }
 
                     chanCnt = 0;
                 }
-
+            }
-
+        }
-       // Poke: If there are no playback tracks, then the earlier check
-       // about the time indicator being past the end won't happen;
-       // do it here instead (but not if looping or scrubbing)
-       if (numPlaybackTracks == 0)
-           CallbackCheckCompletion(mCallbackReturn, 0);
 
         // wxASSERT( maxLen == toGet );
 
         em.RealtimeProcessEnd();
-       delete bufHelper.release();
+        bufHelper.reset();
     }
     mLastPlaybackTimeMillis = ::wxGetUTCTimeMillis();
 

src/AudioIOBase.h

@@ -11,9 +11,6 @@ Paul Licameli split from AudioIO.h
 #ifndef __AUDACITY_AUDIO_IO_BASE__
 #define __AUDACITY_AUDIO_IO_BASE__
 
-
-
-
 #include <cfloat>
 #include <functional>
 #include <vector>
@@ -38,7 +35,7 @@ class BoundedEnvelope;
 // Windows build needs complete type for parameter of wxWeakRef
 // class MeterPanelBase;
 #include "widgets/MeterPanelBase.h"
-using PRCrossfadeData = std::vector< std::vector < float > >;
+using PRCrossfadeData = std::vector< std::vector<float>>;
 
 #define BAD_STREAM_TIME (-DBL_MAX)
 

src/AudioIOBufferHelper.h

@@ -33,14 +33,8 @@ class AudioIOBufferHelper
     ~AudioIOBufferHelper() {
 
         delete[] tempBufs[0];
-
         delete[] tempBufs;
-
-        tempBufs = nullptr;
-
         delete[] chans;
-
-        chans = nullptr;
     }
 };
 

src/effects/RealtimeEffectManager.cpp

@@ -307,50 +307,46 @@ void RealtimeEffectManager::RealtimeProcessStart()
 //
 size_t RealtimeEffectManager::RealtimeProcess(int group, unsigned chans, float **buffers, size_t numSamples)
 {
+
+    // Allocate the in/out buffer arrays
+    auto ibuf = new float* [chans];
+    auto obuf = new float* [chans];
+    float* temp = safenew float[numSamples];
+
+    const size_t memcpy_size = numSamples * sizeof(float);
+
+    // Allocate new output buffers and copy buffer input into newly allocated input buffers
+    for (unsigned int i = 0; i < chans; i++) {
+        ibuf[i] = new float[numSamples];
+        memcpy(ibuf[i], buffers[i], memcpy_size);
+        obuf[i] = new float[numSamples];
+    }
+
     // Protect ourselves from the main thread
    mRealtimeLock.Enter();
 
    // Can be suspended because of the audio stream being paused or because effects
-   // have been suspended, so allow the samples to pass as-is.
+   // have been suspended, so in that case do nothing.
-   if (mRealtimeSuspended || mStates.empty())
+   if (!mRealtimeSuspended && !mStates.empty()){
-   {
+
-      mRealtimeLock.Leave();
-      return numSamples;
-   }
 
        // Remember when we started so we can calculate the amount of latency we
        // are introducing
        wxMilliClock_t start = wxGetUTCTimeMillis();
 
-   // Allocate the in/out buffer arrays
-   auto ibuf = new float* [chans];
-   auto obuf = new float* [chans];
-
-   // And populate the input with the buffers we've been given while allocating
-   // NEW output buffers
-   for (unsigned int i = 0; i < chans; i++)
-   {
-      ibuf[i] = buffers[i];
-      obuf[i] = new float[numSamples];
-   }
-
        // 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 &state : mStates)
+       for (auto& state : mStates) {
-   {
+           if (state->IsRealtimeActive()) {
-      if (state->IsRealtimeActive())
-      {
                state->RealtimeProcess(group, chans, ibuf, obuf, numSamples);
                called++;
            }
 
-      for (unsigned int j = 0; j < chans; j++)
+           for (size_t j = 0; j < chans; j++) {
-      {
+               memcpy(temp, ibuf[j], memcpy_size);
-         float *temp;
+               memcpy(ibuf[j], obuf[j], memcpy_size);
-         temp = ibuf[j];
+               memcpy(obuf[j], temp, memcpy_size);
-         ibuf[j] = obuf[j];
-         obuf[j] = temp;
            }
        }
 
@@ -358,22 +354,32 @@ size_t RealtimeEffectManager::RealtimeProcess(int group, unsigned chans, float *
        // in the temporary buffers.  If that's the case, we need to copy it over to
        // the caller's buffers.  This happens when the number of effects processed
        // is odd.
-   if (called & 1)
+       if (called & 1) {
-   {
+           for (size_t i = 0; i < chans; i++) {
-      for (unsigned int i = 0; i < chans; i++)
+               memcpy(buffers[i], ibuf[i], memcpy_size);
-      {
-         memcpy(buffers[i], ibuf[i], numSamples * sizeof(float));
            }
        }
 
-   delete ibuf;
-   delete[] obuf;
-
        // Remember the latency
-   mRealtimeLatency = (int) (wxGetUTCTimeMillis() - start).GetValue();
+       mRealtimeLatency = (int)(wxGetUTCTimeMillis() - start).GetValue();
+   }
 
    mRealtimeLock.Leave();
 
+   delete[] temp;
+
+   for (size_t i = 0; i < chans; i++) {
+       delete[] obuf[i];
+   }
+
+   delete[] obuf;
+
+   for (size_t i = 0; i < chans; i++) {
+       delete[] ibuf[i];
+   }
+
+   delete[] ibuf;
+
    //
    // This is wrong...needs to handle tails
    //
@@ -519,9 +525,9 @@ size_t RealtimeEffectState::RealtimeProcess(int group,
    const auto numAudioIn = mEffect.GetAudioInCount();
    const auto numAudioOut = mEffect.GetAudioOutCount();
 
-   auto clientIn = new float* [numAudioIn];
+   auto clientIn = safenew float* [numAudioIn];
-   auto clientOut = new float* [numAudioOut];
+   auto clientOut = safenew float* [numAudioOut];
-   auto dummybuf = new float [numSamples];
+   auto dummybuf = safenew float [numSamples];
 
    decltype(numSamples) len = 0;
    auto ichans = chans;
@@ -617,6 +623,7 @@ size_t RealtimeEffectState::RealtimeProcess(int group,
       // Bump to next processor
       processor++;
    }
+
    delete[] clientIn;
    delete[] clientOut;
    delete[] dummybuf;