From c8bfa4fcb4a5aa06dcba4ac0c2e69ad8b8a60a7a Mon Sep 17 00:00:00 2001 From: Tony Mountifield Date: Fri, 19 Feb 2021 23:07:48 +0000 Subject: [PATCH 1/5] Fix some memory leaks. --- src/recorder/jamcontroller.cpp | 29 ++++++++++++++++++++--------- src/recorder/jamrecorder.cpp | 16 ++++++++++++++++ src/recorder/jamrecorder.h | 2 ++ 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/recorder/jamcontroller.cpp b/src/recorder/jamcontroller.cpp index 982b74cab3..7a24edbe35 100755 --- a/src/recorder/jamcontroller.cpp +++ b/src/recorder/jamcontroller.cpp @@ -30,7 +30,8 @@ CJamController::CJamController() : bRecorderInitialised ( false ), bEnableRecording ( false ), strRecordingDir ( "" ), - pthJamRecorder ( nullptr ) + pthJamRecorder ( nullptr ), + pJamRecorder ( nullptr ) { } @@ -77,15 +78,25 @@ void CJamController::SetRecordingDir ( QString newRecordingDir, int iServerFrameSizeSamples, bool bDisableRecording ) { - if ( bRecorderInitialised && pthJamRecorder != nullptr ) + if ( bRecorderInitialised ) { - // We have a thread and we want to start a new one. - // We only want one running. - // This could take time, unfortunately. - // Hopefully changing recording directory will NOT happen during a long jam... - emit EndRecorderThread(); - pthJamRecorder->wait(); - pthJamRecorder = nullptr; + if ( pthJamRecorder != nullptr ) + { + // We have a thread and we want to start a new one. + // We only want one running. + // This could take time, unfortunately. + // Hopefully changing recording directory will NOT happen during a long jam... + emit EndRecorderThread(); + pthJamRecorder->wait(); + delete pthJamRecorder; + pthJamRecorder = nullptr; + } + if ( pJamRecorder != nullptr ) + { + // We have a recorder running already. Terminate it. + delete pJamRecorder; + pJamRecorder = nullptr; + } } if ( !newRecordingDir.isEmpty() ) diff --git a/src/recorder/jamrecorder.cpp b/src/recorder/jamrecorder.cpp index 859f8ab9c6..11b3a994e9 100755 --- a/src/recorder/jamrecorder.cpp +++ b/src/recorder/jamrecorder.cpp @@ -89,6 +89,7 @@ void CJamClient::Frame(const QString _name, const CVector& pcm, int iSe void CJamClient::Disconnect() { static_cast(out)->finalise(); + delete out; out = nullptr; wavFile->close(); @@ -134,6 +135,21 @@ CJamSession::CJamSession(QDir recordBaseDir) : vecptrJamClients.fill(nullptr); } +/** + * @brief CJamSession::~CJamSession + */ +CJamSession::~CJamSession() +{ + for (int i = 0; i < jamClientConnections.count(); i++ ) + { + if ( jamClientConnections[i] ) + { + delete jamClientConnections[i]; + jamClientConnections[i] = nullptr; + } + } +} + /** * @brief CJamSession::DisconnectClient Capture details of the departing client's connection * @param iChID the channel id of the client that disconnected diff --git a/src/recorder/jamrecorder.h b/src/recorder/jamrecorder.h index a42fe0c457..340e593821 100755 --- a/src/recorder/jamrecorder.h +++ b/src/recorder/jamrecorder.h @@ -108,6 +108,8 @@ class CJamSession : public QObject CJamSession(QDir recordBaseDir); + virtual ~CJamSession(); + void Frame(const int iChID, const QString name, const CHostAddress address, const int numAudioChannels, const CVector data, int iServerFrameSizeSamples); void End(); From 6489e6a9494e8789fa9a18738a210ff64f01d13f Mon Sep 17 00:00:00 2001 From: Tony Mountifield Date: Sat, 20 Feb 2021 11:19:41 +0000 Subject: [PATCH 2/5] Ensure out was really set before dereferencing. --- src/recorder/jamrecorder.cpp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/recorder/jamrecorder.cpp b/src/recorder/jamrecorder.cpp index 11b3a994e9..87e6857ecb 100755 --- a/src/recorder/jamrecorder.cpp +++ b/src/recorder/jamrecorder.cpp @@ -45,7 +45,8 @@ CJamClient::CJamClient(const qint64 frame, const int _numChannels, const QString startFrame (frame), numChannels (static_cast(_numChannels)), name (name), - address (address) + address (address), + out (nullptr) { // At this point we may not have much of a name QString fileName = ClientName() + "-" + QString::number(frame) + "-" + QString::number(_numChannels); @@ -77,7 +78,7 @@ void CJamClient::Frame(const QString _name, const CVector& pcm, int iSe for(int i = 0; i < numChannels * iServerFrameSizeSamples; i++) { - *out << pcm[i]; + if (out) *out << pcm[i]; } frameCount++; @@ -88,9 +89,12 @@ void CJamClient::Frame(const QString _name, const CVector& pcm, int iSe */ void CJamClient::Disconnect() { - static_cast(out)->finalise(); - delete out; - out = nullptr; + if (out) + { + static_cast(out)->finalise(); + delete out; + out = nullptr; + } wavFile->close(); From e920490341efbf03fc1655135099e17fd5ac9cbf Mon Sep 17 00:00:00 2001 From: Tony Mountifield Date: Sat, 20 Feb 2021 23:31:50 +0000 Subject: [PATCH 3/5] Remove unnecessary check. --- src/recorder/jamrecorder.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/recorder/jamrecorder.cpp b/src/recorder/jamrecorder.cpp index 87e6857ecb..cbe8fed989 100755 --- a/src/recorder/jamrecorder.cpp +++ b/src/recorder/jamrecorder.cpp @@ -78,7 +78,7 @@ void CJamClient::Frame(const QString _name, const CVector& pcm, int iSe for(int i = 0; i < numChannels * iServerFrameSizeSamples; i++) { - if (out) *out << pcm[i]; + *out << pcm[i]; } frameCount++; From a26ff71167fef83f6dda7932d7e4737e0f6dc272 Mon Sep 17 00:00:00 2001 From: Tony Mountifield Date: Sat, 20 Feb 2021 23:34:37 +0000 Subject: [PATCH 4/5] Free pJamRecorder just before creating a new one. --- src/recorder/jamcontroller.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/recorder/jamcontroller.cpp b/src/recorder/jamcontroller.cpp index 7a24edbe35..2cc4cecd24 100755 --- a/src/recorder/jamcontroller.cpp +++ b/src/recorder/jamcontroller.cpp @@ -91,16 +91,16 @@ void CJamController::SetRecordingDir ( QString newRecordingDir, delete pthJamRecorder; pthJamRecorder = nullptr; } + } + + if ( !newRecordingDir.isEmpty() ) + { if ( pJamRecorder != nullptr ) { // We have a recorder running already. Terminate it. delete pJamRecorder; pJamRecorder = nullptr; } - } - - if ( !newRecordingDir.isEmpty() ) - { pJamRecorder = new recorder::CJamRecorder ( newRecordingDir, iServerFrameSizeSamples ); strRecorderErrMsg = pJamRecorder->Init(); bRecorderInitialised = ( strRecorderErrMsg == QString::null ); From b9c16dd79861c9410f20e739a30af7dd1440d6e7 Mon Sep 17 00:00:00 2001 From: Tony Mountifield Date: Sun, 21 Feb 2021 15:06:59 +0000 Subject: [PATCH 5/5] Minor cleanups. --- src/recorder/jamcontroller.cpp | 24 +++++++++++------------- src/recorder/jamrecorder.cpp | 1 + 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/recorder/jamcontroller.cpp b/src/recorder/jamcontroller.cpp index 2cc4cecd24..164f6a43fd 100755 --- a/src/recorder/jamcontroller.cpp +++ b/src/recorder/jamcontroller.cpp @@ -78,26 +78,24 @@ void CJamController::SetRecordingDir ( QString newRecordingDir, int iServerFrameSizeSamples, bool bDisableRecording ) { - if ( bRecorderInitialised ) + if ( bRecorderInitialised && pthJamRecorder != nullptr ) { - if ( pthJamRecorder != nullptr ) - { - // We have a thread and we want to start a new one. - // We only want one running. - // This could take time, unfortunately. - // Hopefully changing recording directory will NOT happen during a long jam... - emit EndRecorderThread(); - pthJamRecorder->wait(); - delete pthJamRecorder; - pthJamRecorder = nullptr; - } + // We have a thread and we want to start a new one. + // We only want one running. + // This could take time, unfortunately. + // Hopefully changing recording directory will NOT happen during a long jam... + emit EndRecorderThread(); + pthJamRecorder->wait(); + delete pthJamRecorder; + pthJamRecorder = nullptr; } if ( !newRecordingDir.isEmpty() ) { if ( pJamRecorder != nullptr ) { - // We have a recorder running already. Terminate it. + // We have a reference to a CJamRecorder instance that should now have finished. + // Clean up the instance before replacing it. delete pJamRecorder; pJamRecorder = nullptr; } diff --git a/src/recorder/jamrecorder.cpp b/src/recorder/jamrecorder.cpp index cbe8fed989..eb8edd6450 100755 --- a/src/recorder/jamrecorder.cpp +++ b/src/recorder/jamrecorder.cpp @@ -144,6 +144,7 @@ CJamSession::CJamSession(QDir recordBaseDir) : */ CJamSession::~CJamSession() { + // free up any active jamClientConnections for (int i = 0; i < jamClientConnections.count(); i++ ) { if ( jamClientConnections[i] )