Is IComponent::getState()/setState() thread-safe?

Hi,

In Bitwig Studio we use IComponent::getState() from the GUI thread.
But I cam across a crash where IComponent::getState() was concurrent to a program change executed on the Audio thread.

So please, could you guys clarify if getState() and setState() are thread-safe?

Also in general I think that the documentation could be made more clear regarding the threading model and which thread can call which method.

Many thanks,
Alex

Hi
there is in the SDK documention a workflow diagram “Audio Processor Call Sequence”, setState and getState are called normally from the UI Thread when the plugin is used in a realtime context, in an offline context set/getState could be called in the same thread than the process call.

We will add some more explanation in the SDK doc.

Cheers

Thank you very much! :slight_smile:

So if I understand what you are saying getState/setState can potentially being called at the same time as process is called… So should the process method use a lock to ensure that
a) the state is not being modified (via setState) in the middle of processing
b) getState gets a fully valid state

I am not sure how else to protect the integrity of the state if getState/setState are not guaranteed to be called outside of process…

Thanks
Yan

Hi,
please never ever use a lock/mutex or whatever it is called on the audio process thread.
To exchange data to the audio process thread use an atomic value.

Cheers,
Arne

Even with atomic values, if the state is comprised of more than one value, then I just don’t see how you make it thread safe IF there is no guarantee of when getState/setState are being called…

Here is a very simple example in my VST plugin: 2 variables define the state of the plugin. Each variable on its own is probably atomic.

 int fSwitchState; // 0 = A, B = 1
 bool fSoften;

In the process method:

...
  int32 numParamsChanged = inputParameterChanges.getParameterCount();
  for(int i = 0; i < numParamsChanged; ++i)
  {
    IParamValueQueue *paramQueue = inputParameterChanges.getParameterData(i);
    if(paramQueue != nullptr)
    {
      ParamValue value;
      int32 sampleOffset;
      int32 numPoints = paramQueue->getPointCount();

      // we read the "last" point (ignoring multiple changes for now)
      if(paramQueue->getPoint(numPoints - 1, sampleOffset, value) == kResultOk)
      {
        switch(paramQueue->getParameterId())
        {
          case kAudioSwitch:
            fSwitchState = value;
            break;

          case kSoftenSwitch:
            fSoften = value == 1.0;
            break;
        }
      }
    }
  }

And the setState method

  IBStreamer streamer(state, kLittleEndian);

  float savedParam1 = 0.f;
  streamer.readFloat(savedParam1);
  fSwitchState = savedParam1;

  bool savedParam2 = true;
  streamer.readBool(savedParam2);
  fSoften = savedParam2;

And the getState method

  IBStreamer streamer(state, kLittleEndian);
  streamer.writeFloat(fSwitchState);
  streamer.writeBool(fSoften);

If getState is called absolutely any time, then there might be a case when the 2 values are being read/assigned in the process method while they are being written to the streamer… so you could end up with fSwitchState having the new value while fSoften has the old one… and there you go, the state is written invalid. The fact that the 2 values are atomic is not enough.

How do you go about this if you do not use any locks? Or is there some form of guarantee in when and how getState/setState are being called?

Yan

Hi,

struct MyParams {
	double v1;
	bool v2;
};

class Processor : public AudioEffect
{
//....
	std::atomic<MyParams*> setStateParams;
	std::atomic<MyParams*> toDeleteParams;
	MyParams processParams;
};

tresult process (ProcessData& data)
{
	auto stateParams = setStateParams.exchange (nullptr);
	if (stateParams)
	{
		processParams = *stateParams;
		auto test = toDeleteParams.exchange (stateParams);
		assert (test == nullptr);
	}
//....
}

void cleanupMemory()
{
	auto toDelete = toDeleteParams.exchange (nullptr); // cleanup memory
	if (toDelete)
		delete toDelete;
	toDelete = setStateParams.exchange (nullptr); // cleanup memory
	if (toDelete)
		delete toDelete;
}

tresult setState (IBStream* stream)
{
//....
	cleanupMemory ();
	// allocate new state
	auto stateParams = new MyParams;
	// read state from stream into stateParams !
	setStateParams.exchange (stateParams);
}
tresult setActive (bool state)
{
	if(!state)
		cleanupMemory ();
}

