Messaging in MidiDemo.cpp

General JUCE discussion
1

I'm trying to understand the rationale behind the messaging code in MidiDemo.cpp

<!--break-->

I'm looking through MidiDemo.cpp

I can see that it handles callbacks from the active midi device as well as the on-screen midi keyboard:

class MidiDemo  : public Component,
                  private ComboBox::Listener,
                  private MidiInputCallback,
                  private MidiKeyboardStateListener,
                  private AsyncUpdater
{

    void handleIncomingMidiMessage (MidiInput*, const MidiMessage& message) override // <-- MIDI device callback
    {...}

    // Note ON/OFF are on-screen keyboard callbacks
    void handleNoteOn (MidiKeyboardState*, int midiChannel, int midiNoteNumber, float velocity) override
    {...}

    void handleNoteOff (MidiKeyboardState*, int midiChannel, int midiNoteNumber) override
    {...}

all three of these call:

postMessageToList (m);

I can see that these callbacks may be occurring on different threads, and we would like to handle them on the main thread.

But I don't quite understand the implementation. It seems to involve two stages of sending a message.

Firstly,

    void postMessageToList (const MidiMessage& message)
    {
        // this only gets reached from the above three functions, i.e. if a midi signal was received
        //   or if we got a note ON/OFF from the MIDI keyboard component
        //
        // remember, we are in whatever thread initiated this midi signal
        //
        // we want to respond on the main thread, hence we post a message
        //   which will get picked up in the message thread
        //
        // CallbackMessage contains the machinery for this
        (new IncomingMessageCallback (this, message))->post();
    }

    // This is used to dispach an incoming message to the message thread
    struct IncomingMessageCallback
           : public CallbackMessage // <-- contains post()
    {
        IncomingMessageCallback (MidiDemo* d, const MidiMessage& m)
            : demo (d), message (m) {}

        // this will fire when the message gets processed (on the message thread)
        void messageCallback() override
        {
            // so this code gets executed on the message thread
            if (demo != nullptr)
                demo->addMessageToList (message);
        }

        Component::SafePointer<MidiDemo> demo;
        MidiMessage message;
    };

and now finally we handle the message on the message-thread:

    void addMessageToList (const MidiMessage& message)
    {
        midiMessageList.add (message);
        triggerAsyncUpdate();
    }

I can see that the intention here is to allow a bunch of messages to build up before AsyncUpdater sends out handleAsyncUpdate() from the main thread

    // AsyncUpdater calls this from the main thread
    void handleAsyncUpdate() override
    {
        messageListBox.updateContent();
        messageListBox.scrollToEnsureRowIsOnscreen (midiMessageList.size() - 1);
        messageListBox.repaint();
    }

but given that this is happening, was there really any need to go through the first messaging process?

Do we have three threads here, or two?  It looks as though firstly there is the thread that the midi signal came in on,ext the messaging thread, and finally the main thread.

Wouldn't it make sense to just cut to the chase and just call addMessageToList(...) from each of the original three midi-handling callbacks?

Finally, it looks as though if an event from the on-screen keyboard occurs while we processing from the external midi source, it is just going to get lost:

    void handleNoteOn (MidiKeyboardState*, int midiChannel, int midiNoteNumber, float velocity) override
    {
        if (! isAddingFromMidiInput)
        {
            MidiMessage m (MidiMessage::noteOn (midiChannel, midiNoteNumber, velocity));
            m.setTimeStamp (Time::getMillisecondCounterHiRes() * 0.001);
            postMessageToList (m);
        }
    }

Wouldn't a better approach be to spin on isAddingFromMidiInput?

It seems the problem is that two separate threads might simultaneously call back, trying to add an item to the midi-events list. Is this why the first level of messaging happens? To merge this into a single message stream...

Seeing as midiMessageList.add (...) requires very little processing, wouldn't it be more efficient to use a spinlock?

π

2

Agreed, it could be done differently (and yes, probably better!), but (a) it's only a demo! and (b) it's not as simple as you assume.

Calling addMessageToList directly from the callbacks wouldn't work - that would involve a criticalsection, which would need to be locked by the gui thread, which would block the midi input thread every time the listbox is redrawn. Ideally, the way to do this would be to push the messages to a lockless fifo, but for the purposes of a demo, just posting them to the system queue is the same principal, but much easier to write.

3

Thanks Jules,

Maybe a comment in the code along the lines of:

/*
 This class receives MIDI signals from two sources:
     1. the device manager, via handleIncomingMidiMessage callback
     2. an on-screen midi keyboard, via handleNoteOn and handleNoteOff callbacks
 
 In each case, the message is ultimately handled thus:

     void addMessageToList (const MidiMessage& message)
     {
         midiMessageList.add (message);
         triggerAsyncUpdate();
     }
 
 However, there is a problem. The two sources will be running on different threads,
   and two threads could find themselves simultaneously in the above routine.
 
 To avoid this, each callback posts a message to a system thread.
 
 Additionally, handleIncomingMidiMessage disables handleNoteOn and handleNoteOff
 
 While this is probably not the only (or even best) way to accomplish the task
   (a critical section / spinlock in addMessageToList(...) would ensure that
   midiMessageList.add(...) is executed atomically), it serves the purpose of
   demonstrating several JUCE mechanisms.
 */

... may help future travellers.

I very much like the idea of favouring variety of mechanisms over optimal efficiency. This is the first time I have learned a technology through code, and as far as I'm concerned it is the best way to learn. Like teaching English through songs, all of the target material is presented in context.

Also what a great introduction to C++!

π

4

Thanks, but I thought I explained that you can't use a lock there! And a spinlock would be really bad!

The point is that the UI thread will block and hold that lock for a very long time (in audio terms) every time it updates the list. There will be a huge amount of contention on the lock, which will mess-up the audio thread. And spinlocks are only suitable for locks that have extremely low contention and which are never held for a long time.

The only "better" way of doing it would be with a lock-free fifo, which would be a ball-ache to write for such a simple demo class.

5

I didn't mean to spin around the UI update, just to spin around populating midiMessageList.add (message);
And then every screen refresh, just flush the list.

Arrrrrh! I think I see the problem. There is a possibility the list might be getting modified during refresh, i.e. while it is getting painted.

And the obvious way around that would be to make sure it gets modified on the same thread as the painting.

So there are TWO separate places where we get re-entrancy issues, requiring:
    1. adding messages from two sources to a single list
    2. making sure this list doesn't change during painting

One final question, if I may:

Does Juce have something akin to Unity3D's Update() that fires before screen refresh allowing all visual components to position themselves etc.?  On iOS it would be CADisplayLink()

6

Yes, that's what I was trying to explain!

No idea about Update(), as I'm not sure what it does.. Sounds like resized() to me?