I have a loop where quite a lot of AudioFormatReader objects are created using AudioFormatManager.createReaderFor(InputStream). The docs for that method says:
If it returns a reader, it’s the caller’s responsibility to delete the reader.
But if I delete the reader, either explicitly using delete or by wrapping it in a ScopedPointer, the program will crash. If I omit that part, everything appears to work fine.
Although I’m a bit concerned that there’s a memory leak, since the docs explicitly tell me to delete it?
Depends what you are doing with the reader. If you are using it e.g. in an AudioFormatReaderSource, that will delete it on destruction (if you set the bool flag deleteReaderWhenThisIsDeleted to true).
Also the original poster might be putting them into an OwnedArray or such, which would take care of the deletion too. It would be great if people actually just posted the code they have written instead of explaining just a small portion of it…
My code goes through a ZipFile of wav files and stores each of them as SamplerSound objects in an unordered_map, with the name as key.
Here’s my loop that goes through the ZipFile entries:
for (int i = 0; i < numEntries; i++) {
const ZipFile::ZipEntry *entry = zipFile.getEntry(i);
String name = entry->filename;
if (name.endsWith(".wav") && samples[name] == nullptr) {
InputStream *input = zipFile.createStreamForEntry(i);
AudioFormatReader *reader = formatManager.createReaderFor(input);
SamplerSound *sampSound = new SamplerSound("", *reader, noteRange, 0, 0.0, 0.0, 30.f);
samples[name] = sampSound;
}
}
According to the docs for SamplerSound, it says the following about the AudioFormatReader argument to the constructor:
This object can be safely deleted by the caller after this constructor returns
So this in combination with the docs for AudioFormatManager made me believe that I have to dispose of the AudioFormatReader myself. But if I do that in any of the ways mentioned above, the plugin host will crash.
That is indeed unexpected. A look at the sources shows, that the reader is just used in the constructor to read into an AudioSampleBuffer and is after that discarded, so there is no double ownership.
What could have happened, since you probably deleted the InputStream as well, that you didn’t reverse the order, accidently deleting the InputStream before the reader?
Yeah, I try to avoid using delete as well, I just tried it now to see if it would make a difference.
My code seems to be working now! I must have messed something up, maybe I deleted the InputStream by mistake (I seem to remember wrapping it in a ScopedPointer at some point)? That would certainly cause a crash when deleting the AudioFormatReader, since it deletes the InputStream in its destructor…
Also you might need to check for null. There might be occasion when you get reader as null.
This might also cause crash.
Good call, thanks! Yes, my actual code has more checks in it. I stripped those and some other additional code (for instance progress reporting for the benefit of the UI) to keep the example clean.
In fact, I broke out most of the body of the loop to another function (SampleSound *loadSample(ZipFile &zipFile, int entryIndex)), because I actually have two loops: The first loop just loads the sounds that are needed urgently (based on settings), and the second loads the rest. This mechanism is there to make the plugin start producing sounds as quickly as possible.
Hopefully you are doing all that in a thread safe manner.
Yes, at least I think so. There is only ever one thread writing to the map (std::unordered_map), the audio processing thread only reads from it. And I’ve made it so that nothing breaks if the sample lookup returns null, it just skips playback of that sample and tries again next time that sample is requested.
Do you think I need something more serious than that, i e do you think there are cases where the map would return a pointer to a SamplerSound object that’s somehow invalid? I’m working under the assumption that the pointer is inserted into the map only when the SamplerSound constructor is complete.
That’s not thread safe if you are not doing something to make it thread safe. (Like using a mutex.)
So that means that there are other possible outcomes than receiving a valid SamplerSound pointer or nullptr? Do you mean that the reading operation can mess with the writing operation in the other thread somehow?
Would this be a correct way to go about it:
I add CriticalSection _mapAccess as a member variable in my class (which is the only class that has access to the map since it’s a private member).
In the method that does the writing:
for (int i = 0; i < numEntries; i++) {
const ZipFile::ZipEntry *entry = zipFile.getEntry(i);
String name = entry->filename;
const ScopedLock lock(_mapAccess);
if (_buffer[name] == nullptr && name.endsWith(".wav")) {
_buffer[name] = loadSample(zipFile, i);
}
}
It’s undefined behavior, so anything can happen, including the code working. The C++ standard library containers are not thread safe, so they must not be manipulated and/or read from multiple threads at the same time without synchronization. What might happen in practice in your use case is that the std::unordered_map does a reallocation when you are assigning an element into it and the audio thread might then see the container in an inconsistent state. You might get back a null pointer, a valid pointer or something else in that case when reading from the container. But even without the reallocations happening it can lead to wrong results.
By the way, for the benefit of others who might come across this thread, I made a pretty serious mistake in my code above: I was loading the sample while holding the lock, which will of course slow down other threads considerably and completely unnecessarily…
So it’s better to do something like:
for (int i = 0; i < numEntries; i++) {
const ZipFile::ZipEntry *entry = zipFile.getEntry(i);
const String &name = entry->filename;
if (name.endsWith(".wav")) {
SamplerSound *sampSound = loadSample(zipFile, i);
if (sampSound != nullptr) {
const ScopedLock lock(_mapAccess);
_sample[name] = sampSound;
}
}
}
Another addition, your unordered_map doesn’t handle ownership and lifecycle.
I don’t know how you handle the destruction, but whenever you remove or replace the pointer in the map, you must delete the pointee, otherwise you leak the SamplerSound objects.
And when you do, you risk the SamplerSound still being used.
For that purpose, the SamplerSound is a ReferenceCountedObject, and you can define the map like that, if you didn’t already:
Now when you remove or replace a Ptr in the map, if it is the only reference, it will be destroyed. If it is used in playback, that code still prevents the deletion. Only if the reference there goes out of scope, it will be deleted.
Only drawback: the deletion can happen on the audio thread.
I will never replace a SamplerSound in the map during the lifetime of the plugin. I just load them on startup, and they won’t be deleted until the map itself is deleted (i e when the plugin itself is destroyed).
Is this safe enough, or should I still use SamplerSound::Ptr?
Mixing ScopedPointers and ReferenceCountedPointers will definitely cause all kinds of dangling pointer bugs!
You can’t safely use Strings in your audio callback, as they may allocate when copied.
You may be able to avoid race conditions if you’re very careful to use a read-only unordered_map in an audio callback and are absolutely sure it won’t be modified at the same time. But std containers give no guarantees about what happens inside their methods, and you can’t assume that calling the operator won’t allocate (e.g. perhaps some versions of unordered_map may occasionally decide to optimise their internal data structures when you call this method)
(Oh, and leading underscores for names are generally considered bad style in C++ - they’re used by the standard library and for other reserved symbols.)
