Fatal problem for using MidiInput


#1

I found a problem with MidiInput on Mac OS X.
I wrote following code, that is delived from “StartingPoint” project.

class MainComponent	:	public Component, public MidiInputCallback
{
private:

public:

	//======================================================================
	MainComponent()
		:	Component( T("Main Component") )
	{
		StringArray mArray = MidiInput::getDevices();
		for (int i = 0; i < mArray.size(); ++i) {
			MidiInput* pInput = MidiInput::openDevice(i, this);
			pInput->start();
		}
	}

	~MainComponent()
	{
		
		deleteAllChildren ();
	}

	virtual void handleIncomingMidiMessage (MidiInput *source, const MidiMessage &message) {}
};

Bulid it and run the app and then receiving long SysEx causes operating system will be frozen for several seconds. The mouse cursor can’t move, MOTU 828 audio output is also frozen at that time. It seems that a kind of deadlock is happen. It doesn’t depend on the size of SysEx but by the timing. (Longer SysEx increases the probability.)

Without running this app, other apps (like MIDI Monitor) can receive SysEx properly. But when runnning this app, other apps also fail to receive SysEx.

My environment is,
Apple PowerBookG4(Latest model) 1.67GHz, 2GB RAM, 100GB HDD
(* Keyboard and TrackPad is connected by USB internally)
Mac OS X 10.4.3
Xcode 2.2
JUCE v1.22
MOTU 828mkII <-> KORG Z1
KORG microKONTROL <-> KORG MS2000

Does anyone can receive SysEx properly?

Best regards,
Masanao Hayashi
Korg Inc.


#2

I found the following workaround, and I could receive SysEx properly.
I don’t know what does the original code do…

[code]static void midiInputProc (const MIDIPacketList* pktlist,
void* readProcRefCon,
void* srcConnRefCon)
{
MidiPortAndCallback* const mpe = (MidiPortAndCallback*) readProcRefCon;
const ScopedLock sl (callbackLock);

if (activeCallbacks.contains (mpe) && mpe->active)
{
    const double time = Time::getMillisecondCounterHiRes() * 0.001;

    const MIDIPacket* packet = &pktlist->packet[0];

    for (unsigned int i = 0; i < pktlist->numPackets; ++i)
    {

        if (packet->length > 3)
        {
            const uint8* d = (const uint8*) (packet->data);
            int size = packet->length;
            do
            {
                int used = 0;
                const MidiMessage m (d, size, used, 0, time);

                mpe->callback->handleIncomingMidiMessage (mpe->input, m);

                size -= used;
                d += used;

            } while (size > 0);
        }
        else
        {
            const MidiMessage m ((const uint8*)(packet->data),
                                 packet->length,
                                 time);

            mpe->callback->handleIncomingMidiMessage (mpe->input, m);
        }

		const MidiMessage m ((const uint8*)(packet->data),
                                 packet->length,
                                 time);

		mpe->callback->handleIncomingMidiMessage (mpe->input, m);

        packet = MIDIPacketNext (packet);
    }
}

}
[/code]

Masanao Hayashi


#3

Well the code that you’ve commented out breaks up longer byte sequences into smaller messages, if that’s appropriate. But I guess there’s an infinite loop possible if the message isn’t parsed properly, and that must be what’s happening.

I think it needs to be able to break up messages, because I’m sure I remember some problems on tracktion where certain midi devices lumped several messages together into one packet, so it’s better to fix the code that’s there than to get rid of it. How about this:

        for (unsigned int i = 0; i < pktlist->numPackets; ++i)
        {
            const uint8* d = (const uint8*) (packet->data);
            int size = packet->length;

            if (size > 3)
            {
                // make sure the packet doesn't contain multiple messages that need
                // to be broken up into separate callbacks..
                do
                {
                    int used = 0;
                    const MidiMessage m (d, size, used, 0, time);

                    if (used == 0)
                    {
                        jassertfalse // malformed midi message
                        break;
                    }

                    mpe->callback->handleIncomingMidiMessage (mpe->input, m);

                    size -= used;
                    d += used;

                } while (size > 0);
            }
            else
            {
                const MidiMessage m (d, size, time);
                mpe->callback->handleIncomingMidiMessage (mpe->input, m);
            }

            packet = MIDIPacketNext (packet);
        }