Coded out of my head, so typos may very well happened.
I hope this makes it clear.

Cheers,
Arne.

I think I understand the gist. Probably need to do something similar with getState and reading parameter (inputParameterChanges) though

That being said, why is allocating/freeing memory better than using a very short live lock? (playing the devil’s advocate here… but you can implement very efficient locks… and in this case the lock is being held ONLY during the copy of the memory)

Ex:

tresult process (ProcessData& data)
{
   MyParams stateParams;
   withLock {
     stateParams = processParams;
   }
}

tresult setState (IBStream* stream)
{
   MyParams stateParams;
   // read state from stream into stateParams !
   withLock {
     processParams = stateParams;
   }
}

Thanks

Yan

Please watch the good and funny video from Pete Goodliffe about the “The Golden Rules of Audio Programming” @ ADC. Then you hopefully know why you never ever use a lock in a realtime audio thread.

Cheers,
Arne

Based on your code example, I built a couple of abstractions that are thread safe/lock free…

You can check them out there
SingleElementQueue vst-ab-switch/ABSwitchProcessor.h at v1.1.1 · pongasoft/vst-ab-switch · GitHub
AtomicValue vst-ab-switch/ABSwitchProcessor.h at v1.1.1 · pongasoft/vst-ab-switch · GitHub

The SingleElementQueue is being used to communicate the new state to the processor (UI thread calls setState which calls SingleElementQueue::push to push the new state in the queue, and processor calls SingleElementQueue::pop to read it) (vst-ab-switch/ABSwitchProcessor.cpp at v1.1.1 · pongasoft/vst-ab-switch · GitHub)

The AtomicValue is being used to save the new state (processor calls AtomicValue::set / UI thread calls getState which calls AtomicValue::get) (vst-ab-switch/ABSwitchProcessor.cpp at v1.1.1 · pongasoft/vst-ab-switch · GitHub). This can’t use a queue because the value needs to be present whenever getState is called…

Yan

I did watch the video and ironically the section after “never ever use locks” is “never ever allocate memory” in realtime audio thread… (which is something I am particularly familiar with since in the Propellerhead Rack Extension SDK, allocating memory in the realtime audio thread is actually forbidden and will crash the plugin…)

So I guess I am back to my original point/issue. The solution you are suggesting obviously works (I did end up implementing it) but it does allocate and free memory in the realtime audio thread.

In the video, the issue with locks is that they can cause deadlocks, start small and then eventually have a really huge amount of code wrapped in the lock. Which are fine concerns but seems to me that they can be alleviated by good practice… if a lock is only used for copying the state object (easy to encapsulate this by even simply using a std::atomic…) then I don’t see the concern. The memory allocation is a much more serious issue as you can’t control what is going on under the cover… if the implementation uses a lock or need to defragment etc…

Definitely not an easy topic :frowning:

Yan

If you look again on my example, the memory allocations are all done outside the audio thread. I’ve not checked your code, but if you do allocations on the audio thread there then its your implementation you need to change.

Cheers,
Arne

Yes you are correct.

How do you implement getState though? The process methods receives param changes (through IParameterChanges) and changes its state accordingly after which getState is called by the UI thread. How does process writes this state in a thread safe way without using locks or allocating memory?

Thanks
Yan

the setState() is indeed much more complex if you want to get this done right. But most of the time you can just make a copy of your data in the beginning of the setState() method without getting interrupted. That said a correct solution involves a lock free ring buffer, where you push changes from the audio thread and read it back from the main/UI thread.

Cheers,
Arne

Hello Arne

I created a small test case with your exact code (wrapped in a mock processor class to run in a test case) using googletest and which as you pointed out is only doing memory management on the ui thread. Here is the test case:

struct MyParams {
  double v1;
  bool v2;
};

class Processor
{
public:

  std::atomic<MyParams *> setStateParams{nullptr};
  std::atomic<MyParams *> toDeleteParams{nullptr};
  MyParams processParams{0, true};

  ~Processor()
  {
    cleanupMemory();
  }

