From d69160975d15858b4abb7883af4ce074a84affd7 Mon Sep 17 00:00:00 2001 From: Emily Mabrey Date: Sun, 1 Aug 2021 01:39:50 -0400 Subject: [PATCH] Fix memory leak in `RealtimeEffectManager` Signed-off-by: Emily Mabrey --- src/AudioIO.cpp | 293 +++++++++++++------------- src/AudioIOBase.h | 5 +- src/AudioIOBufferHelper.h | 6 - src/effects/RealtimeEffectManager.cpp | 133 ++++++------ 4 files changed, 213 insertions(+), 224 deletions(-) diff --git a/src/AudioIO.cpp b/src/AudioIO.cpp index 316cfd495..c1747c786 100644 --- a/src/AudioIO.cpp +++ b/src/AudioIO.cpp @@ -635,14 +635,13 @@ struct AudioIoCallback::ScrubState : NonInterferingBase mStopped.store( true, std::memory_order_relaxed ); } -#if 0 - // Needed only for the DRAG_SCRUB experiment - // Should make mS1 atomic then? - double LastTrackTime() const - { - // Needed by the main thread sometimes - return mData.mS1.as_double() / mRate; - } +#ifdef DRAG_SCRUB + // Needed only for the DRAG_SCRUB experiment + // Should make mS1 atomic then? + double LastTrackTime() const { + // Needed by the main thread sometimes + return mData.mS1.as_double() / mRate; + } #endif ~ScrubState() {} @@ -909,14 +908,11 @@ class MidiThread final : public AudioThread { // ////////////////////////////////////////////////////////////////////// -void AudioIO::Init() -{ - ugAudioIO.reset(safenew AudioIO()); - Get()->mThread->Run(); -#ifdef EXPERIMENTAL_MIDI_OUT -#ifdef USE_MIDI_THREAD - Get()->mMidiThread->Run(); -#endif +void AudioIO::Init() { + ugAudioIO.reset(new AudioIO()); + Get()->mThread->Run(); +#if defined(EXPERIMENTAL_MIDI_OUT) && defined(USE_MIDI_THREAD) + Get()->mMidiThread->Run(); #endif // Make sure device prefs are initialized @@ -990,7 +986,8 @@ AudioIO::AudioIO() #ifdef EXPERIMENTAL_AUTOMATED_INPUT_LEVEL_ADJUSTMENT mAILAActive = false; #endif - mStreamToken = 0; + + mStreamToken = 0; mLastPaError = paNoError; @@ -1007,18 +1004,18 @@ AudioIO::AudioIO() PaError err = Pa_Initialize(); - 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)); - if (!paErrStr.empty()) - 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); + 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"); + const wxString paErrStr = LAT1CTOWX(Pa_GetErrorText(err)); + if (!paErrStr.empty()) + 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); // Since PortAudio is not initialized, all calls to PortAudio // functions will fail. This will give reasonable behavior, since @@ -1029,19 +1026,19 @@ AudioIO::AudioIO() #ifdef EXPERIMENTAL_MIDI_OUT PmError pmErr = Pm_Initialize(); - if (pmErr != pmNoError) { - 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)); - if (!pmErrStr.empty()) - 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); + if (pmErr != pmNoError) { + 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"); + const wxString pmErrStr = LAT1CTOWX(Pm_GetErrorText(pmErr)); + if (!pmErrStr.empty()) + 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); // 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,85 +3840,79 @@ bool AudioIoCallback::FillOutputBuffers( return true; } - //Real time process section - { - std::unique_ptr bufHelper = std::make_unique(numPlaybackChannels, framesPerBuffer); - auto& em = RealtimeEffectManager::Get(); - em.RealtimeProcessStart(); + if (numPlaybackTracks == 0) { + CallbackCheckCompletion(mCallbackReturn, 0); + return false; + } - bool selected = false; - int group = 0; - int chanCnt = 0; + 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(framesPerBuffer, GetCommonlyReadyPlayback()); + // Choose a common size to take from all ring buffers + const auto toGet = std::min(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 + // simply use a gain of 0.0 for silent audio and go on through to the stage of + // applying that 0.0 gain to the data mixed into the buffer. + // Then (and only then) we would have if needed fast paths for: + // - Applying a uniform gain of 0.0. + // - Applying a uniform gain of 1.0. + // - Applying some other uniform gain. + // - Applying a linearly interpolated gain. + // I would expect us not to need the fast paths, since linearly interpolated gain + // is very cheap to process. - // 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 - // simply use a gain of 0.0 for silent audio and go on through to the stage of - // applying that 0.0 gain to the data mixed into the buffer. - // Then (and only then) we would have if needed fast paths for: - // - Applying a uniform gain of 0.0. - // - Applying a uniform gain of 1.0. - // - Applying some other uniform gain. - // - Applying a linearly interpolated gain. - // I would expect us not to need the fast paths, since linearly interpolated gain - // is very cheap to process. + bool drop = false; // Track should become silent. + bool dropQuickly = false; // Track has already been faded to silence. - 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; + decltype(framesPerBuffer) len = 0L; - // TODO: more-than-two-channels - auto nextTrack = - t + 1 < numPlaybackTracks - ? mPlaybackTracks[t + 1].get() - : nullptr; + std::unique_ptr bufHelper = std::make_unique(numPlaybackChannels, framesPerBuffer); + //Real time process section + { + em.RealtimeProcessStart(); - // First and last channel in this group (for example left and right - // channels of stereo). - bool firstChannel = vt->IsLeader(); - bool lastChannel = !nextTrack || nextTrack->IsLeader(); + for (unsigned int t = 0; t < numPlaybackTracks; t++) { + WaveTrack* channel_one = mPlaybackTracks[t].get(); + bufHelper.get()->chans[chanCnt] = channel_one; - if (firstChannel) { - selected = vt->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; - } + const WaveTrack* channel_two = t + 1 < numPlaybackTracks ? mPlaybackTracks[t + 1].get() : nullptr; - if (mbMicroFades) - dropQuickly = dropQuickly && TrackHasBeenFadedOut(*vt); + // 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(); - decltype(framesPerBuffer) len = 0; + if (firstChannel) { + 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)); + } + dropQuickly = drop = TrackShouldBeSilent(*channel_one); + } - 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], - floatSample, - toGet); - // wxASSERT( len == toGet ); - if (len < framesPerBuffer) - // This used to happen normally at the end of non-looping - // plays, but it can also be an anomalous case where the - // 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++; - } + if (mbMicroFades) + dropQuickly = dropQuickly && TrackHasBeenFadedOut(*channel_one); + + if (dropQuickly) { + len = mPlaybackBuffers[t]->Discard(toGet); + // keep going here. + // we may still need to issue a paComplete. + } else { + const auto ptrToSample = (samplePtr)bufHelper.get()->tempBufs[chanCnt]; + + len = mPlaybackBuffers[t]->Get(ptrToSample, floatSample, toGet); + + if (len < framesPerBuffer) { + //Make sure we fill the output buffer even if we have insufficient length + memset(&bufHelper.get()->tempBufs[chanCnt][len], 0, (framesPerBuffer - len) * sizeof(float)); + } + chanCnt++; + } // PRL: Bug1104: // There can be a difference of len in different loop passes if one channel @@ -3932,56 +3923,56 @@ bool AudioIoCallback::FillOutputBuffers( // available, so maxLen ought to increase from 0 only once mMaxFramesOutput = std::max(mMaxFramesOutput, len); - if (!lastChannel) - continue; + if (lastChannel) { + // Last channel of a track seen now + len = mMaxFramesOutput; - // Last channel of a track seen now - len = mMaxFramesOutput; + if (!dropQuickly && selected) + len = em.RealtimeProcess(group, chanCnt, bufHelper.get()->tempBufs, len); - if (!dropQuickly && selected) - len = em.RealtimeProcess(group, chanCnt, bufHelper.get()->tempBufs, len); - group++; + group++; - CallbackCheckCompletion(mCallbackReturn, len); - if (dropQuickly) // no samples to process, they've been discarded - continue; + CallbackCheckCompletion(mCallbackReturn, len); - // Our channels aren't silent. We need to pass their data on. - // - // Note that there are two kinds of channel count. - // c and chanCnt are counting channels in the Tracks. - // chan (and numPlayBackChannels) is counting output channels on the device. - // chan = 0 is left channel - // chan = 1 is right channel. - // - // 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++) { - vt = bufHelper.get()->chans[c]; + if (!dropQuickly) { + // no samples to process, they've been discarded - if (vt->GetChannelIgnoringPan() == Track::LeftChannel || vt->GetChannelIgnoringPan() == Track::MonoChannel) - AddToOutputChannel(0, outputMeterFloats, outputFloats, bufHelper.get()->tempBufs[c], drop, len, vt); + // Our channels aren't silent. We need to pass their data on. + // + // Note that there are two kinds of channel count. + // c and chanCnt are counting channels in the Tracks. + // chan (and numPlayBackChannels) is counting output channels on the device. + // chan = 0 is left channel + // chan = 1 is right channel. + // + // 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. + const bool len_in_bounds = len > 0; - if (vt->GetChannelIgnoringPan() == Track::RightChannel || vt->GetChannelIgnoringPan() == Track::MonoChannel) - AddToOutputChannel(1, outputMeterFloats, outputFloats, bufHelper.get()->tempBufs[c], drop, len, vt); - } + for (int c = 0; c < chanCnt && len_in_bounds; c++) { + const auto channel = bufHelper.get()->chans[c]; - chanCnt = 0; - } + const bool playLeftChannel = channel->GetChannelIgnoringPan() == Track::LeftChannel || channel->GetChannelIgnoringPan() == Track::MonoChannel; + const bool playRightChannel = channel->GetChannelIgnoringPan() == Track::RightChannel || channel->GetChannelIgnoringPan() == Track::MonoChannel; + if (playLeftChannel) + AddToOutputChannel(0, outputMeterFloats, outputFloats, bufHelper.get()->tempBufs[c], drop, len, channel); - // 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); + if (playRightChannel) + AddToOutputChannel(1, outputMeterFloats, outputFloats, bufHelper.get()->tempBufs[c], drop, len, channel); + } - // wxASSERT( maxLen == toGet ); + chanCnt = 0; + } + } + } - em.RealtimeProcessEnd(); - delete bufHelper.release(); - } - mLastPlaybackTimeMillis = ::wxGetUTCTimeMillis(); + // wxASSERT( maxLen == toGet ); + + em.RealtimeProcessEnd(); + bufHelper.reset(); + } + mLastPlaybackTimeMillis = ::wxGetUTCTimeMillis(); ClampBuffer( outputFloats, framesPerBuffer*numPlaybackChannels ); if (outputMeterFloats != outputFloats) diff --git a/src/AudioIOBase.h b/src/AudioIOBase.h index df9e1c6ea..778c6420d 100644 --- a/src/AudioIOBase.h +++ b/src/AudioIOBase.h @@ -11,9 +11,6 @@ Paul Licameli split from AudioIO.h #ifndef __AUDACITY_AUDIO_IO_BASE__ #define __AUDACITY_AUDIO_IO_BASE__ - - - #include #include #include @@ -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>; #define BAD_STREAM_TIME (-DBL_MAX) diff --git a/src/AudioIOBufferHelper.h b/src/AudioIOBufferHelper.h index 7afbc6d13..4081ef2df 100644 --- a/src/AudioIOBufferHelper.h +++ b/src/AudioIOBufferHelper.h @@ -33,14 +33,8 @@ class AudioIOBufferHelper ~AudioIOBufferHelper() { delete[] tempBufs[0]; - delete[] tempBufs; - - tempBufs = nullptr; - delete[] chans; - - chans = nullptr; } }; diff --git a/src/effects/RealtimeEffectManager.cpp b/src/effects/RealtimeEffectManager.cpp index 7127dad6e..e4e5c92cb 100644 --- a/src/effects/RealtimeEffectManager.cpp +++ b/src/effects/RealtimeEffectManager.cpp @@ -307,73 +307,79 @@ void RealtimeEffectManager::RealtimeProcessStart() // size_t RealtimeEffectManager::RealtimeProcess(int group, unsigned chans, float **buffers, size_t numSamples) { - // Protect ourselves from the main thread + + // 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. - if (mRealtimeSuspended || mStates.empty()) - { - mRealtimeLock.Leave(); - return numSamples; + // have been suspended, so in that case do nothing. + if (!mRealtimeSuspended && !mStates.empty()){ + + + // Remember when we started so we can calculate the amount of latency we + // are introducing + wxMilliClock_t start = wxGetUTCTimeMillis(); + + // 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) { + if (state->IsRealtimeActive()) { + state->RealtimeProcess(group, chans, ibuf, obuf, numSamples); + called++; + } + + for (size_t j = 0; j < chans; j++) { + memcpy(temp, ibuf[j], memcpy_size); + memcpy(ibuf[j], obuf[j], memcpy_size); + memcpy(obuf[j], temp, memcpy_size); + } + } + + // Once we're done, we might wind up with the last effect storing its results + // 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) { + for (size_t i = 0; i < chans; i++) { + memcpy(buffers[i], ibuf[i], memcpy_size); + } + } + + // Remember the latency + mRealtimeLatency = (int)(wxGetUTCTimeMillis() - start).GetValue(); } - // 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) - { - if (state->IsRealtimeActive()) - { - state->RealtimeProcess(group, chans, ibuf, obuf, numSamples); - called++; - } - - for (unsigned int j = 0; j < chans; j++) - { - float *temp; - temp = ibuf[j]; - ibuf[j] = obuf[j]; - obuf[j] = temp; - } - } - - // Once we're done, we might wind up with the last effect storing its results - // 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) - { - for (unsigned int i = 0; i < chans; i++) - { - memcpy(buffers[i], ibuf[i], numSamples * sizeof(float)); - } - } - - delete ibuf; - delete[] obuf; - - // Remember the latency - 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 clientOut = new float* [numAudioOut]; - auto dummybuf = new float [numSamples]; + auto clientIn = safenew float* [numAudioIn]; + auto clientOut = safenew float* [numAudioOut]; + 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;