Wouldn't it make more sense to add a shared_ptr to AudioProcessorParameters?

Hi JUCE team,

What do you think of replacing

void AudioProcessor::addParameter (AudioProcessorParameter* param)

with

void AudioProcessor::addParameter (std::shared_ptr<AudioProcessorParameter> const&);

and

void AudioProcessor::addParameterGroup (std::unique_ptr<AudioProcessorParameterGroup>);

with

void AudioProcessor::addParameterGroup (std::shared_ptr<AudioProcessorParameterGroup> const&);

(see complete patch for JUCE 7 here: https://user.fm/files/v2-794a93ed96944c53636af1cfe3daed00/0001-Use-shared_ptr-to-add-AudiProcessorParameters.patch)

My personal motivation for doing that is that I’m using my own personal parameter manager and I keep them in structures outside the JUCE classes (e.g. distributed in different classes, passed as arguments to threads, etc). Passing a raw pointer was exposing me to situations where JUCE would destroy the parameter objects before I stop using them and shared_ptr just handles all of this automatically.

I think it would make the code cleaner in general though (std::move is no longer necessary) and save you the energy of explaining that AudioProcessorParameter pointers are captured and deallocated internally by JUCE to a new user. Last but not least it can be implemented with backwards compatibility.

The only con is that, if I’m not mistaken, it may imply one extra reference count than unique_ptr when the plugin initializes its parameters (note addParameter just passes a reference to the shared_ptr) but I think it is well worth it.

Your thoughts?

It makes sense that the AudioProcessor expects to own the parameter objects. In what scenario does JUCE delete parameters while you’re still using them?

In general, I consider shared_ptr to be a bad design choice. It should rarely (if ever) be used.

1 Like

Those parameters may be accessed from very different places such as UI components or separate threads. The point of using shared_ptr is avoiding any risks. It will always be possible to calculate those scenarios but as in with raw pointers, it is better to handle this automatically.

Can you elaborate on that being a bad design? I view C++ as a language that gives you choice instead of forcing to do things in a particular way. shared_ptr in my opinion gives more flexibility. I’m not saying though that I would recommend using it though.

1 Like

I would be particularly concerned about parameters passed to threads that may get stuck e.g. parameters that may require to load a preset file and when that is done change some other parameters (notifying the host). This is by the way something that JUCE can’t handle with the current parameter system. That’s why I have my own.

Those UI components or other threads should never exist or be running once your processor is deleted, though. Nothing that uses an AudioProcessor’s parameters should outlive the AudioProcessor.

I would fix this issue by not passing individual parameter objects to your worker threads. Just create a struct holding all the current values, pass the thread one when it starts, and have the thread return a new one when it’s done. The worker thread itself should not have direct access to actual parameter objects.

shared_ptr introduces global state, and it is very easy to use it incorrectly and introduce ownership deadlocks that lead to memory leaks. shared_ptr has very very few valid use cases, and it is one of the most misunderstood and misused classes in the standard library.

I don’t think any changes to JUCE are the answer to this issue. I think the answer is to rethink your threading model and keep parameter objects themselves private to the AudioProcessor.

1 Like

Wouldn’t a raw pointer even increase that risk of a memory leak? As I said the main problem is that after those threads run after a certain parameter change, some other parameters have to changed, so it is not enough to create a struct with their values: the host needs to be notified of those changes.

Example:
delay plugin
parameter feedback and mix are automatable
parameter preset loads preset files and is also automatable
parameter preset changes and after a heavy preset file is loaded the delay parameters feedback and mix (among others) have to be updated in the host. This has to be done in a separate thread because it is reading from disk.

How would you code this with the current JUCE classes?

  • worker threads runs, has no access to parameter objects
  • when it’s done, it returns the new parameter values to the processor
  • then the processor changes its actual parameter values and updates the host

You are right. Thanks. The resulting code is more complex though and harder to maintain. Do you recommend me any readings about the situations where shared_ptr may introduce ownership deadlocks? I’m not familiar with those.

2 Likes

Appreciate it!

Ok, I understand now. Those reference cycles never occur or are very easy to detect in my usage of shared_ptr (they would produce also a cycle when running jobs after parameter changes). I have other reasons to use shared_ptr, mainly to simplify the code, but I now understand better that it isn’t a design choice without risks.

I still think though it would be nice to have the option to use shared_ptr’s (at our own risk). It’s C++ after all. Passing shared_ptr to addParameter doesn’t necessarily mean you have to pass them to other classes if you deem that to be too risky. But obviously I will have to leave that decision to the JUCE fearless leaders! :wink:

Thank you in any case for considering my suggestion and your valuable comments!