As I announced in the "Sampler Skeleton Thread", here is a subclass of SamplerSound and SamplerVoice that allow disk streaming:
https://github.com/chrisboy2000/streaming_sampler
It comes with an example Audio plugin project and API documentation that shows the usage. Feel free to use it whereever you like (no licence restriction) and drop me a feedback if you found something strange.
Best,
Christoph
Thank you ! Good idea and maybe a potential great module for juce.
Thanks. I'm working on a juce module of a complete modular sampler system with scripting, modulators and effects (probably open source) and thought this spin-off class would benefit greatly from a wide user base, since I think it is pretty basic stuff which every sampler should be able to do, and there is no need for other developers to stumble about the same problems (it was kind of a mess to get the buffer swapping working)
Thanks for the module! I have added it to my sampler and it seems to work.
But I think I found a bug: Every now and then I hit the following exception:
void SampleLoader::fillInactiveBuffer()
{
jassert(sound != nullptr);
I am not quite sure why. But my guess is the following: Assume that the function SampleLoader::runJob() has started. But right after it starts, StreamingSamplerVoice::stopNote() is called, which calls loader.reset() and thereby sets sound=nullptr. I think, that StreamingSamplerVoice::stopNote() can be called asynchronously from anther thread, for example by midi events coming from the keyboard (note off event). I have to admit though, that I am not quite sure about that last assumption about asnychronous calls.
But what would I need to do, in order to make SampleLoader::fillInactiveBuffer() more thread safe, so that I cannot hit the jassert?
Maybe I simply should change the assert into an if-check, like:
if (sound == nullptr) return;
But I do not think, that would be 100% safe. I guess, sound needs to be protected by a lock as well.
Weird, I do not seem to run into that case even with ridiculous small sizes and my good old spinning HDD, so thanks for the hint.
I already ScopedLocked this function, but without supplying an actual lock - stupid typing mistake :). I already commited a fix yesterday for this, so maybe this thing is already solved...
https://github.com/chrisboy2000/streaming_sampler/commit/7ceda9c8ed4252ebda87d7399042ef954ed8e545
If not, I will change that assert and add one to reset(), but muting the warning signs is never the best solution to a problem :)
Best,
Christoph
Hi Christoph,
no, I am already using the version, where you fixed the lock of the reset() function. So I implemented the following change in fillInactiveBuffer() (added a lock):
void SampleLoader::fillInactiveBuffer()
{
ScopedLock sl(lock);
if (sound == nullptr) return;
Not sure, this is the best solution (too much locking???) but it seems to work.
But then I ran into a new jassert. It happens in juce_AudioSampleBuffer.h. And it is quite easy for me to reproduce. I just hold the sound, until it has finished playing:
const float* getReadPointer (int channelNumber, int sampleIndex) const noexcept
{
jassert (isPositiveAndBelow (channelNumber, numChannels));
jassert (isPositiveAndBelow (sampleIndex, size));
The problem is the last jassert. In my case, the sampleIndex is 11264 and size is 11000. I.e. we are trying to get a read pointer, which is larger than the buffer. The number 11000 is actually the preload size, i.e:
#define PRELOAD_SIZE 11000 #define BUFFER_SIZE_FOR_STREAM_BUFFERS 11000
The problem is the following: the streaming sampler is not only using the preload sizes as buffer sizes. Another buffer size is set in the setBufferSize() method of SampleLoader:
void SampleLoader::setBufferSize(int newBufferSize)
{
bufferSize = newBufferSize;
b1 = AudioSampleBuffer(2, bufferSize);
b2 = AudioSampleBuffer(2, bufferSize);
So there is on fixed buffer size (from the #define macro) and on variable buffer size from this function, which get mixed up. This new buffer size comes from the prepareToPlay() method in my audio engine. In my case, my audio engine is passed a quite large buffer size of 512. I multiplied this size with 32, as you did in you demo example. That results in a buffer size of 16384, which is larger than the PRELOAD_SIZE of 11000.
As a temporary fix I simply enlarged the preload buffers to 22000:
#define PRELOAD_SIZE 22000 #define BUFFER_SIZE_FOR_STREAM_BUFFERS 22000
That works for me, but it is not a good solution of course.
As a result(?), I ran into another assert. But this assert happens really very rarely and is quite difficult for me to reproduce. It’s this one:
void SampleLoader::requestNewData()
{
writeBufferIsBeingFilled = true; // A poor man's mutex but gets the job done.
#if(USE_BACKGROUND_THREAD)
// check if the background thread is already loading this sound
jassert(! backgroundPool->contains(this));
Apparently it is possible, that the background thread already is loading the sound. No idea how/why.
First of all thanks for checking it out so thoroughly.
Not sure, this is the best solution (too much locking???)
Is locking really necessary? Does the simple
if(sound == nullptr) return;
not help? From my little knowledge of multithreading, you do lock the actual disk read operation, which makes multithreading senseless.
Regarding the other stuff:
You should be able to set both buffer sizes. The preload buffer can be set using StreamingSamplerSound::setPreloadSize() and the streaming buffers (two of them) can be set using SampleLoader::setBufferSize().
The thing is that the read pointer that normally points to one of the streaming buffers initially points to the preload buffer of the freshly started sound to prevent copying all the buffers content at start note which is already a performance critical moment, so in order to not create an out of bounds situation like you described, the preload buffer MUST be equal or bigger than the streaming buffers.
This is not a big issue, I think I added a check in my wrapping code for this, but I will think about something ( I want those classes to have as little knowledge about each other, so maybe it is better to implement a check in your code before calling any of these methods).
I wrote the demo with a ASIO buffer size of 256 which stays under the preload size, so I didn't run into that issue, but I will change that to make it more clear.
The last assert is exactly what you expected and means that the job of loading new samples is not done when it is added again. This is a performance issue - try to play with the buffer sizes and see if it disappears - I have pretty nice results with streaming buffer sizes of 8192 or 4096.
What hardware / OS are you using?
I just checked in those things. Any improvements?
Hi again,
From my little knowledge of multithreading, you do lock the actual disk read operation, which makes multithreading senseless.
I was not aware, that fillInactiveBuffer() actually is reading from disk. I am not very familiar with multithreading either, but I guess, locking the disk access is not such a good idea. It works OK for me, when I remove the lock and just use the "if-check".
You should be able to set both buffer sizes. The preload buffer can be set using StreamingSamplerSound::setPreloadSize() and the streaming buffers (two of them) can be set using SampleLoader::setBufferSize().
The solution with manually changing the preloadbuffer is not really something I like. Would it be possible, to make this check automatically instead? That would make the class safer to use, I think.
The last assert is exactly what you expected and means that the job of loading new samples is not done when it is added again
Can that lead to a problem somewhere? Just thinking, in case someone is using a slow hard disk or something and then he could run into this more often.
I just checked in those things. Any improvements?
When I applied your changes, they worked fine for me. I am not hitting any assertions anymore :-)
Next thing I am looking at is the renderNextBlock() function, when I find time. Because I am hearing some slight audio artefacts in the playback.
BTW: I am using OSX 10.9.4 on my MacBook. And at the moment I am doing tests in a stand-alone application. But I also have a plugin-version of this application, which I test under Garage Band.
It works OK for me, when I remove the lock and just use the "if-check".
That's exactly what I've checked in, so everybody is happy now :)
The solution with manually changing the preloadbuffer is not really something I like. Would it be possible, to make this check automatically instead? That would make the class safer to use, I think.
The thing is those buffer sizes are seperate because they adress different hardware specifications (random access for preload buffer vs read speed for the streaming buffers), so in order to let the user customize these settings seperately according to his hardware seems to be reasonable to me.
I added an assertion that is hit whenever you start a sound with a too small preload buffer. But the problem is that the streaming buffer belongs to the voice (which can play a variety of sounds with different preload buffers if desired), so checking every sounds' preload buffer when changing the voice size would mean that the voice must know exactly which sounds are going to be played, which is not the way the original sampler class is designed (what if you add new sounds on runtime ?)
Can that lead to a problem somewhere? Just thinking, in case someone is using a slow hard disk or something and then he could run into this more often.
You will get glitches, but this is what happens if your hardware is too slow, so there is nothing you can do about that besides changing the buffer sizes.
Next thing I am looking at is the renderNextBlock() function, when I find time. Because I am hearing some slight audio artefacts in the playback.
Yeah, thanks. A possible problem could be the amount of samples that are fetched. For the interpolation you need a little bit more samples and if it's really bad luck, you could cause a buffer swap although the samples are not really used. I spent a few hours removing those cases, but it is possible that they still appear on certain pitch factor / buffer size combinations.
But always check the Release build for artifacts, I'm hitting the drop out zone really fast in Debug builds (seems like the file reading is way slower here).
BTW: I am using OSX 10.9.4 on my MacBook. And at the moment I am doing tests in a stand-alone application. But I also have a plugin-version of this application, which I test under Garage Band.
Great, I'm the windows guy then...
Best,
Christoph
First of all: thanks for that code - it looks very helpful.
Is locking really necessary? Does the simple
if(sound == nullptr) return;not help? From my little knowledge of multithreading, you do lock the actual disk read operation, which makes multithreading senseless.
The thing is: another thread could call SampleLoader::reset concurrently, which contains:
sound = nullptr;
So there is no guarantee that for any operation that follows a check like
if(sound == nullptr)
, sound still is != nullptr - so the lock is necessary (at least in theory: one could argue that chances of that happening are very slim, but for multithreading, it's better to abide Murphy's law).
if(sound != nullptr && sound->hasEnoughSamplesForBlock(bufferSize + positionInSampleFile))
{
sound->fillSampleBuffer(*writeBuffer, bufferSize, (int)positionInSampleFile);
}
Thanks for the hint It is indeed theoretically possible that sound gets set to nullptr after testing the first condition (although it's very unlikely).
A lock-free solution would be using a local copy of the pointer (setting sound to nullptr does not delete the object, so we're not having any dangling issues:
StreamingSamplerSound *soundCopy = sound;
if(soundCopy != nullptr && soundCopy->hasEnoughSamplesForBlock(bufferSize + positionInSampleFile))
{
soundCopy->fillSampleBuffer(*writeBuffer, bufferSize, (int)positionInSampleFile);
}
Any side effects can be discarded, since the buffers that will be filled won't get read after calling reset(). So the worst case would be an unnecessary read operation, which is not the most evil thing in the world :)
I'll add this with the next commit (I also added sample truncation, looping with crossfades and sample start modulation, but it needs some testing.)
hit the wrong reply link
Copying the pointer seems like a good solution - but here's a next (very theoretical) problem:
On the other hand, if the object pointed by sound is not destroyed, what you just suggested chris (copying the pointer) is the best solution imho!
The Synthesiser class allows dynamic unloading of sounds which might cause the sound object to be deleted. SynthesiserSounds are reference-counted, so maybe the sound pointer within SampleLoader (and its working copy) should be ReferenceCountedObjectPtr?
Well I would assume that an application which deletes sounds dynamically would at least stop the sound that is currently played and wait until it has tailed off before deleting it (or you get some nasty clicks), so this is a rather exotic case we are talking about.
Seems sensible indeed. (I'm sorry, I have much more experience with proof-reading students' multithread exercise codes than with Synthesiser coding and got a bit caught there...)
No need to be sorry. I'm grateful that you read my little "multithread exercise". In fact I was quite certain that everybody with a profund understanding of multithreading would laugh at me and my honky "writeBufferIsBeingFilled" mutex :)
