Dropping samples?

hmm, I just took all the file stuff out of AudioRecorder::startRecording method (I pre-create the file which is passed into the AudioRecorders RecOut::startRecording method when the channel you wish to record is selected) and it didn’t seem to get any better. Are you suggesting that I also somehow break out the code in RecOut::startRecording and RecOut::stop? There are maybe certain things I can preallocate, like the WavAudioFormat, but the rest seems that it needs to be created/deleted each time right…? Unless its fine to create the ThreadedWriter and everything (basically everything in RecOut::startRecording) once when the class is instantiated and just have a global bool in my audio callbacks that gets checked if its true or not before calling ThreadedWriter::write?

I’m refering to these methods…

void RecOut::startRecording (const File& file)
{
	stop();
	if (sampleRate > 0)
	{
		// Create an OutputStream to write to our destination file...
		file.deleteFile();
		ScopedPointer<FileOutputStream> fileStream (file.createOutputStream());
		
		if (fileStream != 0)
		{
			// Now create a WAV writer object that writes to our output stream...
			WavAudioFormat wavFormat;
			AudioFormatWriter* writer = wavFormat.createWriterFor (fileStream, sampleRate, 1, 16, StringPairArray(), 0);
			
			if (writer != 0)
			{
				fileStream.release(); // (passes responsibility for deleting the stream to the writer object that is now using it)
				
				// Now we'll create one of these helper objects which will act as a FIFO buffer, and will
				// write the data to disk on our background thread.
				threadedWriter = new AudioFormatWriter::ThreadedWriter (writer, backgroundThread, 32768);
				
				// And now, swap over our active writer pointer so that the audio callback will start using it..
				const ScopedLock sl (writerLock);
				activeWriter = threadedWriter;
			}
		}
	}
}

and

[code]void RecOut::stop()
{
// First, clear this pointer to stop the audio callback from using our writer object…
{
const ScopedLock sl (writerLock);
activeWriter = 0;
}

// Now we can delete the writer object. It's done in this order because the deletion could
// take a little time while remaining data gets flushed to disk, so it's best to avoid blocking
// the audio callback while this happens.
threadedWriter = 0;

}[/code]

I would do the following:

  • Each recorder would have its own thread and thread queue (I would not use the ThreadedWriter)
  • Create a reference counted AudioSampleBuffer
  • From inside the audio callback, copy the audio data into a reference counted AudioSampleBuffer
  • From inside the audio callback, queue a call to each recorder’s thread queue with a reference to the AudioSampleBuffer
  • Each recorder, upon receiving a buffer, simply writes it synchronously (since its getting called on its own thread).

It would be easy enough to add some kind of start/stop calls into this model.

‘bond’ is just a simplified version of boost::bind that I wrote to avoid making my DspDemo depend on boost.

You would want to use boost::bind, or depending on your build environment it might be available as std::bind or std::tr1::bind.

To bind a class member you would write

bind (&Class::member, classPtr, param1, param2, …)

where classPtr is a Class*

I’m thinking I should try to get it working both ways and here’s why-- the way Jules is doing it is pretty much identical to how I had it structured, and so if it recently worked for him on a project, I’d really like to know why it is that mine is exhibiting this behavior. Vinn - the way you propose looks very interesting to me on many fronts, especially about learning about threads and concurrency, something I’d really like to school myself on as I become a better and more efficient programmer (been reading the websites you linked to in the other thread). As this is for a piece of software I need to have finished for my PhD research, the most important part is really that I have it finished as soon as possible, so I can use the software for data collection which I need to collect and stay analyzing soon. Hopefully I can have this all finished by the end of this week. Id be interested to get both models working, and compare the results. As I am not only recording Audio data, but upsampling (sorta) and doing the same recording with data from analog/digital sensors (on instruments/performers) and via OSC, it is really important that everything is synchronous and accurate as possible…

  1. I guess I’ll see what Jules says about reducing the workload during the audio thread lock
  2. Vinn, as I try to conceptualize your method (apologize in advance if some of this is brutally simple):
    *Make my RecOut class extend AudioIODeviceCallback and Thread

