AudioProcessorValueTreeState && thread-safety

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

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

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 …

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

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

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

When you look at this can you also consider what happens with additional data attached to the ValueTree, e.g. a sample file name.

If chkn’s original problem is valid then it would be nice if saving the state in getStateInformation or restoring in setStateInformation was guaranteed not to be properly synchronised with any attempts to change the additional data from the message thread.

1 Like

As long we have a standalone application, all this wouldn’t be a problem. The problem is that as a plugin you never now if the request is done by the message-thread, or from a complete different thread.
The solution would be, that all access to the ValueTree should be secured by the same CriticalSection.
This has the advantge that even other threads can have safe access to the ValueTree.

Either there should be a wrapper that handles this access-policy, and forbids direct access to it, or what i like more, completey ditch the whole ValueTree approach, which IMHO generates more problems than it solves. And undo mechanismen could be implemented differently. (The state-ValueTree could be generated/applied by request)

Arrays can have an optional CriticalSection, but ValueTree not, the problem is that ValueTree can consist out of several ValueTrees, to implement this would be complicated. Maybe like the UndoManager, as extra criticalSection which needs to be parameter for every method call.

2 Likes

Okay - so I’ve been thinking about thread-safety of APVTS in the bath. How does this sound for a strategy … critique welcome… :slight_smile:

setStateInformation()
Check to see if we are on the message thread. If so import the data, otherwise push the received data into a queue, trigger an event to handle it on the message thread and wait for that work to complete before returning.

getStateInformation()
Maintain a copy of the ValueTree (updated via a ValueTree::Listener or ValueTreeSynchronizer?), protected with a lock, that’s then possible to convert the copy to XML and return as a blob without threading issues and without having to wrap the main ValueTree in some kind of lock.

Parameter listeners
The current listeners seem to be called on any old thread. I.e. whatever thread the host decides to call setValue from. Instead these could be called asynchronously on the message thread by hooking into a ValueTree::Listener.

Non-parameter information
This is about how to get information, attached to the APVTS::state ValueTree from the to the audio thread safely. E.g. settings that you don’t want to expose as parameters but which control aspects of processing. I need to have a think about this still, but some kind of interface that looks like the interface for getting parameters but returns something like a thread safe CachedValue.

Little things…
The AudioProcessorValueTreeState uses juce::Atomic which includes a full memory fence can this just use std::atomic::store with std::memory_order_release, which should mean that the parameter is definitely written before the flag is set.

Also, the tutorial obtains the float pointers to the parameter values in processBlock. These I’m guessing would be better as member variables, as otherwise there’s quite a bit of looking stuff up with string comparisons (CharacterFunction::compare is hardly a pretty place to be).

Here’s a quick profile of our simple plugin with a few parameters following the pattern in the tutorial:
image

It would only get worse with lots of parameters with similar names…

Thoughts?

PS. I’ve not thought about this original problem: “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.”

1 Like

This is fine if your parameter listeners are only needed to update the UI, but if you need those callbacks for altering the processing in some other places of your plug-in, you’re going to have big issues during offline bounces/rendering of tracks.

Cakewalk SONAR, for one, completely grabs the message thread during those, and runs the rendering in another thread, so your callbacks will queue up until the end of the offline rendering, when serving them may be too late.

I think two sets of listeners may be needed, one for sync notifications and another for asynchronous ones, or the possibility to specify, when calling addListener(), whether the given listener should be notified synchronously on whatever thread, or possibly asynchronously on the message thread.

Something in that direction was proposed here: Async addition to ListenerList

Ouch - good to know.

Does that mean it may not get a run of the message thread after setStateInformation is called?

This is seeming like a better idea by the minute

1 Like

Well… in the specific context of an offline bounce (let’s call it a “non-realtime render”), I don’t think the DAW has reasons to call setStateInformation(): from the DAW point of view, that has to do with restoring session state and/or recalling previously saved states of the plug-in, which is not something that happens during a render.

On the other hand, you may very well need to call setStateInformation() for your own purposes during a render, for example to react to the reception of a MIDI Program Change, which may be programmed to happen at a given time during your song.
In that case, I confirm that SONAR will not allow the message thread to run anytime soon after you queue that call for async processing.

As a test, I have also tried blocking and waiting after posting one such notification, to see after how long the message thread would have picked it up (with a generous timeout of 10 seconds). I found that sometimes it gets picked up after some hundreds of milliseconds (which still slows things down terribly), but more often than not the message thread doesn’t even pick them up at all within the given timeout.

I think we had a situation with a non-DAW host which was calling setStateInformation right before a render commenced. But I can’t remember the details. I’ll ask Doug who was working on it.

Anyway I’ve half re-written AudioProcessorValueTreeState this morning to take out the ValueTree. Instead it creates one when required:

/**
 * Thread-safe, return the current state of the processor configuration.
 */
ValueTree toValueTree () const;

I’m just redoing the attachment classes. Does anyone understand this self-callback-mutex? Unless I’m mistaken setValue and comboBoxChanged here should only really be called on the message thread anyway, so what’s the lock all about?

struct AudioProcessorValueTreeState::ComboBoxAttachment::Pimpl  : private AttachedControlBase,
                                                                  private ComboBox::Listener
{
    Pimpl (AudioProcessorValueTreeState& s, const String& p, ComboBox& c)
        : AttachedControlBase (s, p), combo (c), ignoreCallbacks (false)
    {
        sendInitialUpdate();
        combo.addListener (this);
    }

    ~Pimpl()
    {
        combo.removeListener (this);
        removeListener();
    }

    void setValue (float newValue) override
    {
        const ScopedLock selfCallbackLock (selfCallbackMutex);

        {
            ScopedValueSetter<bool> svs (ignoreCallbacks, true);
            combo.setSelectedItemIndex (roundToInt (newValue), sendNotificationSync);
        }
    }

    void comboBoxChanged (ComboBox* comboBox) override
    {
        const ScopedLock selfCallbackLock (selfCallbackMutex);

        if (! ignoreCallbacks)
        {
            beginParameterChange();
            setNewUnnormalisedValue ((float) comboBox->getSelectedId() - 1.0f);
            endParameterChange();
        }
    }

    ComboBox& combo;
    bool ignoreCallbacks;
    CriticalSection selfCallbackMutex;

    JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (Pimpl)
};

It would get called synchronously from setValue() above, via ComboBox::setSelectedItemIndex()

Yes, but on the same thread so it won’t lock?

Couldn’t setValue() be called from another thread?

I dont’ think so:

void parameterChanged (const String&, float newValue) override
{
    lastValue = newValue;

    if (MessageManager::getInstance()->isThisTheMessageThread())
    {
        cancelPendingUpdate();
        setValue (newValue);
    }
    else
    {
        triggerAsyncUpdate();
    }
}

The Pimpl structure of these classes really doesn’t help either…

Okay, some work still to do, but something like this: