Possible race condition?

[lucianoiam]

Isn't there a risk of writing to an invalid location if the UI was closed exactly at this point? and hence the shared memory referenced by FloatFifoControl, deleted.

OneKnob-Series/plugins/common/OneKnobPlugin.hpp

Line 139 in 26ca598

[falkTX]

Should be fine, the DSP side is the owner/server of the shared memory, the UI side simply connects to it.
When the UI is closed, IIRC it just disconnects its side of the shared memory "file". It is still kept in memory due to it being originally created by the DSP side.

the code for this is in https://github.com/DISTRHO/OneKnob-Series/blob/main/plugins/common/SharedMemory.hpp
the DSP side calls create, while UI side calls connect.

I have been using this same approach for plugin bridges in carla, no issues in there so far.

[lucianoiam]

I agree that is the right approach, but this part got me confused

OneKnob-Series/plugins/common/OneKnobUI.hpp

Line 311 in 7951033

if (! lineGraphsData.create())

Isn't the UI controlling the shared memory lifetime in this case?

[falkTX]

Indeed, I have the logic for this wrong.
The case of

OneKnob-Series/plugins/common/OneKnobPlugin.hpp

Line 79 in 7951033

if (std::strcmp(key, "filemapping") == 0)

should be the other way around.

I was trying to avoid creating a shm segment if the UI is never open, but this is the wrong approach for sure.
Might need to add some new plugin callbacks to inform of (possibly remote) UI open and close.

[lucianoiam]

I stumbled upon this scenario independently, I could not find a way to make the Plugin own the shared memory because that would require notifying the UI about the filename. AFAIK state updates are only possible in the UI->Plugin direction and not the other way around, unless the filename is set as a defaultStateValue. But defaultStateValue is a no-go because on VST3 the Plugin constructor runs 3 times and UI gets the defaultStateValue from instance no. 2, which is not finally the one running the DSP. That hack works with CLAP though.

BTW is it by design that VST3 creates multiple Plugin instances?

I thought finishing Plugin::updateStateValue() could help solving this problem. It is currently marked as TODO API under construction.

[falkTX]

AFAIK state updates are only possible in the UI->Plugin direction and not the other way around

This is being worked on in develop branch, and already in use for Cardinal.
There is a way to send messages from DSP to UI, with the only caveat that you cant do that during run().
That is the updateStateValue stuff. set as WIP because I didnt validate it on all formats yet.

BTW is it by design that VST3 creates multiple Plugin instances?

Yes, unless you set instance-access to 1. In order to get proper DSP/UI separation for VST3, due to their weird component vs controller vs view architecture, we need to create 2 DSP objects. 1 is associated with the component, the other with the controller. then view gets the UI stuff.
The VST3 separation of component and controller is a super weird one.. so it needed a little forcing to be usable for DPF.

[lucianoiam]

Cool, will give updateStateValue a try and report on the DPF project if needed.
I could guess it was something related to the Very Sophisticated Technology
Thanks

[lucianoiam]

This synchronization pseudocode could be helpful until the missing state method is completed, it relies on a std::atomic_flag. It works nice for my project:

#include <atomic>

// UI side (shmem owner)

SomeUI::~SomeUI()
{
    setState("shared_mem", "the_owner_will_destroy_it");  // blocks if needed

    fSharedMemory.close();
}

// Plugin side (shmem client)

SomePlugin::setState(const char* key, const char* value)
{
    if (std::strcmp("shared_mem", "the_owner_will_destroy_it") == 0) {

        if (fShMemIsBusy.test_and_set()) {
            // Flag was already true, the shared memory is busy.
            // Wait for the current render cycle to release it.

            while (fShMemIsBusy.test_and_set()) {};

            // Flag cleared, now set again to true, free to proceed.

        } else {
            // Flag was false but now is true, free to proceed.
        }

        // Do the dangerous business
        fSharedMemory.close();

        // Done
        fShMemIsBusy.clear();

    }
}

void SomePlugin::run(const float** inputs, float** outputs, uint32_t frames)
{
    if (fShMemIsBusy.test_and_set()) {
        // Shared memory is being closed

    } else {

        // Safe to access shared memory
        if (fSharedMemory.isCreatedOrConnected()) {

           ...
        
        }

        // Release
        fShMemIsBusy.clear();
    }

    ...
}

EDIT : updated the state value string for clarity

[falkTX]

What is fShMemIsBusy ? an std::atomic? afaik that is not a POD struct, so it is not suitable for shared memory.
I will anyhow do this in an alternative approach. but the OneKnob Series project does not have a release date, so I am not in a rush

[lucianoiam]

Yes, it is a member of the Plugin instance, the snippet is not clear in that sense but does not enforce the shared memory to hold any data in particular.

[lucianoiam]

class SomePlugin : public Plugin
{
   ...

private:
   SharedMemory<...> fSharedMemory;
   std::atomic_flag  fShMemIsBusy;

   ...
};

[falkTX]

right, so for testing that could work in theory but I advise to NOT use anything with constructors and other stuff when going with shared memory.
best to rely on a simple int and use GCC/clang internals to do atomic operations on it.

if you see how the float-fifo thing works on this repo, there is a clear separation between POD data and the C++ class that controls it. so we can have the data/buffer on shared memory, but still have conveniences of C++ working on top of it.

[lucianoiam]

Do you mean the atomic_flag there could pose a risk? its lifetime is bound to the Plugin instance, and only Plugin has access to it.

In my project, there is a VisualizationData class and the plugin uses placement new to instantiate it in a shared memory segment. This class does all the plumbing regarding visualization data: one RT method pushes data, another non-RT pops. It is high level on purpose, implements both data and logic, and as such requires to have some non-POD members. Can you think of any drawbacks? this is the first time I try this design and it works nice, but...

Anyways, regardless the logic for accessing the shared memory, the key challenge seems to be making sure Plugin does not attempt to access invalid memory access shared memory while it is being destroyed, and the only solutions I can think of are either moving ownership of the shared memory to the Plugin (IMO the "natural" approach), or implementing synchronization (unnecessarily complicated). I am not fond of the latter but avoiding DISTRHO_PLUGIN_WANT_DIRECT_ACCESS is an interesting hobby 😆

[falkTX]

In my project, there is a VisualizationData class and the plugin uses placement new to instantiate it in a shared memory segment. This class does all the plumbing regarding visualization data: one RT method pushes data, another non-RT pops. It is high level on purpose, implements both data and logic, and as such requires to have some non-POD members. Can you think of any drawbacks? this is the first time I try this design and it works nice, but...

I can think of cases where UI and DSP do not match in terms of build options, packing or even architecture, and thus the non-POD data would not match.
But those are a bit exotic scenarios.

For Carla I have plugin bridges with different architectures, so the shm data must match between them, which poses a small headache at times.

[lucianoiam]

That is a good point to keep in mind, thanks.