What might be happening is that the driver is sending a packet containing a sysex with some random extra data at the end - if so, this code will work, but will throw an assertion, and you can have a look at what the packet contains. If it’s something that happens a lot, you could get rid of the assertion.


#4

Infinite loop??? Ah, that’s must the problem I had…

I didn’t see that but I saw 0xf8 and 0xfe was inserted int the SysEx data, in the same packet. I think CoreMIDI should get rid of it and pass it as independent packet immediately because that is a realtime message.

Masa


#5

Yes, some things do get stuffed in the same packet, probably by the driver rather than CoreMidi itself. I don’t even know if CoreMidi inspects the data to see if it’s proper midi.

Did you try the code I posted? I’m ready to release a new version today, and will include this unless you spot a bug in it!


#6

[quote=“jules”]Yes, some things do get stuffed in the same packet, probably by the driver rather than CoreMidi itself. I don’t even know if CoreMidi inspects the data to see if it’s proper midi.

Did you try the code I posted? I’m ready to release a new version today, and will include this unless you spot a bug in it![/quote]

I’ve just tried it, but the result was the same… The code I wrote (commented out) was worked fine at the moment.

Masa


#7

Ah - too late for the release!

But didn’t it throw the assertion? The only thing I could see that could go wrong is if the ‘used’ value was zero, and that would have looped. But my new code can’t do that - it always breaks out… ?


#8

[quote=“jules”]Ah - too late for the release!

But didn’t it throw the assertion? The only thing I could see that could go wrong is if the ‘used’ value was zero, and that would have looped. But my new code can’t do that - it always breaks out… ?[/quote]

Sorry for the late reply. I was busy today…

It didn’t seem to throw any assertion but frozen several seconds like former one. So I continue to use my workaround now.

I think other guys don’t deal with SysEx in the today’s our environment…

Masa

P.S.
I’m going to by MacBook Pro and get it in the next month!!
Are you ready???


#9

…ok, one last idea. I spotted a way that the ‘used’ variable could actually return -1 if the data’s really really messed up, so my fix should really have read:

                    if (used <= 0)
                    {
                        jassertfalse // malformed midi message
                        break;
                    }

instead of “if (used == 0)”

And yes, those MacBooks look pretty nice. Might have to treat myself to one of those…


#10

I am experiencing the opposite situation on the Mac. Rather than getting MIDI messages combined into one packet, I have a device that is streaming one SysEx message split over two or more packets, each as its own message:

first message (1 packet): F0 00 00
second message (1 packet): 66 17 01 48 55 31 30 30 30 32 F7

When the first message is received in midiInputProc(), it is immediately sent on as a MidiMessage to handleIncomingMidiMessage() because it is only 3 bytes long:

const MidiMessage m (d, size, time); mpe->callback->handleIncomingMidiMessage (mpe->input, m);

The message isn’t recognized by my code and ignored.

When the second message, larger than 3 bytes, is received in midiInputProc(), it also is turned into a MidiMessage, but it is thrown away because it is malformed:

[code] int used = 0;
const MidiMessage m (d, size, used, 0, time);

                if (used <= 0)
                {
                    jassertfalse // malformed midi message
                    break;
                }[/code]

So the split SysEx message is ultimately lost. I don’t know what the solution should be here. Perhaps letting the malformed MIDI message through to the handler regardless? I’m going to try that and let my handler see when there’s part of a SysEx message coming in and buffer it until the F7 (or some other status byte) comes.

This works on Windows, for some reason – maybe because the SysEx handler fills a buffer as it receives data and the MidiIN thread checks the buffer and builds a message from that?


#11

Nasty… let me know if you find a good fix for it.


#12

Here’s how I changed juce_mac_CoreMidi.cpp to handle incoming SysEx messages split across multiple packets/messages…

// *** Modification Start
static const int midiBufferSize = 512;
// *** Modification End