[quote]- Create a reference counted AudioSampleBuffer

  • From inside the audio callback, copy the audio data into a reference counted AudioSampleBuffer
  • From inside the audio callback, queue a call to each recorder’s thread queue with a reference to the AudioSampleBuffer[/quote]
    These are all referring to the same buffer right? So I just create one (ref counted) AudioSample buffer (in my RecOut class) when the class is instantiated, and each audio callback I clear the buffer, copy in the new data and then pass it to an AudioFormatWriter (lets call it myWriter) via writeFromAudioSampleBuffer using the ThreadQueue call:
    e.g. m_queue.call (bond (&AudioFormatWriter::writeFromAudioSampleBuffer, myWriter, AudioSampleBuffer &source, int startSample, int numSamples));
  • Next I call m_queue.process()

Thanks!

[quote=“jordanh”]These are all referring to the same buffer right? So I just create one (ref counted) AudioSample buffer (in my RecOut class) when the class is instantiated, and each audio callback I clear the buffer, copy in the new data and then pass it to an AudioFormatWriter (lets call it myWriter) via writeFromAudioSampleBuffer using the ThreadQueue call:
e.g. m_queue.call (bond (&AudioFormatWriter::writeFromAudioSampleBuffer, myWriter, AudioSampleBuffer &source, int startSample, int numSamples));

  • Next I call m_queue.process()[/quote]

They refer to the same buffer but you need to create a new reference counted buffer each time the audio I/O callback executes. The old one might still be getting written out (this would be the situation if the I/O cannot keep up with the stream of generated buffers).

AudioSampleBuffer cannot be passed as a reference through the thread queue. It needs to be passed by value. You want something like this:

struct ReferenceCountedBuffer : public ReferenceCountedObject
{
  typedef ReferenceCountedObjectPtr <ReferenceCountedBuffer> Ptr;

  AudioSampleBuffer buffer;
};

You would need to do new ReferenceCountedBuffer in the audio I/O callback and assign it to a variable of type ReferenceCountedBuffer::Ptr. The argument passed to the function used in bind (or bond) needs to be of type ReferenceCountedBuffer::Ptr. Since writeFromAudioSampleBuffer() does not take the Ptr as a parameter, you would have to write a wrapper function.

I didn’t realize this was just a school project. Getting a thread queue implementation up and running is a non-trivial task, and definitely has a higher “start up” cost than a straight critical section, although the benefits definitely pay off as the system grows in size (although this might not be your use-case).

You might be misunderstanding how the queue works. m_queue.process() has to be called from the destination thread, not the thread that is doing the m_queue.call(). There are two players at work here:

  1. Audio I/O Callback.

    • Acts as the “Producer”
    • Puts a sequence of reference counted sample buffers into each thread queue
  2. Writer thread

    • Acts as the “Consumer”
    • Blocks until there is work in it’s associated thread queue
    • Calls m_queue.process() when there is work
    • Receives reference counted sample buffers as an argument to a callback function

What is missing from the DspFilters demo is the “signaling” mechanism that tells the consuming thread when to wake up and call process(). In my demo, process() is simply called once at the beginning of every audio I/O callback. However, the ThreadQueue implementation provided in the demo contains overridable virtual methods (signal and reset) which allow you to implement your own signaling.

In your case, you would need to create a subclass of ThreadQueue and override the signal() and/or reset() functions. You could use the juce::Thread::notify() and juce::Thread::wait() functions - the implementation of these is done using a juce::WaitableEvent which is perfectly suited.

Well, I say: “reduce the workload during the audio thread lock”

Like I said, in my version all I do is lock it, set a couple of boolean flags, and then unlock it. Don’t do any more than that.

If you don’t mind me asking, when do you create your fileoutputstream and ThreadedWriter? Do you do it upon creating an instance of your recorder and then reuse them somehow? I thought they needed to be deleted / created each time; That’s the one thing I can’t conceptualize how to take out of the equation-- perhaps I just do all that (creating the FileOutputStream, ThreadedWriter, etc) before calling my lock, and then set my shared bool which gets checked in each recorder audio callback to write or not? Thanks!

