AudioProcessorValueTreeState && thread-safety


#21

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.


#22

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.


#23

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.”


#24

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


#25

Ouch - good to know.

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


#26

This is seeming like a better idea by the minute


#27

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.


#28

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)
};

#29

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


#30

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


#31

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


#32

I dont’ think so:

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

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

#33

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


#34

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


#36

I’d like to know how things have progressed and whether it’s worth going with AudioProcessorValueTreeState


#37

We are still working on this. That’s all I can say, sorry!


#38

Considering AudioProcessorValueTreeState has issues, what’s the currently recommended way to manage AudioProcessorParameters in a thread-safe manner in an audio plug-in?


#39

you could just use AudioProcessorParameterWithID or, some of its derived classes AudioParameterFloat/Bool/Choice like the DSP Demo


#40

Hi Fabian. Could this be helpful as a starting point? https://github.com/getdunne/juce-AudioProcessorValueTreeStateTest
I can’t claim to be an expert, but I’m also very interested to create “best practice” samples for JUCE. With so few available it’s a tough learning curve.


#41

I pushed some functionality that addresses these concerns a few days ago:

The (get/set)StateInformation functions should now look like

    void getStateInformation (MemoryBlock& destData) override
    {
        auto state = parameters.copyState();
        ScopedPointer<XmlElement> xml (state.createXml());
        copyXmlToBinary (*xml, destData);
    }
    
    void setStateInformation (const void* data, int sizeInBytes) override
    {
        ScopedPointer<XmlElement> xmlState (getXmlFromBinary (data, sizeInBytes));
        
        if (xmlState != nullptr)
            if (xmlState->hasTagName (parameters.state.getType()))
                parameters.replaceState (ValueTree::fromXml (*xmlState));
    }

and we’ll update the tutorial once this change makes it to the master branch.