Oscillators: Fixed the data race on the static juce::Random shared by start()

modules/tracktion_core/tracktion_TestConfig.h

@@ -49,6 +49,7 @@
 #define ENGINE_UNIT_TESTS_LOOP_INFO                     1
 #define ENGINE_UNIT_TESTS_MIDILIST                      1
 #define ENGINE_UNIT_TESTS_MODIFIERS                     1
+#define ENGINE_UNIT_TESTS_OSCILLATORS                   1
 #define ENGINE_UNIT_TESTS_PAN_LAW                       1
 #define ENGINE_UNIT_TESTS_PATCHBAY                     1
 #define ENGINE_UNIT_TESTS_PLAYBACK                      1

modules/tracktion_engine/tracktion_engine_utils.cpp

@@ -79,6 +79,7 @@ extern "C" char MacGetMacFSRefForREXDLL (FSRef* fsRef)
 #include "utilities/tracktion_Envelope.cpp"
 #include "utilities/tracktion_FileUtilities.cpp"
 #include "utilities/tracktion_Oscillators.cpp"
+#include "utilities/tracktion_Oscillators.test.cpp"
 #include "utilities/tracktion_PropertyStorage.cpp"
 #include "utilities/tracktion_ParameterHelpers.cpp"
 #include "utilities/tracktion_UIBehaviour.cpp"

modules/tracktion_engine/utilities/tracktion_Oscillators.cpp

@@ -60,8 +60,7 @@ static float sawDown (float phase, float freq, double sampleRate)
 //==============================================================================
 void Oscillator::start()
 {
-    static juce::Random r;
-    phase = r.nextFloat();
+    phase = random.nextFloat();
 }
 
 void Oscillator::setSampleRate (double sr)
@@ -203,11 +202,9 @@ MultiVoiceOscillator::MultiVoiceOscillator (int maxVoices)
 
 void MultiVoiceOscillator::start()
 {
-    static juce::Random r;
-
     for (int i = 0; i < oscillators.size(); i += 2)
     {
-        float phase = r.nextFloat();
+        const float phase = random.nextFloat();
         oscillators[i + 0]->start (phase);
         oscillators[i + 1]->start (phase);
     }

modules/tracktion_engine/utilities/tracktion_Oscillators.h

@@ -77,6 +77,7 @@ class Oscillator
 
     BandlimitedWaveLookupTables::Ptr lookupTables;
 
+    juce::Random random;
     std::default_random_engine generator;
     std::normal_distribution<float> normalDistribution {0.0f, 0.1f};
 };
@@ -103,6 +104,7 @@ class MultiVoiceOscillator
 
 private:
     juce::OwnedArray<Oscillator> oscillators;
+    juce::Random random;
 
     int voices = 1;
     float detune = 0, spread = 0, gain = 1.0f, note = 69.0f, pan = 0.0f;

modules/tracktion_engine/utilities/tracktion_Oscillators.test.cpp