struct MidiPortAndCallback
{
    MidiInput* input;
    MIDIPortRef port;
    MIDIEndpointRef endPoint;
    MidiInputCallback* callback;
// *** Modification Start
    double pendingTime;
    int pendingLength;
    uint8 pending[midiBufferSize];
// *** Modification End
    bool active;
};

static CriticalSection callbackLock;
static VoidArray activeCallbacks;

static void midiInputProc (const MIDIPacketList* pktlist,
                           void* readProcRefCon,
                           void* srcConnRefCon)
{
    const double time = Time::getMillisecondCounterHiRes() * 0.001;

    MidiPortAndCallback* const mpe = (MidiPortAndCallback*) readProcRefCon;
    const ScopedLock sl (callbackLock);

    if (activeCallbacks.contains (mpe) && mpe->active)
    {
        const MIDIPacket* packet = &pktlist->packet[0];

        for (unsigned int i = 0; i < pktlist->numPackets; ++i)
        {
            const uint8* d = (const uint8*) (packet->data);
            int size = packet->length;

// *** Modification Start
            while (size > 0)
            {
                // make sure the packet doesn't contain multiple messages that need
                // to be broken up into separate callbacks... also handle the case
                // where the packet is a partial SysEx message
                int used = 0;
                const MidiMessage m (d, size, used, 0, time);

                if (used <= 0)
                {
                    // this is a malformed message, i.e. the first byte of data isn't a status byte.
                    // if we've got a pending SysEx message, we'll add this byte to the buffer;
                    // if there's no pending SysEx message, we'll ignore this byte
                    if (mpe->pendingLength > 0 && mpe->pendingLength < midiBufferSize)
                         mpe->pending[mpe->pendingLength++] = *d;

                    // move on to the next byte of data
                    size--;
                    d++;
                }
                else if (m.isSysEx() && m.getRawData()[used - 1] != 0xf7)
                {
                    // this is a partial SysEx message (it starts with 0xf0 but doesn't end with 0xf7),
                    // so save it as pending in case the rest of its data is following in a separate
                    // packet/message (note that previous pending data is thrown away).
                    if (used < midiBufferSize)
                    {
                        memcpy(mpe->pending, m.getRawData(), used);
                        mpe->pendingLength = used;
                        mpe->pendingTime = time;
                    }

                    // move past the used data on to next segment of data
                    size -= used;
                    d += used;
                }
                else if (used == 1 && m.getRawData()[0] == 0xf7)
                {
                    // this is the end of a SysEx message -- it will be ignored if there is no
                    // pending SysEx message
                    if (mpe->pendingLength > 0 && mpe->pendingLength < midiBufferSize)
                    {
                        // add this byte to the end of the pending SysEx message
                        mpe->pending[mpe->pendingLength++] = 0xf7;
 
                        // attempt to build a MIDI message with the pending SysEx data
                        const MidiMessage sysex (mpe->pending, mpe->pendingLength, used, 0, mpe->pendingTime);

                        // if it's valid, send it to our handler
                        if (used > 0)
                            mpe->callback->handleIncomingMidiMessage (mpe->input, sysex);
                    }

                    // move on to the next byte of data
                    size--;
                    d++;

                    // clear the pending message
                    mpe->pendingLength = 0;
                }
                else
                {
                    // a valid MIDI message was built so send it to our handler
                    mpe->callback->handleIncomingMidiMessage (mpe->input, m);

                    // move past the used data on to next segment of data
                    size -= used;
                    d += used;

                    // clear the pending message
                    mpe->pendingLength = 0;
                }
            }
// *** Modification End

            packet = MIDIPacketNext (packet);
        }
    }
}