Yeah, just set up all your threads and file stuff, and then when it’s all ready to go, lock it and start it all running.

So I think I have it all set up per your suggestions, but the problem still pops up after a few recordings… I have it set up like so:

When recording is enabled and the user presses play, it loops through my collection of AudioRecorders (tracks) and calls a method which handles creating a file (FileOutputStream) and the threadedWriter; that all happens pretty much identically to the juce audio recording demo. Once it has looped through all recorders, it calls the audio lock, and sets a flag shared by all recorders either true or false. Inside each AudioRecorders audio callback, it checks this flag to either write the samples to its buffer, or to call stop(), which deletes the ThreadedWriter as per the demo. I also tried taking out my channel selection, so it just writes inputChannelData from the audio callback just like in the demo, but the error persists.

Could there be something else thats the culprit? I’ll keep having a go… thanks

Been testing all morning, and these are the things I’ve been able to reproducibly verify:

[] The first recording / take always seems to be fine, and then the problem arises, sometimes on the second take, but usually some later take (4, 5, etc)
[
] If I record the same input on 2+ tracks at once, the resulting .wav files are identical-- including glitches
[] Removing the shared bool and audio callback lock, the problem disappears. In this way, recording is set up identically to the demo. Clicking in the gui or pressing spacebar to call startRecording() and stop()
[
] Adding the shared bool seems to bring the problem back. I tried this both with and without calling the audio lock before setting the shared bool. Currently the bool is checked in the recorders audio callbacks to either write or call stop() depending on the state of the bool like so

hmm…

void RecOut::audioDeviceIOCallback (const float** inputChannelData, int numInputChannels, float** outputChannelData, int numOutputChannels, int numSamples)
{		
	const ScopedLock sl (writerLock);
	if (activeWriter != 0 )
	{
		if(*finishWriting == true)
		{
			activeWriter->write(inputChannelData, numSamples);
		}else {
			stop();
		}
	}
	// We need to clear the output buffers, in case they're full of junk..
}

[EDIT] I did more testing tonight, and got the same behaviour in the JUCE Demo. All I did was modify AudioDemoRecordPage so that it modifies a shared bool instead of calling start stop directly on the recorder-- when the record button is pressed, if a shared bool (writingState) is false, it creates a new file and passes it into the recorders startRecording, then it briefly calls the audio lock and flips the shared writingState bool. Then in the AudioRecorder class I changed it per above (if the shared bool writingState is true, it should write the new data, else it should call stop() ).

@Jules - Can you think of anything else that could be causing this problem or that different that you can remember between the project you had this working on the way I’ve set it up?
Best, J

Well, still haven’t gotten this figured out, but I recorder the metronome out from Ableton via soundflower and noticed something interesting looking at the waveforms. It seems that the glitches that I thought were dropped sample or junk samples are actually copies of the correct data-- as per the screenshot attached, this quarter-note (crotchet) beat looks like the initial transient / half of the waveform is played twice, separated by some silence. It actually happens quite fast-- this time its slow enough to sound like a subtle delay, other times like weird phase problems. On other audio sources that aren’t just a single tick, it sounds like random samples inserted at random…

Interestingly this never happens on the first take, but as a new ThreadedWriter is created each time, I can’t see whats different between takes. The only thing that lives between takes in the recorder itself, as well as the background thread which the ThreadedWriter uses. I did try starting/stopping the background thread with each recording, but that didn’t seem to fix anything, so I changed it back since it takes time to start/stop the thread, and I figured it wouldn’t hurt to have it going for the life of the recorder…

Any ideas? The problem disappears (both in my code, and the juce demo) if I remove the audiocallback lock and shared bool synchronization but surely theres a way! :slight_smile:

Came back to this again tonight and still no luck… A few posts earlier I think I may have mentioned that I implemented the shared flag method in the juce demo and got the same behaviour there… First recording is always fine, and then some after usually within the next 4 or 5 takes the recordings get messed up.

Jules, I know you are extremely busy, and have already helped immensely, so I feel bad even asking (and especially every time I come back to this thread!) but if you find yourself with a few minutes at some point, if you can get it working with the shared flag in the demo, where it doesn’t get weird after a few takes I’d really like to know what you did.

