How many times you call it really depends on context.
For example, if you have a synth with 64 voices, each calling a chain of processors and not a single function, you’ll interact with that atomic quite a lot.
That atomic is accessed in every single AudioBuffer function, like applyGain(), getArrayOfWritePointers(), addFrom(), etc. And even functions that are meant to be called per sample like setSample().
I agree that in many cases the impact on your speed would be minimal, but there are other cases I ran into, like a long chain of nested processes, where it’s not.
That’s ok if you just use the buffer to take pointers, otherwise the flag is loaded and stored everywhere: in applyGain[Ramp], in add/setSample, in addFrom, copyFrom, findMindMax, getMagnitude… pretty much everywhere. I don’t think it’s fair to dismiss this with a generic “go profile and come back” -the impact would be completely different for each case. The question is whether the atomic is necessary or not.
If you write in different T** positions from different threads, there shouldn’t be any problems (but there will if you write in the same positions!). The only thing they have in common is the isClear flag, that’s the reason it’s atomic I think
However I don’t see many of us writting in this from multiple threads, that why I’m using an AudioBuffer copy named LightAudioBuffer, replacing its atomic flag by a normal boolean. But this would be better if I could specify with a template parameter if I want an atomic flag or an simple bool.
If you do that, and pass a reference to that AudioBuffer to different threads, you’ll run into more complicated data races with that atomic. I don’t think it solves that problem. The only problem it solves is TSAN getting angry at you for sharing AudioBuffers, and it was angry for a good reason!
Template parameter implies that’s a feature you’d want. I think it’s not a feature you should ever want. Instead, you should use a better mechanism to share AudioBuffers, like FIFOs, or passing a T** when you know you’ll write to different sections, etc.
Not sure I understand you correctly, but I agree with you that limiting ValueTree to access from the message thread only, by default, would be great and eliminate a bunch of bugs.
Yeah, that’s what I suspected. TSan got angry at this example for a good reason. I think it would have been better to rewrite the example and use proper syncing instead of locking + touching all kinds of shared state from different threads.
I understand you. I would never share an AudioBuffer betweens threads. But that atomic is there for that reason. That’s why I would replace it with a simple bool. But my guess is that someone are filling the different channels from different threads (ie, from track1 thread → channel 0,1 / from track 2 thread → channel 2,3… and so on).
ValueTree uses a ReferenceCountedObject, which increases a decreases an atomic value. If the purpose of the ValueTree is being used in the Message thread, I don’t see why should we have an atomic there. Using the light version of SingleThreadedReferenceCountedObject should increase performance without sacrifying anything. I’m not sure if ValueTree continue using an atomic - I’ll check
Yeah, so if I was that person, I think I’d make it clear that in this case you have to pass the internal T** and manage the chunks at your own risk, as calling high level AudioBuffer functions will create data races even with the atomic…
Imagine this:
//Thread 1:
void setSample (int destChannel, int destSample, Type newValue) noexcept
{
*(channels[destChannel] + destSample) = newValue;
//Thread 2 calls clear() here, thinking it's already clear, does nothing instead
isClear = false;
//Buffer will never be cleared unless we add yet another explicit call to clear()
}
That’s true, but this is also not meant to be used for anything performance related, and it’s possible I might pass the refcount to a different thread as a messaging system for example. So I can see the use cases for an atomic (where I can’t with AudioBuffer).
Yes, I agree that using something like AudioBlock that just wraps the direct T** for all future calls will indeed be much faster and will solve the problem for a local function.
But unfortunately there’s so much existing code in the library that already uses AudioBuffer, which that atomic is a negative impact on.
Apologies for taking a while to respond to this, I wanted to get the rest of the team’s input before making any changes.
The original forum thread that lead to this change was here:
and the “fix” that I pushed was to make the AudioBuffer::isClear flag atomic which silenced tsan but clearly had some unintended side effects.
Given that AudioBuffer makes no guarantees about thread safety and contains no other synchronisation I don’t think it should be the responsibility of the class to be thread safe. Making isClear atomic also doesn’t make it thread safe since some higher level synchronisation would be required to actually guard the clearing and filling of the audio data, it just makes tsan happy.
I’ve reverted that change and made isClear a plain old bool here:
and I’ve pushed a fix for the actual problem in BufferingAudioSource here:
My issue remains. If the name of the function is clear(), then it should just clear…and not depend on the state some of some flag that’s not documented. This function currently assumes that the programmer is using setSample() or similar. What if someone isn’t using setSample() and instead is getting writePointers and using them (very common), and wants to clear the buffer? It doesn’t work because of this flag.
I’d even venture to say more programmers use getWritePointer() and access samples that way rather than setSample(), but I could be wrong.
Maybe add a clear(bool force = false) so it can overlook the flag?
At the very least, add something to the documentation. I know I’ve spent more than a few minutes several times wondering why a damn buffer wouldn’t clear.
Well, when I stated the problem I wasn’t using the setSample() at all. Instead I was taking sections from the buffer using getWritePointer and getArrayOfWritePointers() and wrapping them inside an AudioBlock or writting directly in it. But takings hundreds of sections from dozens of AudioBuffer was expensive.
And to be honest, I didn’t understand your problem, could you explain it further? When you take an audio section using getWritePointer(), the flag automatically becomes true, even if you don’t write nothing. So as soon as you call clear(), the flag will set off, and the buffer will be actually cleared.
But if you only get the write pointer in prepareToPlay() and not every processBlock(), the flag was only set that one time and not on subsequent calls.
This flag makes an assumption on how the buffer is used, but it’s an odd assumption and again…it’s not documented.
To me, it’s fairly normal to create a buffer, resize it and get the writePointers only during prepareToPlay() and keep the pointers for use in processBlock(). They haven’t changed (except in prepareToPlay) so why get them again? And who knows when I may want to clear a buffer? That’s up to me.
HeapBlock::clear() doesn’t check a flag. Nor does FloatVectorOperations::clear(), or MidiBuffer::clear().
Again, if it’s called clear(), it should just clear and not check anything.