  void process()
  {
    auto stateParams = setStateParams.exchange(nullptr);
    if(stateParams)
    {
      processParams = *stateParams;
      auto test = toDeleteParams.exchange(stateParams);
      DCHECK_F(test == nullptr);
    }
  }

  void cleanupMemory()
  {
    auto toDelete = toDeleteParams.exchange(nullptr); // cleanup memory
    if(toDelete)
      delete toDelete;
    toDelete = setStateParams.exchange(nullptr); // cleanup memory
    if(toDelete)
      delete toDelete;
  }

  void setState(MyParams const &params)
  {
    cleanupMemory();
    // allocate new state
    auto stateParams = new MyParams{params.v1, params.v2};
    // read state from stream into stateParams !
    setStateParams.exchange(stateParams);
  }
};

TEST(Processor, MultiThreadSafe)
{
  constexpr int N = 10000;

  Processor processor;

  auto ui = [&] {
    for(int i = 0; i < N; i++)
    {
      processor.setState(MyParams{static_cast<double>(i), true});
    }
  };

  auto processing = [&] {
    for(int i = 0; i < N; i++)
    {
      processor.process();
    }
  };

  std::thread uiThread(ui);
  std::thread processingThread(processing);

  uiThread.join();
  processingThread.join();
}

After running a few iterations, the test fails

Running main() from gtest_main.cc
Stack trace:
5       0x7fff59172c5d thread_start + 13
4       0x7fff5917356d _pthread_body + 0
3       0x7fff591736c1 _pthread_body + 340
2          0x10cdf48e5 void* std::__thread_proxy<std::tuple<std::unique_ptr<std::__thread_struct, std::default_delete<std::__thread_struct> >, pongasoft::VST::Test::Processor_MultiThreadSafe_Test::TestBody()::$_5> >(void*) + 5171          0x10cdf4b64 pongasoft::VST::Test::Processor_MultiThreadSafe_Test::TestBody()::$_5::operator()() const + 520          0x10cdf4e25 pongasoft::VST::Test::Processor::process() + 677

2018-06-18 14:17:04.040 (   0.001s) [            A41B]    test-concurrent.cpp:318   FATL| CHECK FAILED:  test == nullptr   
Process finished with exit code 134 (interrupted by signal 6: SIGABRT)

This demonstrates that the code is actually not thread safe even if there is only 1 ui thread calling setState and 1 processing thread calling process…

I know you said “typos may very well happened” but since I have been struggling so much with this, I have been trying to create my own version without any success whatsoever. Do you know have a version of this code that is thread safe that you could point me too?

This seems to be an incredibly complicated problem and I really think that the VST SDK should provide at a minimum a solution for implementing both getState and setState (which have different requirements) in a thread safe, lock free, memory-allocation-on-the-ui-thread-only fashion…

Thanks
Yan

I wanted to close the loop on this topic. Part of the Jamba framework (GitHub - pongasoft/jamba: A lightweight VST2/3 framework) and after spending considerable amounts of time on this issue I believe I finally came up with a solution that is indeed thread safe (as long as there is only 1 consumer and 1 producer) lock free and memory allocation free and also has the advantage of avoiding copies entirely since pointers are exchanged… I know it kinda sounds too good to be true so I certainly hope I did not mess it up :slight_smile:

The code is here jamba/Concurrent.h at v2.0.1 · pongasoft/jamba · GitHub in the LockFree namespace. The 2 main classes are

LockFree::SingleElementQueue and LockFree::AtomicValue

// example

struct Stats {...};

SingleElementQueue<Stats> queue{};

// in 1 thread (ex: RT)
// with copy
Stats stats = computeStats();
queue.push(stats);

// or without copy (use internal pointer)
queue.updateAndPush([this](Stats *oStats) { computeStats(oStats); });

// in another thread (ex: GUI)
// with copy
Stats stats;
if(queue.pop(stats)) { ... };

// or without copy (use internal pointer)
Stats *stats = queue.pop();
if(stats) { ... };

The reason why you can use the internal pointers is because the push and pop side have their own pointers so since each side is called by only 1 thread it is ok to use it…

Although part of Jamba, this file in particular depends on and only so feel free to copy/reuse in your own projects even if you don’t want to use Jamba…

Yan