LoopingMidiNode: Fixed a memory corruption when clipping a sequence t…

…o a loop range
Tracktion · Jul 3, 2023 · 7e2ff04 · 7e2ff04
1 parent 6ac3059
commit 7e2ff04
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 3 deletions.
diff --git a/modules/tracktion_engine/playback/graph/tracktion_LoopingMidiNode.cpp b/modules/tracktion_engine/playback/graph/tracktion_LoopingMidiNode.cpp
@@ -664,13 +664,17 @@ namespace MidiHelpers
         if (clipRange.isEmpty())
             return;
 
+        // Use a special number that won't be in use to signify an event to remove
+        // We have to do it like this to avoid allocating a new sequence
+        constexpr auto timeStampToRemoveFlag = std::numeric_limits<double>::min();
+
         // First adjust all the note times
         for (auto& m : sequence)
             if (m.message.isShortMessage())
                 if (auto sm = m.message.getShortMessage(); sm.isNoteOn() || sm.isNoteOff())
                    m.timeStamp = clipRange.clipValue (m.timeStamp);
 
-        // Then remove any zero or negative length notes
+        // Then change the timestamps of an zero or negative length notes
         for (int i = (int) sequence.events.size(); --i >= 0;)
         {
             const size_t index = static_cast<size_t> (i);
@@ -694,10 +698,15 @@ namespace MidiHelpers
                 if (noteLength > 0.0)
                     continue;
 
-                sequence.events.erase (sequence.events.begin() + static_cast<int> (*noteOffIndex));
-                sequence.events.erase (sequence.events.begin() + i);
+                sequence.events[*noteOffIndex].timeStamp    = timeStampToRemoveFlag;
+                sequence.events[index].timeStamp            = timeStampToRemoveFlag;
             }
         }
+
+        // Finally, erase any events with the flagged timestamp
+        sequence.events.erase (std::remove_if (sequence.events.begin(), sequence.events.end(),
+                                               [] (const auto& e) { return juce::approximatelyEqual (e.timeStamp, timeStampToRemoveFlag); }),
+                               sequence.events.end());
     }
 }
 

diff --git a/modules/tracktion_engine/playback/graph/tracktion_LoopingMidiNode.test.cpp b/modules/tracktion_engine/playback/graph/tracktion_LoopingMidiNode.test.cpp
@@ -45,6 +45,8 @@ class LoopingMidiNodeTests : public juce::UnitTest
 
         runProgramChangeTests (false);
         runProgramChangeTests (true);
+
+        runSequenceClippingTests();
     }
 
 private:
@@ -229,6 +231,74 @@ class LoopingMidiNodeTests : public juce::UnitTest
         }
     }
 
+    void runSequenceClippingTests()
+    {
+        beginTest ("Clipping sequence to range");
+
+        struct BytesAndTimeStamp
+        {
+            uint8_t bytes[3];
+            double timestamp;
+        };
+
+        BytesAndTimeStamp data[] = {
+          { { 0x90, 0x4e, 0x7c }, 12.0 },
+          { { 0x90, 0x42, 0x7c }, 12.0 },
+          { { 0x90, 0x49, 0x7c }, 12.0 },
+          { { 0x80, 0x42, 0x00 }, 12.4354 },
+          { { 0x80, 0x49, 0x00 }, 12.4578 },
+          { { 0x80, 0x4e, 0x00 }, 12.4764 },
+          { { 0x90, 0x4e, 0x7c }, 14.5 },
+          { { 0x90, 0x49, 0x7f }, 14.5 },
+          { { 0x90, 0x42, 0x7c }, 14.5 },
+          { { 0x80, 0x4e, 0x00 }, 14.9335 },
+          { { 0x80, 0x42, 0x00 }, 14.9519 },
+          { { 0x80, 0x49, 0x00 }, 14.9752 },
+          { { 0x90, 0x42, 0x7b }, 15.5 },
+          { { 0x80, 0x42, 0x00 }, 15.9744 },
+          { { 0x90, 0x4e, 0x7b }, 16.0 },
+          { { 0x90, 0x49, 0x7b }, 16.0 },
+          { { 0x80, 0x49, 0x00 }, 17.0 },
+          { { 0x80, 0x4e, 0x00 }, 17.0312 },
+          { { 0x90, 0x42, 0x7b }, 20.0 },
+          { { 0x80, 0x42, 0x00 }, 20.3751 },
+          { { 0x90, 0x49, 0x7b }, 22.0 },
+          { { 0x90, 0x42, 0x7a }, 22.0 },
+          { { 0x90, 0x4e, 0x7b }, 22.5 },
+          { { 0x80, 0x42, 0x00 }, 22.5101 },
+          { { 0x80, 0x49, 0x00 }, 22.5207 },
+          { { 0x80, 0x4e, 0x00 }, 23.0279 },
+          { { 0x90, 0x4e, 0x79 }, 24.0 },
+          { { 0x90, 0x42, 0x7a }, 24.0 },
+          { { 0x90, 0x49, 0x7a }, 24.0 },
+          { { 0x80, 0x49, 0x00 }, 24.1976 },
+          { { 0x80, 0x42, 0x00 }, 24.2033 },
+          { { 0x80, 0x4e, 0x00 }, 24.2284 } };
+
+        choc::midi::Sequence seq;
+
+        for (auto d : data)
+            seq.events.push_back ({ d.timestamp, choc::midi::ShortMessage (d.bytes[0], d.bytes[1], d.bytes[2]) });
+
+        {
+            std::vector<std::pair<size_t, size_t>> noteOffMap;
+            MidiHelpers::createNoteOffMap (noteOffMap, seq);
+
+            const juce::Range clipRange (12.0, 24.0);
+            MidiHelpers::clipSequenceToRange (seq, clipRange, noteOffMap);
+
+            expectEquals (seq.events.size(), static_cast<size_t> (26));
+
+            bool allEventsWithinRange = true;
+
+            for (auto e : seq)
+                if (! clipRange.contains (e.timeStamp))
+                    allEventsWithinRange = false;
+
+            expect (allEventsWithinRange, "Not all events within the expected range");
+        }
+    }
+
     void runOffsetTests (test_utilities::TestSetup ts)
     {
         beginTest ("MIDI clip with offset");