This app is elemental towards my PhD research, and I start data collection when I visit the US in a month and a half so I really need to figure this out. I’ll be in London in early June if the Raw Material HQ could use a case of beer dropped off :slight_smile:

Any other ideas / probing questions / anything I can try is greatly appreciated. As always thanks-- I can’t wait until I can post that I’ve got it working in this thread!! Also, if anyone is interested, I could maybe generate a stripped down version of the app with just this stuff in there to have a go at it?

Sorry, I’m super-busy right now and haven’t time to look at anything that’s not an actual bug in the library… More than happy to meet you for a beer next month though!

Okay, I think I’ve tracked it down!

The problem arises when I set threadedWriter to 0 (or nullptr). I am doing this within my stop() method, as per the demo. If you look in the stop() method below, the writing works without a hitch if I comment out threadedWriter = 0. The problem is, the output files don’t close properly until threadedWriter gets deleted, so each take only finishes and results in a working file either a) when a new take is started (I think this is because I reuse the threadedWriter variable, see = new ThreadedWriter in start() below) or b) I close the program ( threadedWriter gets deleted and the file closes up properly).

I can’t come up with a reason for this, can you think of why this might be causing the problem? At this point, the only difference between this code, and the juce demo is that instead of calling start() and stop() on a GUI event, the GUI event sets a shared bool (finishWriting in my audioDeviceIOCallback’s, as shown below) which then calls start() or stop()…

So close!

void RecOut::stop()
{
	{
		const ScopedLock sl (writerLock);
		activeWriter = 0;
	}
	threadedWriter = 0;
}
void RecOut::startRecording (const File& file)
{
	if (sampleRate > 0)
	{
		file.deleteFile();
		ScopedPointer<FileOutputStream> fileStream (file.createOutputStream());
		
		if (fileStream != 0)
		{
			WavAudioFormat wavFormat;
			AudioFormatWriter* writer = wavFormat.createWriterFor (fileStream, sampleRate, 1, 16, StringPairArray(), 0);
			
			if (writer != nullptr)
			{
				fileStream.release(); // (passes responsibility for deleting the stream to the writer object that is now using it)
				threadedWriter = new AudioFormatWriter::ThreadedWriter (writer, backgroundThread, 32768);
				const ScopedLock sl (writerLock);
				activeWriter = threadedWriter;
			}
		}
	}
}
void RecOut::audioDeviceIOCallback (const float** inputChannelData, int numInputChannels,
									float** outputChannelData, int numOutputChannels,
									int numSamples)
{	
	monoChannelArray[0] = inputChannelData[channel];
	
	if (activeWriter != 0)
	{
		if(*finishWriting == false){
			const ScopedLock sl (writerLock);
			activeWriter->write(monoChannelArray, numSamples);			
		}else {
				stop();
		}
	}
	
	// We need to clear the output buffers, in case they're full of junk..
	for (int i = 0; i < numOutputChannels; ++i)
		if (outputChannelData[i] != 0)
			zeromem (outputChannelData[i], sizeof (float) * numSamples);
}

Agh! No!! Your audio callback can’t just hang around and wait for a file to finish writing! It’s a high-priority callback, you can’t do anything in there which blocks at all!

Ah my bad! I was working off the demo which locks the writer just like that so I didn’t think it was a problem… Conceptually it also made sense to me, but I see what you’re saying. Simply commenting out that lock doesn’t seem to fix it though, so I’m going to investigate some more…thanks!

Sorry I missed this, a fascinating bit of detective work!

Another tiny lecture here from Mr. Swirly :smiley: on locking, blocking and real-time audio…

There are many ways to cause skips and tiny quality defects in your audio code and only one way to do it right. Indeed, I’m working on a digital audio project with another such issue (though my steady-state performance is rock-steady, thank goodness…), one I have to deal with “tonight”!

When you’re writing code in a real-time system, there are two different worlds in which you’re writing. There’s “regular old code” where you can easily do things like open files, take locks, or use system resources. Then there’s the real time code level - and for that code, you need a level of acute paranoia.

