I'm having a strange problem with the AudioProcessorGraph. It seems that when I remove a node, sometimes the audio callback is called before AudioProcessorGraph::handleAsyncUpdate. AudioProcessorGraph::processBlock then is still using the old renderingOps, which still contains a processBufferOp for the deleted node, hence crash.
I've never had this problem with any of my other projects, so I'm certain it's my fault, but I can't figure out why. What am I doing wrong?
I am having the same issue one year later. How is AudioProcessorGraph::removeNode supposed to work? I had it working ok, when I removed nodes on the audio thread when I was sure process would not be called, but to avoid memory freeing on the audio thread I would prefer to use a cleanup thread that occasionally destroys unnused nodes.
However I get terrible crashes like that. I looked into how the AudioProcessorGraph removes nodes and I am surprised to see that removeNode immediately destroys the node. Shouldn’t it wait until the GraphRenderingOps is done? I thought the fact AudioGraphProcessor::Node is a reference-counted class would mean, the renderingOps would keep a reference to delay destruction until it is destroyed itself, but that doesn’t seem to be the case. I also see no way to determine whether the node has finished processing from the outside.
How is this supposed to be used? Right now I can only think of waiting until ReleaseResources, but that seems extremely wasteful.
Wow thanks for that (on a sunday morning!). I was indeed not locking the callback lock. I guess I assumed that would happen automagically.
However isn’t the use of a lock for this a big no-no? If I read the code right, once the graph updates, processing cannot continue (due to the lock) until the node is destroyed? The destruction of a Node which includes an AudioProcessor is a very undefined operation that takes an undefined amount of time. I see red flags everywhere :o.
Are you sure that you are locking the callbackLock before you call the AudioProcessorGraph’s processBlock method? As with any AudioProcessor, you must lock the AudioProcessor’s callbackLock before calling processBlock.
If you are locking the callbackLock, then I can’t really see what is wrong with the code. The removeNode call will remove the node from the nodes array but that shouldn’t be a problem as it is a reference counted object. The handeAsyncUpdate will create a new renderingGraph but the old one will still be there. There may be a problem if you manually call releaseResource etc. as there can be a race condition if the AudioProcessorGraph tried to call prepareToPlay on your AudioProcessor which can be called if the callbackLock is not held. However, this prepareToPlay method should normally only be called when the node is added for the first time.
After the new rendering graph is built, the AudioProcessorGraph will lock the callbackLock and swap both graphs.
Yep thanks for all the explanations. Using the callback lock, things obviously run much better now. However I am still getting the occasional crash. I’m really stress testing the graph, creating and removing many nodes many times while audio is running at small buffer sizes.
Sometimes I get a crash during buildRenderingSequence(). My suspection is that it happens in case the nodes array changes while the sequence is built. I see in the debugger that orderedNodes has more members than nodes in that case and the crash is a pure virtual call to a no longer defined AudioProcessor variable.
In my code nodes gets changed from a removeNode(). As I seem to have gathered a few readers here with in-depth knowledge of the AudioProcessorGraph… what can I do to work around this problem? I believe I should avoid calling removeNode() while the graph is rebuilding, but I currently see no way to do that. Is there a way to know a rebuild is currently happening? Shouldn’t nodes be locked (or copied) while the orderedNodes array is built and used? Or… shouldn’t orderedNodes use the ref-count system?
Unless I’m totally mistaken again, the orderedNodes just copies pointers from the ref-counted nodes array.
The only simple-minded solution I see so far would be to delay the removeNode call for some predefined time so I can be sure rebuilding has stopped. Usually this is maybe not a problem because removeNode calls come from the same thread doing the updates, but for me that is not the case.
While I am at it. It would be absolutely awesome if there was a mechanism to delay audio graph rebuilds. Sometimes (on patch changes) I need to do many changes to the graph that all belong together and it would be great if I could tell the graph to stop updating until all the new nodes and connections are queued up so there would for sure only be one graph rebuild at a defined time. Maybe something can be done with the AsyncUpdater and cancelPendingUpdate(), but it is a private parent so that seems to be out of the question without changes to the Juce source code.
It appears my problems are gone once I use an AsyncUpdater to remove Nodes. Instead of removing nodes straight away from a separate thread, I queue them and use triggerAsyncUpdate() to do the real removing. I believe this has the effect that the graph rebuilds and the node removals cannot happen at the same time because they now happen on the same thread which only executes one asyncUpdate at a time. however it does feel clunky. And now I’m starting to wonder whether the same procedure would need to be applied for removing connections. At least there no pointers are involved.
As AudioProcessorGraph is already an AsyncUpdater, wouldn’t it make a lot of sense to handle node and possibly connection removal inside that class in handleAsyncUpdate? Right now I believe the Documentation is lacking vital information about thread safety. If I’m correct, removeNode (and possibly removeConnection) can only be safely called from the message thread. Of course in the plugin host example that happens automatically as they get triggered from user interactions only, but this might not always be the case. I for instants need nodes to go on processing until they faded into a bypass mode, so I can remove them without clicks and therefore I am using a different thread to clean up nodes no longer doing anything.
Sigh… now I got a crash in getNodeById… Is the sensible thing to say any AudioProcessorGraph manipulation is only safe when it’s done on the messagethread because only that guarantees the integrity of the nodes and connections arrays?
I see in that multithreaded AudioProcessorGraph by chkn, he used a reconfigurationLock which probably solves these issues. Seems I’ll have to create my own AudioProcessorGraph for my own needs after all.
In the meantime I did all manipulations on the message thread which helps with the crashes, but leads to undefined behavior for me because the order of things can get messed up. I could do synchronous message thread calls, but I think that’s much worse than a lock performance-wise.
For future reference and in case anyone else runs into the same kinds of problems, here is what I ended up doing.
The situation is I have an AudioProcessorGraph inside my plugin which I use to allow the user to freely connect some modules. The modules are now implemented as AudioProcessors and have the ability to fade in and out of processing so they can be added and removed to the graph without cracks and pops. There is a data-pump thread that removes no longer used modules once they declare they are done fading out.
I use the AudioProcessorGraph as an independent object and initially I forgot to use the callback lock during processing which resulted in lots of bad crashes. But the crashes didn’t fully stop once I used that and I found out the problem is my data pump thread which removes nodes and connections and sometimes creates shortcut connections.
I got issues because the graph updates using the AsyncUpdater class which means the updates run on the juce message thread. If an update collided with my data pump thread undefined things did happen.
I ended up copying the whole source of AudioProcessorGraph to create my own version. In my version I added two critical sections, one for the nodes array and one for the connections array and now these arrays are protected during iteration at all times. I later removed the AsyncUpdater altogether because I heard about problems with the message thread in this forum and now I’m using my data pump thread to do the updates as well which probably means the critical sections don’t ever block.
I also added stuff to my altered class to postpone updates during patch restores where lots of graph chances lead to multiple graph rebuilds in a row.
I haven’t seen a crash in days with these changes.
I think it would be nice to add a sentence to the AudioProcessorGraph description about thread safety of the class. Which is more or less non-existent. As previously stated I believe it only works correctly as long as all graph manipulations come from the message thread which is unfortunate for my use case and is also not at all visible anywhere in the documentation.
