As it’s stated in the docs, the suspendProcessing method is supposed to be used when a heavy task is to be performed. But this code:
suspendProcessing(true);
// Do something slow...
loadTeraBytesOfSamplesFromAUsb2Stick();
suspendProcessing(false);
causes a priority inversion and renders the whole suspendProcessing feature useless if the call between grabs the callback lock (which it should do for eg. reloading samples).
You should never grab the callback lock for longer than an absolutely minimal time. The whole point of suspending the thread is that it lets you do these longer operations, but regardless of whether it’s suspended, you shouldn’t ever hold the callback lock.
I am just grabbing the lock for deleting the sounds and each time I add a new sound to the synthesiser (after creating them, just for the array.add() method)
However, these run on a lower priority thread and I am getting some priority inversion warnings in my logger once in a while which got me curious.
I thought I was following the approach from JUCE’s own synthesiser class - although now that I am rereading the code, it uses it’s own critical section for changing voices / sounds instead of the global one. Is this the reason for that?
Yes, the actual audio processor callback lock is only to be used as an absolutely last resort, and must never ever held for more than an absolutely trivial, non-blocking operation.
Alright, thanks for clearing that up. I was perhaps a bit too relaxed with throwing around CriticalSections and ScopedLocks. I am not using the getCallbackLock() method directly, so changing this won’t cause too much trouble.
Am I right in assuming that if the global callback lock is held by eg. the message thread, it could cause nasty behavior because the processBlock callback isn’t called and this may cause uninitialized buffer data being passed through the plugin?
If we’re at it, what would you consider as best practice for adding modules (filters, modulators, etc at runtime)?
Use a global CriticalSection (that is not the callback lock) that locks during insertion of the module into the array. Adding these modules is not supposed to happen while normal playback so it is not 100% important that it is super smooth. As long as the processBlock method is being called and has a chance to at least clear the buffers it’s getting from the host?
Add a local CriticalSection that is unique to the holder of the array? This may slow things down when there are many modules as it will call the enterLock function more often for different CriticalSections (I am assuming that acquiring a lock is slower if the current thread doesn’t have it already).
Suspend the processing while inserting and reenable it after the operation? In this case, maybe something like a ScopedSuspender could come in useful:
{
ScopedSuspender ss(processor); // Calls processor->suspendProcessing(true) on creation
// Do some stuff
} // Calls processor->suspendProcessing(false) on deletion
Edit: By global I mean owned by the main processor - not actually being a global variable
Alright, thanks for clearing that up. Come to think of it, the difference between 1. and 3. is rather academic if you use the ScopedTryLock (you’ll fail to enter instead of getting false from suspendProcessing, but the resulting behaviour should be the same).
This might be a starting point for the ScopedSuspender class - it’s not exactly rocket science
/** A simple RAII class that suspends the given AudioProcessor on construction and "unsuspends" it when it goes out of scope.
*
* By using this class you make sure you don't leave your AudioProcessor hanging around being accidently suspended.
*/
class ScopedSuspender
{
public:
ScopedSuspender(AudioProcessor* processorToSuspend) :
p(processorToSuspend)
{
p->suspendProcessing(true);
}
~ScopedSuspender()
{
p->suspendProcessing(false);
};
private:
AudioProcessor* p;
};
I would prefer using a WeakReference as member in case the AudioProcessor gets deleted in the mean time, but the AudioProcessor class doesn’t support WeakReferences yet (as you need to define the friend class stuff in it’s private section).
Better than that, it should be the suspendProcessing() method that cares for reentrancy, so that if you call it twice with true, then you have to call it twice with false to actually resume from suspended state.
In other words, it should internally keep a counter rather than a simple boolean value, and the AudioProcessor should be suspended whenever the counter is bigger than 0
(and assert like crazy in case it gets decremented to a negative value)