It must become second-nature to you is that any code in the direct signal path to your audio outputs has only microseconds to achieve its tasks. In Juce, this code is likely in an implementation of PositionableAudioSource::getNextAudioBlock. Anything that blocks this code - even for milliseconds - will cause skips in your output audio. So that means no disk access! No network access! No long calculations. Even memory management should be avoided. And no locks.

Wait, no locks? Well, you can’t really get away without one lock in your signal path, as far as I can see. TheVinn can chime in here with lock-free queues (which I actually think should be part of Juce, I’ll write more about that later), but it seems to me you need to synchronize each time you get a new block just in case some other thread has changed that part of memory since the last time you saw it.

OK, so you need one lock in your signal path. But that’s scary. If some other thread holds that lock for milliseconds, then BANG - drop-outs! The same is true for holding locks that involve the visual GUI - jerkiness!

So you need to also cultivate a huge level of paranoia when using locks and assume you can just not do anything that’s not tiny, disk/network/big computation are all out.

So how do you deal? Well, you work on keeping your locked areas as small as possible! Remember “swap” and “assignment” are your friends. Here’s a canonical example:[code]MyComplexClass copiedData; // Construct before locking.
{
ScopedLock l(lock_);
copiedData = lockedData_;
}

performLengthyComputation(copiedData); // No locks here, you can do disk access or whatever.

{
ScopedLock l(lock)
lockedData_.swap(copiedData);
}
// Destructor of old “lockedData_” goes off here, outside the lock.[/code]

The problem with trying to put lock-free algorithms into Juce (besides patent issues) is that they require 64-bit CAS (Compare And Swap), which are not available on all Juce-supported platforms. Solutions that use 32-bit CAS exit (I’m using them) but they require garbage collection, or special techniques that are specific to the problem domain.

Making lock-free data structures general purpose and therefore suitable for inclusion into a library like Juce, without using a 64-bit CAS, is a difficult task (and by difficult I mean likely impossible).

In addition, Juce is not designed around message passing for thread synchronization. The Juce model is for individual critical sections that guard mutable values shared by threads. Using a queue for synchronization in Juce will lead either to inconsistency, or pigeonholing/forcing users into using queues.

The better solution is for every Juce object that currently has a hard coded CriticalSection (for example, ResamplingAudioSource) to be a template with a parameter “TypeOfLockToUse”. Some Juce objects are like that already (for example, Array, I believe) - and leave the sychronization up to the caller.

AHA!

Thank, TheVinn, I knew there had to be some sort of hidden synchronization-like activity hidden in there. All right, great, we know our limitations.

The better solution is for every Juce object that currently has a hard coded CriticalSection (for example, ResamplingAudioSource) to be a template with a parameter “TypeOfLockToUse”. Some Juce objects are like that already (for example, Array, I believe) - and leave the sychronization up to the caller.

ARG! No, that’s awful! Every single class rewritten! Moving all your code in .cpp files into .h files! (Well, I think the DSP part of Juce would be better if it were more templated but that’s a chat for a different day…)

A much better way, and one that would not require any changes to existing code, is simply to make CriticalSection a container class for some interface, say CriticalSectionBase, and add a new method to CriticalSection that allows you to change the class contained in CriticalSection to be your own. Then Juce classes would simply expose a new method, returning a pointer to their locks, and the user could set some other sort of lock, or a common lock, if they liked. All the old code would work fine, and you could do it incrementally, in any order you pleased - my type of change, low stress.

This strategy is a tiny bit more consumptive - off the top of my head, it requires one more heap allocation containing two pointers and one more vtable indirection during lock calls - but these costs are minuscule, literally measured in dozens of bytes and tens of nanoseconds, compared to the potentially huge costs of the lock.

I’m a little puzzled by the observation that you can’t do lock-free stuff without 64-bit CAS ops…

Although it’s true that not all platforms can physically perform a 64-bit CAS, all the platforms that use 64-bit pointers can do, and surely pointers are the only thing that you’d really need a 64-bit CAS for?

BTW there’s already a lock-free fifo in the library: AbstractFifo is lock-free.