Best coding practices for audio applications - 2 questions (both answered)

General JUCE discussion
1

Hello,

The stuff below is probably tl;dr for some of you so here my 2 questions. Background is below.

  1. Is it safe to call new MidiMessage() on the audio thread?
  2. Is it ever a good idea to have locks on the audio thread?

And here the tl;dr stuff :).

I’ve been reading up on best practices for audio applications programming. One element that keeps returning is “never use unbounded (in time) operations on the audio thread.”; examples of unbounded operations being file I/O or memory allocations (malloc() & friends). And another one being “don’t use locks on the audio thread because you might run into priority inversion”. ( see here for an example of an article I enjoyed that dives into these matters; I’m not experienced enough to judge the advice’s soundness but frankly it all makes sense to me ( http://www.rossbencina.com/code/real-time-audio-programming-101-time-waits-for-nothing )

So I have a series of plugins that generate MIDI data that are close to 1.0 release. Neither of them currently misbehaves (glitches, timing errors). I’m too old to be naive enough to believe that because my plugins seem to behave, they will always behave. So reading the above puzzles me for 2 reasons:

1 . Since my plugins generate MIDI, I do have - on the audio thread - plenty of new MidiMessage() calls. Don’t those allocate memory? If new MidiMessage() is a no-no, how do others go about circumventing that? I notice that e.g. in the Juce arpeggiator tutorial the code does something similar:

midi.addEvent (MidiMessage::noteOn  (1, lastNoteValue, (uint8) 127), offset);

Is that poor practice and only in the tutorial to simplify things? Should I pre-allocate the memory? We are easily talking several 1000s of midi events per second here - so I’d have to pre-allocate - what? - several 100k midiMessages? Of course I could have that managed on a separate low priority thread, that allocates chunks of say 1000 midiMessages, keeps track of much many pre-allocated midiMessages have been “used” and generate more of these when needed (and plenty early). But that too would be unbounded - still risking an underrun of allocates messages. No?

Is pre-allocating the recommended approach? Or is new MidiMessage() safe? How do others deal with this?

2 My plugins generate realtime (midi) pattterns, and there is a fair amount of randomness inthere. Meaning I cannot actually generate the pattern data in real-time; I have to use some buffering because when processBlock decides to decide to perhaps generate an event, it might conclude that it should have done it earlier.

So I buffer these a bit ahead of time (I generate the outline of the patterns in my own datastructures; I translate them to Midi in real-time). The actual generation of the patterns is farmed off to a lower priority thread, and for a whole bunch of reasons, the timing of (re) generation is not critical. The way I do this now is:

  • When the audio thread decides it’s time to generate some more pattern data, it wakes up a low priority worker thread
  • The work thread does its job in separate data structures (also allocates memory, but since it’s not time critical we don’t care how long it takes) so there is no overlap with the data model the audio thread needs; and hence locks are not needed.

HOWEVER: I do use locking - on the audio thread. The way I see it this is not at all risky in this particular case:

  • Audio thread locks the data model when doing processblock
  • worker thread locks the model ONLY to swap the newly generated data into the model (and swap “used” data out of the model). The lock is held by the worker thread ONLY to swap those 2 pointers and that peanuts in terms of processor time.

I fail to see how this could be risky, or how this might cause a problematic priority inversion. A priority inversion might happen of course: when the audio thread wakes up, right after the worker thread snaps the lock to swap those 2 pointers, the audio thread just waits. That’s more than fine - the couple of instructions for swapping those pointers are irrelevant to the amount of work processBlock needs to do - it simply does not make any difference. FWIW I use a spinlock because it’s low overhead and frankly it fits my needs (don’t need the added complexity of critical sections)

For now that seems to work great. But I’m wondering:

  • Am I missing something? Is there a real reason why this would be a bad idea/approach in this particular case?
  • If I am missing something then what is the way to handle situations like this without using locks?

Any thoughts would be appreciated; feel free to alert me to my own stupidity :slight_smile: .

2

Don’t you get memory leaks if you “new” the MidiMessages? MidiBuffer’s addEvent signature wants the events as const references. So, are you doing it like :

buffer.addEvent(*new MidiMessage(...),0);

?

Seems like a leak to me.

The other issues are really complicated and MidiBuffer is potentially doing news/mallocs internally anyway.

1 Like
3

MidiMessage basically has a small buffer optimisation in it so can store standard MIDI messages without allocating. So yes, if you pre-allocate your MIDI buffer it will just be copying some bytes around, not allocating and hence holding the global lock on the heap (which the message thread uses a lot and other threads may use to allocate large blocks of memory).

1000’s of messages a second are not that many. If your buffer size is ~256 then you’re talking a few hundred messages per buffer. That’s not that many.

1 Like
4

Well @Xenakios that code comes from the tutorial, I personally don’t do things like that :slight_smile: - I never compiled and ran the arpeggiator tutorial so I have no idea whether it leaks or not.

I’m assuming that if I create a new MidiMessage object and add it into the processblock midibuffer, it gets properly destructed by whatever calls processblock and subsequently deals with whatever I put in that buffer. Perhaps @jules can confirm this. I certainly don’t have leaks in my code :slight_smile:

Further, since you’re saying that MidiBuffer is potentially doing new/malloc, are you also saying I should not create MidiMessages on the audiothread? In that case the code in the tutorial would not exactly guide developers towards best practices, right?

5

Tx Dave. So if I get you correctly you are saying that:

  • new MidiMessage() is not safe
  • preallocating is the way to go

Correct?

6

My point is that the arpeggiator example does not use “new” for the MidiMessages, but you say your code does. I am almost sure your code is leaking, MidiBuffer’s addEvent wants the MidiMessages as const references and it copies them into the internal Array. Nothing is cleaning up your "new"ed MidiMessages, unless you are deleting them yourself somewhere. Note that MidiMessage does not have the Juce leak detector enabled.

1 Like
7

In your example code from the arpeggiator tutorial I don’t see a “new” keyword. Isn’t that just a stack allocated MidiMessage?

1 Like
8

I’m not cleaning up created MidiMessages. Should I? I got the impression that I am passing the objects on to the processblock midibuffer, and that the called of processblock mops them up after the midi has been sent out of the plugin. See also my reaction to @Achder below.

9

Where are you typing new MidiMessage then?

10

You’re right, the “new” is not inthere. But does MidiMessage::noteOn (1, lastNoteValue) create on the stack? (I thought it was heap allocation) If so, then whatever I add to the processblock midiBuffer will be destroyed once processBlock ends (or once the block in which the allocation is done ends) - that would be strange because the purpose of adding stuff to that buffer is to make sure that the caller of processBlock sends the midimessages out of the plugin - which is kinda hard if the objects no longer exist.

Or are you saying that buffer.addEvent makes a copy of the message? But in that case if would have to malloc it - and that would be equally unsafe!

11

@Xenakios : you may have pointed me somewhere. Actually I do not add to the processblock midibuffer; I add to a locally declared buffer, which gets swapped with the processblock buffer. So when processblock ends, the locally declared midiBuffer is destroyed, and all midimessages are discarded. But the ones I created are passed on the the caller (which I still suspect, disposes of them).

12

This is what happens but it doesn’t have to malloc it.
When you write this the following things happen:

midi.addEvent (MidiMessage::noteOn  (1, lastNoteValue, (uint8) 127), offset);
  1. You create, on the stack a MidiMessage “note on” with the arguments you’ve specified
  2. You call MidiBuffer::addEvent which takes a const& to the MIDI message you just created
  3. If the MidiBuffer has enough free space, it simply copies the bytes of the message passed in, in to its storage
  4. If it doesn’t have enough space, it has to resize and hence do an allocation
  5. The MidiBuffer doesn’t go out of scope, it’s passed as a reference to the processBlock so is owned by the caller

N.B. This is all blown out of the water if you create a sysex MidiMessage as that does need to internally allocate.

4 Likes
13

OK, here is some pseudocode - very rudimentary (doesn’t make a lotta sense, just to explain this case); I probably phrased my question in a confusing way, and typing it up I got myself confused - apologies for that.

void TopiaryAudioProcessor::processBlock(AudioBuffer<float>& buffer, MidiBuffer& midiMessages)
{
    MidiBuffer myNewMidi;
    << lots of loops that do stuff>>
    {
        msg = MidiMessage::noteOn(channel, noteNumber, velocity);
        myNewMidiBuffer->addEvent(msg, 0);
    }

    midiMessages.swapWith(myNewMidiBuffer);
}

So I am assuming that addEvent makes a copy of the midi event (which I created on the stack, admittedly). Doesn’t addEvent then do new MidiEvent somewhere? And would that not be unsafe? Or does MidiBuffer pre-allocate space? (hm - guess I better have a look at the source for MidiBuffer … do that tonite).

Regarding @Xenakios’ leak comment; am I right to assume that the caller of processblock will dispose of anything in midiBuffer (just like I dispose of what was there by putting it in myNewMidiBuffer - when that goes out of scope)?

14

In that instance, when you add to myNewMidi it will allocate. When it goes out of scope it will deallocate anything that it owns.
If you swap it with the midiMessages parameter, it will deallocate anything that was contained in that.

You want to make myNewMidi a member, pre-allocate it by calling ensureSize outside of the audio callback and then do a myNewMidi .data.clearQuick() at the start of processBlock to clear it without deallocating the space.

1 Like
15

Tx Dave (you may wanna re-read my pseudocode? - I hit submit before it was complete). Would you still suggest I pre-allocate myNewMidiBuffer outside of processblock? (easy enough to do really)

Great explanation! But now I’m puzzled by your bit

  1. If it doesn’t have enough space, it has to resize and hence do an allocation

If it has to allocate, might that not be unsafe?

Very interesting comment about the sysex message - I’ll wanna look into that!

16

@dave96 : I chewed on this over a cuppa coffee and I really like your suggestion: if I ensureSize and preallocate myNewMidiBuffer large enough to hold mididata for one block, I should be able to avoid it reallocating and things should be safe. The memory overhead of say 200 midiMessages is hardly a penalty. But I don’t think stricktly speaking I must do that as a member outside the audio callback (though I certainly will) - I’m thinking even if I did such allocation within processblock I should be fine. Any reason my thinking if wrong?

17

OK @dave96 , forget the last comment: I figured it out myself: the ensureSize might actually do allocation, which is why I should NOT have it in processBlock() - I should do that in the initialization of the AudioProcessor!

Big thx for clarifying all that.

Any takers on question 2 about the locking?

18

There was a big discussion about locking a few days ago starting from here: Timur Doumler Talks on C++ Audio (Sharing data across threads)

1 Like
19

I think you answered my point in that thread:

It’s all a bit voodoo here though and the bottom line with audio is if you can’t guarantee how long it will take then don’t do it in the audio thread.

What I’m doing is O(1) and takes insignificant amount of time (swapping 2 pointers) so I should be fine waiting for that on the audio thread. Which was my thinking to start with.

I owe you a couple of beverages for answering both questions! Cheers!

20

No, I think you missed the point. Even if you operation is O(1), there’s a chance the other thread could be put to sleep whilst holding the lock, hence you have no idea how long it will take.

If you’re just swapping two pointers though, make them std::atomic<T*>. But remember, that’s not usually the answer because you need to make sure neither thread is actually using the T* when you swap them.