AudioProcessorValueTreeState && thread-safety

Audio Plugins
1

I’m considering to use AudioProcessorValueTreeState, but the more I look into it, the more it shivers me.
Example (from the tutorial):

https://www.juce.com/doc/tutorial_audio_processor_value_tree_state

    void getStateInformation (MemoryBlock& destData) override
    {
        ScopedPointer<XmlElement> xml (parameters.state.createXml());
        copyXmlToBinary (*xml, destData);
    }

How is thread-safety managed here? The internal-value tree is updated by the message-thread from time to time. What if the host uses another thread to access the current state?

Second problem, what if the internal-parameter value has just been changed, can be guaranteed the next state includes the change?

Another critical problem i see
If the parameter uses an interval, the quantized (stepped) values are stored internally. I think this is problematic, from a hosts perspective, it sets a certain float value, and gets a quantized value back.

2 Likes
2

Would be interesting to hear your opinion on both topics.

3

I think the current mechanism, requires some kind of host behavior, which not at all can be guaranteed.

  • host uses the message thread for getState/setState
  • host waits a while after changing parameters, before it requests a new state
  • host ignores parameter quantization when it sets values.
4

@chkn - did you come to any conclusions here?

5

Also - what thread are the listeners called on? It looks like it’s whatever thread the host calls setParameter from?

6

if the listeners are implemented asynchron, from the message-thread.

My conclusion is not to use it, because it has lots of potential flaws.
One way to “fix” the concurrency if the host access the State, would be to implement a MM-Lock, but this highly dangerous, because of deadlocks.

I would remove any message-thread dependency, and secure access to the ValueTree by a additional Critical-Sections.

1 Like
7

I guess whether this is a problem depends on what the message thread actually does to the ValueTree? The main concern would be anything that adds or removes nodes from it, e.g. loading or saving new preset? Should the ValueTree be exposed as a public member at all here … seems to be the start of trouble?

8

Go on - explain how this might result in a deadlock for me ?

Are you suggesting that the setStateInformation() call should hold the MM lock before writing to the state.

9

What I’m thinking is that it could take the state information, pop it in a queue for the message thread to deal with and block until that action has completed. If it doesn’t block it’ll screw up the timing and there’s a danger of the host getting the wrong settings when working offline (cheers Doug…)

It’d need to check it wasn’t being called on the message thread though - otherwise this approach is going nowhere.

And I guess it could easily deadlock if the host had locked the message thread waiting for this job to complete.

1 Like
10

I don’t think they are called asynchronously. Take this:

void  AudioProcessorValueTreeState::Parameter::setValue (float newValue) override
{
    newValue = range.snapToLegalValue (range.convertFrom0to1 (newValue));

    if (value != newValue || listenersNeedCalling)
    {
        value = newValue;

        listeners.call (&AudioProcessorValueTreeState::Listener::parameterChanged, paramID, value);
        listenersNeedCalling = false;

        needsUpdate.set (1);
    }
}

Which presumably happens on an unspecified thread of the hosts choice? And is the ListenerList<> itself thread-safe? I don’t think so by default

11

Would be good to get a view from the JUCE team on this one … are we mad? :slight_smile:

12

Parameters are Fabian’s speciality, and he’s away this week… Maybe @t0m has an opinion?

1 Like
13

I think these are all legitimate concerns.

The AudioProcessorValueTreeState is due some attention - there’s a few additions and other things to sort out in our backlog.

2 Likes
14

At the very least, as it’s designed to be used in a multi-thread environment the documentation could mention the word ‘thread’, probably more than once :wink:

2 Likes
15

Hey t0m/Fabian - has someone had a think about this yet? :slight_smile:

16

These are all fair points (along with other AudioProcessorValueTreeState issues pointed out on the forum). We’ve been a bit silent on this as we have yet to decide exactly how we want to address these problems.

The goal is to have a “best-practice” sample code on how to deal with parameters and ensure that this approach is thread-safe and interacts well with other JUCE code such as the UndoManager. We are planning to push this out with a point release - either 5.2 or 5.3.

4 Likes
17

In terms of planning here - are you thinking this will be incremental improvement to AudioProcessorValueTreeState rather than something new…?

Just need to make a call about whether we use it for something or revert to our previous conservative but verbose approach …

18

I never realised the benefit to save the exact same information doubled, inside the AudioProcessorParameter and inside a separate ValueTree. I find it better when one information is always stored at one place, so you never have any synchronisation issues. When i need a ValueTree (to save the state) i just created it from there.

PS: Otherwise a ValueTree could be helpful, if JUCE has some kind of high-level ValueTree tools, like thread safe-lock free synchronisation / read-only-shadow-copies which can be read from the audio-thread etc… then yes, a valuetree could be helpful, otherwise no

19

The improvements will definitely be incremental. Maybe there will be some unavoidable breaking changes - but we will try to keep them at a minimum.

1 Like
20

Great to hear that you are working on improvements to the AudioProcessorValueTreeState.
Have you considered allowing the creation of customized parameter attachments? I have requested that earlier here: Please make AudioProcessorValueTreeState::Parameter public!
After all it basically only requires to move the Parameter and AttachedControlBase class definitions over to the header file. As far as I can see that wouldn’t even break any of the existing code.
Even small adjustments to the existing attachments are impossible to make due to the present access restrictions. (like the one discussed here: AudioProcessorValueTreeState::SliderAttachment and RightButtonDown)

2 Likes