@@ -0,0 +1,153 @@
+/*
+    ,--.                     ,--.     ,--.  ,--.
+  ,-'  '-.,--.--.,--,--.,---.|  |,-.,-'  '-.`--' ,---. ,--,--,      Copyright 2024
+  '-.  .-'|  .--' ,-.  | .--'|     /'-.  .-',--.| .-. ||      \   Tracktion Software
+    |  |  |  |  \ '-'  \ `--.|  \  \  |  |  |  |' '-' '|  ||  |       Corporation
+    `---' `--'   `--`--'`---'`--'`--' `---' `--' `---' `--''--'    www.tracktion.com
+
+    Tracktion Engine uses a GPL/commercial licence - see LICENCE.md for details.
+*/
+
+#if TRACKTION_UNIT_TESTS && ENGINE_UNIT_TESTS_OSCILLATORS
+
+#include <tracktion_engine/../3rd_party/doctest/tracktion_doctest.hpp>
+
+#include <atomic>
+#include <set>
+#include <thread>
+#include <vector>
+
+namespace tracktion { inline namespace engine
+{
+
+TEST_SUITE ("tracktion_engine")
+{
+    //==============================================================================
+    static float getFirstSample (Oscillator& o)
+    {
+        juce::AudioBuffer<float> buffer (1, 1);
+        buffer.clear();
+        o.process (buffer, 0, 1);
+        return buffer.getSample (0, 0);
+    }
+
+    static float getFirstSample (MultiVoiceOscillator& o)
+    {
+        juce::AudioBuffer<float> buffer (2, 1);
+        buffer.clear();
+        o.process (buffer, 0, 1);
+        return buffer.getSample (0, 0);
+    }
+
+    template<typename OscillatorType>
+    static OscillatorType createOscillator();
+
+    template<>
+    Oscillator createOscillator<Oscillator>()                     { return {}; }
+
+    template<>
+    MultiVoiceOscillator createOscillator<MultiVoiceOscillator>()  { return MultiVoiceOscillator (1); }
+
+    template<typename OscillatorType>
+    static std::set<float> getFirstSamplesAfterRepeatedStarts (int numStarts)
+    {
+        auto o = createOscillator<OscillatorType>();
+        o.setSampleRate (44100.0);
+        o.setWave (Oscillator::sine);
+
+        std::set<float> firstSamples;
+
+        for (int i = 0; i < numStarts; ++i)
+        {
+            o.start();
+            firstSamples.insert (getFirstSample (o));
+        }
+
+        return firstSamples;
+    }
+
+    template<typename OscillatorType>
+    static std::set<float> getFirstSamplesOfFreshInstances (int numInstances)
+    {
+        std::set<float> firstSamples;
+
+        for (int i = 0; i < numInstances; ++i)
+        {
+            auto o = createOscillator<OscillatorType>();
+            o.setSampleRate (44100.0);
+            o.setWave (Oscillator::sine);
+            o.start();
+            firstSamples.insert (getFirstSample (o));
+        }
+
+        return firstSamples;
+    }
+
+    //==============================================================================
+    TEST_CASE ("Oscillators: start() picks a random phase")
+    {
+        // Each start() should draw a new phase, and each instance should seed
+        // its own generator so fresh instances don't all start from the same phase.
+        // Instance counts are kept low as each setSampleRate() builds a fresh set
+        // of bandlimited lookup tables, which is slow in Debug builds
+        SUBCASE ("Oscillator")
+        {
+            CHECK (getFirstSamplesAfterRepeatedStarts<Oscillator> (8).size() > 1);
+            CHECK (getFirstSamplesOfFreshInstances<Oscillator> (4).size() > 1);
+        }
+
+        SUBCASE ("MultiVoiceOscillator")
+        {
+            CHECK (getFirstSamplesAfterRepeatedStarts<MultiVoiceOscillator> (8).size() > 1);
+            CHECK (getFirstSamplesOfFreshInstances<MultiVoiceOscillator> (4).size() > 1);
+        }
+    }
+
+    TEST_CASE ("Oscillators: start() on separate instances from concurrent threads")
+    {
+        // Regression test for #400. start() used to draw its phase from a
+        // function-local static juce::Random shared by every instance in the
+        // process, so voices started at the same time on different audio threads
+        // raced on its seed. Each thread here owns its own oscillators so nothing
+        // should be shared between them. The race is silent without
+        // ThreadSanitizer, which CI runs this under.
+        constexpr int numThreads = 8;
+        constexpr int numStarts = 1000;
+
+        std::atomic<int> numReady { 0 };
+        std::vector<int> numStartsCompleted ((size_t) numThreads, 0);
+        std::vector<std::thread> threads;
+
+        for (int t = 0; t < numThreads; ++t)
+        {
+            threads.emplace_back ([&, t]
+            {
+                MultiVoiceOscillator multi;
+                Oscillator single;
+
+                ++numReady;
+
+                while (numReady < numThreads)
+                    std::this_thread::yield();
+
+                for (int i = 0; i < numStarts; ++i)
+                {
+                    multi.start();
+                    single.start();
+                }
+
+                numStartsCompleted[(size_t) t] = numStarts;
+            });
+        }
+
+        for (auto& t : threads)
+            t.join();
+
+        for (auto n : numStartsCompleted)
+            CHECK_EQ (n, numStarts);
+    }
+}
+
+}} // namespace tracktion { inline namespace engine
+
+#endif // ENGINE_UNIT_TESTS_OSCILLATORS