MidiInput* MidiInput::openDevice (int index, MidiInputCallback* callback)
{
    MidiInput* mi = 0;

    if (index >= 0 && index < (int) MIDIGetNumberOfSources())
    {
        MIDIEndpointRef endPoint = MIDIGetSource (index);

        if (endPoint != 0)
        {
            CFStringRef pname;

            if (OK (MIDIObjectGetStringProperty (endPoint, kMIDIPropertyName, &pname)))
            {
                log (T("CoreMidi - opening inp: ") + PlatformUtilities::cfStringToJuceString (pname));

                if (makeSureClientExists())
                {
                    MIDIPortRef port;

                    MidiPortAndCallback* const mpe = new MidiPortAndCallback();
                    mpe->active = false;

                    if (OK (MIDIInputPortCreate (globalMidiClient, pname, midiInputProc, mpe, &port)))
                    {
                        if (OK (MIDIPortConnectSource (port, endPoint, 0)))
                        {
                            mpe->port = port;
                            mpe->endPoint = endPoint;
                            mpe->callback = callback;
// *** Modification Start
                            mpe->pendingLength = 0;
// *** Modification End							

                            mi = new MidiInput (getDevices() [index]);
                            mpe->input = mi;
                            mi->internal = (void*) mpe;

                            const ScopedLock sl (callbackLock);
                            activeCallbacks.add (mpe);
                        }
                        else
                        {
                            OK (MIDIPortDispose (port));
                            delete mpe;
                        }
                    }
                    else
                    {
                        delete mpe;
                    }
                }
            }

            CFRelease (pname);
        }
    }

    return mi;
}

So if the data has 0xf0 and there’s no 0xf7 following in the packet, the data is cached as pending. As long as “malformed” (no status byte) data streams in, it is added to the buffer. When 0xf7 shows up, the cached SysEx message is built and sent on.

One of my testers had a device send what is usually an 18-byte SysEx message as six 3-byte messages… I don’t know whether it’s the firmware or the midi driver causing this, but the above code handles it (and all other midi input) now.


#13

Nice one. I had a go at this too, and here’s my version. I think it’s a bit more generic than yours so might handle a few more cases. Let me know if you try it out (I can’t really test it here as my midi i/o is well-behaved):

[code]struct MidiPortAndCallback
{
MidiInput* input;
MIDIPortRef port;
MIDIEndpointRef endPoint;
MidiInputCallback* callback;
MemoryBlock pendingData;
int pendingBytes;
double pendingDataTime;
bool active;
};

static CriticalSection callbackLock;
static VoidArray activeCallbacks;

static void midiInputProc (const MIDIPacketList* pktlist,
void* readProcRefCon,
void* srcConnRefCon)
{
double time = Time::getMillisecondCounterHiRes() * 0.001;

MidiPortAndCallback* const mpe = (MidiPortAndCallback*) readProcRefCon;
const ScopedLock sl (callbackLock);

if (activeCallbacks.contains (mpe) && mpe->active)
{
    const MIDIPacket* packet = &pktlist->packet[0];

    for (unsigned int i = 0; i < pktlist->numPackets; ++i)
    {
        const uint8* d = (const uint8*) (packet->data);
        int size = packet->length;

        while (size > 0)
        {
            if (mpe->pendingBytes > 0)
            {
                // if there's some unfinished data pending from the last call, concatenate
                // it to our incoming data..
                mpe->pendingData.ensureSize (mpe->pendingBytes + size, false);

                memcpy (((char*) mpe->pendingData) + mpe->pendingBytes, d, size);
                size += mpe->pendingBytes;
                d = mpe->pendingData;
                mpe->pendingBytes = 0;
                time = mpe->pendingDataTime;
            }

            int used = 0;
            const MidiMessage m (d, size, used, 0, time);

            if (used <= 0)
            {
                jassertfalse // malformed midi message
                break;
            }
            else if (m.isSysEx() && d [used - 1] != 0x7f)
            {
                // this is an unfinished sysex, so stick it in the pending buffer..
                mpe->pendingData.ensureSize (used, false);

                memmove ((char*) mpe->pendingData, d, used);
                mpe->pendingBytes = used;
                mpe->pendingDataTime = time;
            }
            else
            {
                mpe->callback->handleIncomingMidiMessage (mpe->input, m);
            }

            size -= used;
            d += used;
        }

        packet = MIDIPacketNext (packet);
    }
}

}

