From cd41e31cb5afbc6031edb2011b1b2eae4aa71d02 Mon Sep 17 00:00:00 2001
From: reuk <reuk@users.noreply.github.com>
Date: Mon, 14 Dec 2020 18:15:17 +0000
Subject: [PATCH] DSP:  Ensure that IRs are loaded immediately when Convolution
 is prepared

Previously, if `loadImpulseResponse` was called before `prepareToPlay`,
the IR wasn't guaranteed to have loaded before the first call to
`processSamples`.

Now, we flush the queue of pending IR-load commands during
`prepareToPlay`, which should ensure that the most recently-loaded IR is
ready to use immediately.
---
 .../juce_dsp/frequency/juce_Convolution.cpp   | 27 ++++++-
 .../frequency/juce_Convolution_test.cpp       | 73 +++++++++++++++++--
 2 files changed, 88 insertions(+), 12 deletions(-)

diff --git a/modules/juce_dsp/frequency/juce_Convolution.cpp b/modules/juce_dsp/frequency/juce_Convolution.cpp
index 42b6b6a44a..6fb58f1196 100644
--- a/modules/juce_dsp/frequency/juce_Convolution.cpp
+++ b/modules/juce_dsp/frequency/juce_Convolution.cpp
@@ -86,6 +86,12 @@ public:
     // This function is only safe to call from a single thread at a time.
     bool push (IncomingCommand& command) { return queue.push (command); }
 
+    void popAll()
+    {
+        const ScopedLock lock (popMutex);
+        queue.popAll ([] (IncomingCommand& command) { command(); command = nullptr; });
+    }
+
     using Thread::startThread;
     using Thread::stopThread;
 
@@ -94,13 +100,23 @@ private:
     {
         while (! threadShouldExit())
         {
-            if (queue.hasPendingMessages())
+            const auto tryPop = [&]
+            {
+                const ScopedLock lock (popMutex);
+
+                if (! queue.hasPendingMessages())
+                    return false;
+
                 queue.pop ([] (IncomingCommand& command) { command(); command = nullptr;});
-            else
+                return true;
+            };
+
+            if (! tryPop())
                 sleep (10);
         }
     }
 
+    CriticalSection popMutex;
     Queue<IncomingCommand> queue;
 
     JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (BackgroundMessageQueue)
@@ -892,7 +908,6 @@ public:
     std::unique_ptr<MultichannelEngine> getEngine() { return factory.getEngine(); }
 
 private:
-
     template <typename Fn>
     void callLater (Fn&& fn)
     {
@@ -1017,9 +1032,13 @@ public:
 
     void prepare (const ProcessSpec& spec)
     {
+        messageQueue->pimpl->popAll();
         mixer.prepare (spec);
         engineQueue->prepare (spec);
-        currentEngine = engineQueue->getEngine();
+
+        if (auto newEngine = engineQueue->getEngine())
+            currentEngine = std::move (newEngine);
+
         previousEngine = nullptr;
         jassert (currentEngine != nullptr);
     }
diff --git a/modules/juce_dsp/frequency/juce_Convolution_test.cpp b/modules/juce_dsp/frequency/juce_Convolution_test.cpp
index d7ec4b8198..af93b73702 100644
--- a/modules/juce_dsp/frequency/juce_Convolution_test.cpp
+++ b/modules/juce_dsp/frequency/juce_Convolution_test.cpp
@@ -89,6 +89,19 @@ class ConvolutionTest  : public UnitTest
                 expect (! std::isnan (block.getSample ((int) channel, (int) sample)));
     }
 
+    void checkAllChannelsNonZero (const AudioBlock<float>& block)
+    {
+        for (size_t i = 0; i != block.getNumChannels(); ++i)
+        {
+            const auto* channel = block.getChannelPointer (i);
+
+            expect (std::any_of (channel, channel + block.getNumSamples(), [] (float sample)
+            {
+                return sample != 0.0f;
+            }));
+        }
+    }
+
     template <typename T>
     void nonAllocatingExpectWithinAbsoluteError (const T& a, const T& b, const T& error)
     {
@@ -168,16 +181,21 @@ class ConvolutionTest  : public UnitTest
             }
         };
 
-        const auto time = Time::getMillisecondCounter();
-
-        // Wait 10 seconds to load the impulse response
-        while (Time::getMillisecondCounter() - time < 10'000)
+        // If we load an IR while the convolution is already running, we'll need to wait
+        // for it to be loaded on a background thread
+        if (initSequence == InitSequence::prepareThenLoad)
         {
-            processBlocksWithDiracImpulse();
+            const auto time = Time::getMillisecondCounter();
+
+            // Wait 10 seconds to load the impulse response
+            while (Time::getMillisecondCounter() - time < 10'000)
+            {
+                processBlocksWithDiracImpulse();
 
-            // Check if the impulse response was loaded
-            if (block.getSample (0, 1) != 0.0f)
-                break;
+                // Check if the impulse response was loaded
+                if (block.getSample (0, 1) != 0.0f)
+                    break;
+            }
         }
 
         // At this point, our convolution should be loaded and the current IR size should
@@ -326,6 +344,45 @@ public:
             checkForNans (block);
         }
 
+        beginTest ("Convolutions can cope with a change in samplerate and blocksize");
+        {
+            Convolution convolution;
+
+            auto copy = impulseData;
+            convolution.loadImpulseResponse (std::move (copy),
+                                             2000,
+                                             Convolution::Stereo::yes,
+                                             Convolution::Trim::no,
+                                             Convolution::Normalise::yes);
+
+            const dsp::ProcessSpec specs[] = { { 96'000.0, 1024, 2 },
+                                               { 48'000.0, 512, 2 },
+                                               { 44'100.0, 256, 2 } };
+
+            for (const auto& thisSpec : specs)
+            {
+                convolution.prepare (thisSpec);
+
+                expectWithinAbsoluteError ((double) convolution.getCurrentIRSize(),
+                                           thisSpec.sampleRate * 0.5,
+                                           1.0);
+
+                juce::AudioBuffer<float> thisBuffer ((int) thisSpec.numChannels,
+                                                     (int) thisSpec.maximumBlockSize);
+                AudioBlock<float> thisBlock { thisBuffer };
+                ProcessContextReplacing<float> thisContext { thisBlock };
+
+                nTimes (100, [&]
+                {
+                    addDiracImpulse (thisBlock);
+                    convolution.process (thisContext);
+
+                    checkForNans (thisBlock);
+                    checkAllChannelsNonZero (thisBlock);
+                });
+            }
+        }
+
         beginTest ("Short uniform convolutions work");
         {
             const auto ramp = makeRamp (static_cast<int> (spec.maximumBlockSize) / 2);