From 4850f3ddb5e5cab13bbafdda0d91e2de822485c4 Mon Sep 17 00:00:00 2001 From: Dalton Messmer Date: Sat, 2 Sep 2023 14:32:05 -0400 Subject: [PATCH 1/6] Fix LADSPA effects memory leak --- plugins/LadspaEffect/caps/Descriptor.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/plugins/LadspaEffect/caps/Descriptor.h b/plugins/LadspaEffect/caps/Descriptor.h index 12c5d1c8846..c3e1c325e73 100644 --- a/plugins/LadspaEffect/caps/Descriptor.h +++ b/plugins/LadspaEffect/caps/Descriptor.h @@ -53,7 +53,7 @@ class DescriptorStub PortCount = 0; } - ~DescriptorStub() + virtual ~DescriptorStub() { if (PortCount) { @@ -87,6 +87,7 @@ class Descriptor public: Descriptor() { setup(); } + ~Descriptor() override = default; void setup(); void autogen() From 8a5c3cb1aa27e93e4ecc1c266ebec34747d53494 Mon Sep 17 00:00:00 2001 From: Dalton Messmer Date: Sun, 3 Sep 2023 00:48:16 -0400 Subject: [PATCH 2/6] Fix buffer overflow in PianoView --- src/gui/instrument/PianoView.cpp | 103 +++++++++++++++---------------- 1 file changed, 49 insertions(+), 54 deletions(-) diff --git a/src/gui/instrument/PianoView.cpp b/src/gui/instrument/PianoView.cpp index 20db2e8e83f..d20cbcac52e 100644 --- a/src/gui/instrument/PianoView.cpp +++ b/src/gui/instrument/PianoView.cpp @@ -322,70 +322,65 @@ void PianoView::modelChanged() -// gets the key from the given mouse-position +// Gets the key from the given mouse position /*! \brief Get the key from the mouse position in the piano display * - * First we determine it roughly by the position of the point given in - * white key widths from our start. We then add in any black keys that - * might have been skipped over (they take a key number, but no 'white - * key' space). We then add in our starting key number. - * - * We then determine whether it was a black key that was pressed by - * checking whether it was within the vertical range of black keys. - * Black keys sit exactly between white keys on this keyboard, so - * we then shift the note down or up if we were in the left or right - * half of the white note. We only do this, of course, if the white - * note has a black key on that side, so to speak. - * - * This function returns const because there is a linear mapping from - * the point given to the key returned that never changes. - * - * \param _p The point that the mouse was pressed. + * \param p The point that the mouse was pressed. */ -int PianoView::getKeyFromMouse( const QPoint & _p ) const +int PianoView::getKeyFromMouse(const QPoint& p) const { - int offset = _p.x() % PW_WHITE_KEY_WIDTH; - if( offset < 0 ) offset += PW_WHITE_KEY_WIDTH; - int key_num = ( _p.x() - offset) / PW_WHITE_KEY_WIDTH; + // The left-most key visible in the piano display is always white + const int startingWhiteKey = m_pianoScroll->value(); - for( int i = 0; i <= key_num; ++i ) - { - if ( Piano::isBlackKey( m_startKey+i ) ) - { - ++key_num; - } - } - for( int i = 0; i >= key_num; --i ) - { - if ( Piano::isBlackKey( m_startKey+i ) ) - { - --key_num; - } - } + // Adjust the mouse x position as if x == 0 was the left side of the lowest key + const int adjX = p.x() + (startingWhiteKey * PW_WHITE_KEY_WIDTH); + + // Can early return for notes too low + if (adjX <= 0) { return 0; } + + // Now we can calculate the key number (in only white keys) and the octave + const int whiteKey = adjX / PW_WHITE_KEY_WIDTH; + const int octave = whiteKey / Piano::WhiteKeysPerOctave; - key_num += m_startKey; + // Calculate for full octaves + int key = octave * KeysPerOctave; - // is it a black key? - if( _p.y() < PIANO_BASE + PW_BLACK_KEY_HEIGHT ) + // Adjust for white notes in the current octave + // (WhiteKeys maps each white key to the number of notes to their left in the octave) + key += static_cast(WhiteKeys[whiteKey % Piano::WhiteKeysPerOctave]); + + // Might be a black key, which would require further adjustment + if (p.y() < PIANO_BASE + PW_BLACK_KEY_HEIGHT) { - // then do extra checking whether the mouse-cursor is over - // a black key - if( key_num > 0 && Piano::isBlackKey( key_num-1 ) && - offset <= ( PW_WHITE_KEY_WIDTH / 2 ) - - ( PW_BLACK_KEY_WIDTH / 2 ) ) + // Maps white keys to neighboring black keys + static constexpr std::array neighboringKeyMap { + std::pair{ 0, 1 }, // C --> no B#; C# + std::pair{ 1, 1 }, // D --> C#; D# + std::pair{ 1, 0 }, // E --> D#; no E# + std::pair{ 0, 1 }, // F --> no E#; F# + std::pair{ 1, 1 }, // G --> F#; G# + std::pair{ 1, 1 }, // A --> G#; A# + std::pair{ 1, 0 }, // B --> A#; no B# + }; + + const auto neighboringBlackKeys = neighboringKeyMap[whiteKey % Piano::WhiteKeysPerOctave]; + const int offset = adjX - (whiteKey * PW_WHITE_KEY_WIDTH); // mouse X offset from white key + + if (offset < PW_BLACK_KEY_WIDTH / 2) { - --key_num; + // At the location of a (possibly non-existent) black key on the left side + key -= neighboringBlackKeys.first; } - if( key_num < NumKeys - 1 && Piano::isBlackKey( key_num+1 ) && - offset >= ( PW_WHITE_KEY_WIDTH - - PW_BLACK_KEY_WIDTH / 2 ) ) + else if (offset > PW_WHITE_KEY_WIDTH - (PW_BLACK_KEY_WIDTH / 2)) { - ++key_num; + // At the location of a (possibly non-existent) black key on the right side + key += neighboringBlackKeys.second; } + + // For white keys in between black keys, no further adjustment is needed } - // some range-checking-stuff - return qBound( 0, key_num, NumKeys - 1 ); + return std::clamp(key, 0, NumKeys - 1); } @@ -396,12 +391,12 @@ int PianoView::getKeyFromMouse( const QPoint & _p ) const * * We need to update our start key position based on the new position. * - * \param _new_pos the new key position. + * \param newPos the new key position, counting only white keys. */ -void PianoView::pianoScrolled(int new_pos) +void PianoView::pianoScrolled(int newPos) { - m_startKey = static_cast(new_pos / Piano::WhiteKeysPerOctave) - + WhiteKeys[new_pos % Piano::WhiteKeysPerOctave]; + m_startKey = static_cast(newPos / Piano::WhiteKeysPerOctave) + + WhiteKeys[newPos % Piano::WhiteKeysPerOctave]; update(); } From 5725df8082b4cc11a1adb5ec8cd2656ad18d3726 Mon Sep 17 00:00:00 2001 From: Dalton Messmer Date: Sun, 3 Sep 2023 00:56:54 -0400 Subject: [PATCH 3/6] Avoid using invalid iterators in AutomationClip --- src/core/AutomationClip.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/core/AutomationClip.cpp b/src/core/AutomationClip.cpp index 906cb148c82..875ccef44b7 100644 --- a/src/core/AutomationClip.cpp +++ b/src/core/AutomationClip.cpp @@ -345,13 +345,17 @@ void AutomationClip::removeNode(const TimePos & time) cleanObjects(); - m_timeMap.remove( time ); - timeMap::iterator it = m_timeMap.lowerBound(time); - if( it != m_timeMap.begin() ) + m_timeMap.remove(time); + + if (!m_timeMap.isEmpty()) { - --it; + auto it = m_timeMap.lowerBound(time); + if (it != m_timeMap.begin()) + { + --it; + } + generateTangents(it, 3); } - generateTangents(it, 3); updateLength(); @@ -479,7 +483,8 @@ TimePos AutomationClip::setDragValue( } } - this->removeNode(newTime); + removeNode(newTime); + m_oldTimeMap = m_timeMap; m_dragging = true; } From 5e0f86e3fc39637ce3e008931b11467d70d7a550 Mon Sep 17 00:00:00 2001 From: Dalton Messmer Date: Sun, 3 Sep 2023 01:13:29 -0400 Subject: [PATCH 4/6] Fix memory leaks in SimpleTextFloat --- src/gui/widgets/SimpleTextFloat.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gui/widgets/SimpleTextFloat.cpp b/src/gui/widgets/SimpleTextFloat.cpp index df438423e27..e37753229ac 100644 --- a/src/gui/widgets/SimpleTextFloat.cpp +++ b/src/gui/widgets/SimpleTextFloat.cpp @@ -46,11 +46,11 @@ SimpleTextFloat::SimpleTextFloat() : m_textLabel = new QLabel(this); layout->addWidget(m_textLabel); - m_showTimer = new QTimer(); + m_showTimer = new QTimer(this); m_showTimer->setSingleShot(true); QObject::connect(m_showTimer, &QTimer::timeout, this, &SimpleTextFloat::show); - m_hideTimer = new QTimer(); + m_hideTimer = new QTimer(this); m_hideTimer->setSingleShot(true); QObject::connect(m_hideTimer, &QTimer::timeout, this, &SimpleTextFloat::hide); } From d472e1c3f437010b70a12637a5ac485fc2c8913e Mon Sep 17 00:00:00 2001 From: Dalton Messmer Date: Sun, 3 Sep 2023 02:27:01 -0400 Subject: [PATCH 5/6] Handle potential case where QMap::lowerBound(...) returns end iterator --- src/core/AutomationClip.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/core/AutomationClip.cpp b/src/core/AutomationClip.cpp index 875ccef44b7..fe9534fa285 100644 --- a/src/core/AutomationClip.cpp +++ b/src/core/AutomationClip.cpp @@ -347,9 +347,9 @@ void AutomationClip::removeNode(const TimePos & time) m_timeMap.remove(time); - if (!m_timeMap.isEmpty()) + auto it = m_timeMap.lowerBound(time); + if (it != m_timeMap.end()) { - auto it = m_timeMap.lowerBound(time); if (it != m_timeMap.begin()) { --it; @@ -483,8 +483,7 @@ TimePos AutomationClip::setDragValue( } } - removeNode(newTime); - + this->removeNode(newTime); m_oldTimeMap = m_timeMap; m_dragging = true; } From b87ea75e4eee1f6f4b34f52618566166f7b1adc7 Mon Sep 17 00:00:00 2001 From: Dalton Messmer Date: Sun, 3 Sep 2023 16:21:34 -0400 Subject: [PATCH 6/6] Implement suggestions from review --- src/core/AutomationClip.cpp | 41 +++++++++++++------------------------ 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/src/core/AutomationClip.cpp b/src/core/AutomationClip.cpp index fe9534fa285..3b36f6b49b7 100644 --- a/src/core/AutomationClip.cpp +++ b/src/core/AutomationClip.cpp @@ -345,17 +345,13 @@ void AutomationClip::removeNode(const TimePos & time) cleanObjects(); - m_timeMap.remove(time); - - auto it = m_timeMap.lowerBound(time); - if (it != m_timeMap.end()) + m_timeMap.remove( time ); + timeMap::iterator it = m_timeMap.lowerBound(time); + if( it != m_timeMap.begin() ) { - if (it != m_timeMap.begin()) - { - --it; - } - generateTangents(it, 3); + --it; } + generateTangents(it, 3); updateLength(); @@ -1110,16 +1106,16 @@ void AutomationClip::generateTangents(timeMap::iterator it, int numToGenerate) { QMutexLocker m(&m_clipMutex); - if( m_timeMap.size() < 2 && numToGenerate > 0 ) - { - it.value().setInTangent(0); - it.value().setOutTangent(0); - return; - } - - for( int i = 0; i < numToGenerate; i++ ) + for (int i = 0; i < numToGenerate && it != m_timeMap.end(); ++i, ++it) { - if( it == m_timeMap.begin() ) + if (it + 1 == m_timeMap.end()) + { + // Previously, the last value's tangent was always set to 0. That logic was kept for both tangents + // of the last node + it.value().setInTangent(0); + it.value().setOutTangent(0); + } + else if (it == m_timeMap.begin()) { // On the first node there's no curve behind it, so we will only calculate the outTangent // and inTangent will be set to 0. @@ -1127,14 +1123,6 @@ void AutomationClip::generateTangents(timeMap::iterator it, int numToGenerate) it.value().setInTangent(0); it.value().setOutTangent(tangent); } - else if( it+1 == m_timeMap.end() ) - { - // Previously, the last value's tangent was always set to 0. That logic was kept for both tangents - // of the last node - it.value().setInTangent(0); - it.value().setOutTangent(0); - return; - } else { // When we are in a node that is in the middle of two other nodes, we need to check if we @@ -1163,7 +1151,6 @@ void AutomationClip::generateTangents(timeMap::iterator it, int numToGenerate) it.value().setOutTangent(outTangent); } } - it++; } }