MidiInput* MidiInput::openDevice (int index, MidiInputCallback* callback)
{
MidiInput* mi = 0;

if (index >= 0 && index < (int) MIDIGetNumberOfSources())
{
    MIDIEndpointRef endPoint = MIDIGetSource (index);

    if (endPoint != 0)
    {
        CFStringRef pname;

        if (OK (MIDIObjectGetStringProperty (endPoint, kMIDIPropertyName, &pname)))
        {
            log (T("CoreMidi - opening inp: ") + PlatformUtilities::cfStringToJuceString (pname));

            if (makeSureClientExists())
            {
                MIDIPortRef port;

                MidiPortAndCallback* const mpe = new MidiPortAndCallback();
                mpe->active = false;

                if (OK (MIDIInputPortCreate (globalMidiClient, pname, midiInputProc, mpe, &port)))
                {
                    if (OK (MIDIPortConnectSource (port, endPoint, 0)))
                    {
                        mpe->port = port;
                        mpe->endPoint = endPoint;
                        mpe->callback = callback;
                        mpe->pendingBytes = 0;
                        mpe->pendingData.ensureSize (128);

                        mi = new MidiInput (getDevices() [index]);
                        mpe->input = mi;
                        mi->internal = (void*) mpe;

                        const ScopedLock sl (callbackLock);
                        activeCallbacks.add (mpe);
                    }
                    else
                    {
                        OK (MIDIPortDispose (port));
                        delete mpe;
                    }
                }
                else
                {
                    delete mpe;
                }
            }
        }

        CFRelease (pname);
    }
}

return mi;

}
[/code]

and you’ll need this include as well:

#include "../../../src/juce_core/containers/juce_MemoryBlock.h"


#14

I can’t test it either as my midi is also well-behaved (I had to track all of this down by looking at midi i/o logs from out-of-country beta testers!), but it looks like it will work for most cases. I noticed a couple of things, however, one major and one minor.

  1. There’s the potential to get stuck in a loop if you get a pending sysex and no 0xf7 comes, i.e. another midi message streams in. For example, if your midi data stream is

F0 00 00 B0 12 34 … …

the pending buffer keeps getting loaded! And if another sysex comes in:

F0 00 00 B0 12 34 F0 00 12 F7

the whole thing is treated as one midi message! Really, this is a problem in the MidiMessage constructor:

[code] if (byte >= 0x80)
{
if (byte == 0xf0 || byte == 0xf7)
{
const uint8* d = (const uint8*) src;

        while (d < src + sz)
        {
            if (*d++ == 0xf7)
                break;
        }

        size = 1 + (int) (d - src);

        data = (uint8*) juce_malloc (size);
        *data = (uint8) byte;
        memcpy (data + 1, src, size - 1);
    }[/code]

The sysex should be aborted if another status byte appears, but here it only looks for a closing 0xf7.

  1. This is the minor one. If you have two midi messages come in like this:

F0 00 00 (time t1)
12 F7 B0 12 34 (time t2)

they are processed as:

F0 00 00 12 F7 (time t1)
B0 12 34 (time t1)

but I think t2 should be used for the latter’s time. Like I said… minor.


#15

Very good points - how about this solution that uses a timeout in case the data in the buffer gets stale… (have to admit I’ve not even tried compiling this, but it’s a fairly simple change)

[code]static void midiInputProc (const MIDIPacketList* pktlist,
void* readProcRefCon,
void* srcConnRefCon)
{
double time = Time::getMillisecondCounterHiRes() * 0.001;
const double originalTime = time;

MidiPortAndCallback* const mpe = (MidiPortAndCallback*) readProcRefCon;
const ScopedLock sl (callbackLock);

if (activeCallbacks.contains (mpe) && mpe->active)
{
    const MIDIPacket* packet = &pktlist->packet[0];

    for (unsigned int i = 0; i < pktlist->numPackets; ++i)
    {
        const uint8* d = (const uint8*) (packet->data);
        int size = packet->length;

        while (size > 0)
        {
            time = originalTime;

            if (mpe->pendingBytes > 0)
            {
                // in case a non-terminated sysex turns up, let's have a timeout
                // on how long the stuff in the buffer is valid for..
                if (time < mpe->pendingDataTime + 500)
                {
                    // if there's some unfinished data pending from the last call, concatenate
                    // it to our incoming data..
                    mpe->pendingData.ensureSize (mpe->pendingBytes + size, false);

                    memcpy (((char*) mpe->pendingData) + mpe->pendingBytes, d, size);
                    size += mpe->pendingBytes;
                    d = mpe->pendingData;
                    time = mpe->pendingDataTime;
                }
                else
                {
                    // could be caused by a very broken sysex message coming in, or by
                    // a device sending consecutive packets with a very long pause in between..
                    jassertfalse                                    
                }

                mpe->pendingBytes = 0;
            }

            int used = 0;
            const MidiMessage m (d, size, used, 0, time);

            if (used <= 0)
            {
                jassertfalse // malformed midi message
                break;
            }
            else if (m.isSysEx() && d [used - 1] != 0x7f)
            {
                // this is an unfinished sysex, so stick it in the pending buffer..
                mpe->pendingData.ensureSize (used, false);

                memmove ((char*) mpe->pendingData, d, used);
                mpe->pendingBytes = used;
                mpe->pendingDataTime = time;
            }
            else
            {
                mpe->callback->handleIncomingMidiMessage (mpe->input, m);
            }

            size -= used;
            d += used;
        }

        packet = MIDIPacketNext (packet);
    }
}

}
[/code]


#16

That will definitely clear out stale data, but it doesn’t solve the problem of losing information when a sysex message is interrupted. I think changing the MidiMessage constructor to stop a pending sysex message when a status byte occurs would do the trick and negate the need for a timeout… something like:

[code]MidiMessage::MidiMessage (const uint8* src,
int sz,
int& numBytesUsed,
uint8 lastStatusByte,
double t) throw()
: timeStamp (t),
message (0)
{
unsigned int byte = (unsigned int) *src;

if (byte < 0x80)
{
    byte = (unsigned int)(uint8) lastStatusByte;
    numBytesUsed = -1;
}
else
{
    numBytesUsed = 0;
    --sz;
    ++src;
}

if (byte >= 0x80)
{

// START MODIFICATION
if (byte == 0xf0)
{
const uint8* d = (const uint8*) src;

        while (d < src + sz)
        {
            if (*d++ >= 0x80)
            {
                // stop if we hit a status byte -- if it's not
                // the end of the sysex, don't include the byte
                if (*(d - 1) != 0xf7)
                    d--;

                break;
            }
        }

        size = 1 + (int) (d - src);

        data = (uint8*) juce_malloc (size);
        *data = (uint8) byte;
        memcpy (data + 1, src, size - 1);
    }
    else if (byte == 0xf7)
    {
        size = 1;
        data = (uint8*) &message;
        *data = (uint8) byte;
    }

// END MODIFICATION
else if (byte == 0xff)
{
int n;
const int bytesLeft = readVariableLengthVal (src + 1, n);
size = jmin (sz + 1, n + 2 + bytesLeft);

        data = (uint8*) juce_malloc (size);
        *data = (uint8) byte;
        memcpy (data + 1, src, size - 1);
    }
    else
    {
        static const char extraDataLengths[] = {
            2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
            2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
            2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
            2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
            1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
            1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
            2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
            0, 1, 2, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };

        size = extraDataLengths [byte - 0x80];
        data = (uint8*) &message;
        *data = (uint8) byte;

        if (size > 0)
        {
            data[1] = src[0];

            if (size > 1)
                data[2] = src[1];
        }

        ++size;
    }

    numBytesUsed += size;
}
else
{
    data = (uint8*) &message;
    message = 0;
    size = 0;
}

}[/code]


#17

Aren’t sysex messages allowed to contain bytes > 0x7f?? I thought they could do…


#18

Only MIDI realtime messages (0xF8 throuth 0xFF) can interrupt a MIDI message. If a sysex message is interrupted by any channel or system common message (0x80 through 0xF7), the message is considered terminated. Same goes for any other channel or system common message.

  • kbj

#19

Ok, cool. That’s a good fix then.


#20