Bug on the FluidSynth Android app

[dalewking]

I know this is not the correct place, but you have been unresponsive at contact@volcanomobile.net.

Your FluidSynth app has a bug in that if I am sequencing a Midi File and sending it to your app it will crash if the first things it sees are Meta messages it will crash, but if it sees a channel message first it doesn't crash.

For example opening a port in this Kotlin Android code:

    val port = device.openInputPort(0)

    port.send(byteArrayOf(0x90.toByte(), 0x60, 127), 0, 3)
    port.send(byteArrayOf(0xff.toByte(), 0x59, 2, 0 , 0), 0, 5)

This works, but if I comment out the first port.send call the app crashes.

I also had a feature request to allow specifying multiple soundfonts.

Are you still supporting that app?

[loki666]

I'm on it

[dalewking]

Any update on this?

[loki666]

not yet, didn't had much time to work on it,

I can send you a test project that reproduce the issue if you wanna help

[dalewking]

I am now at a point where I have the bandwidth to help with it.

[loki666]

ok let me wrap my project

[loki666]

https://github.com/loki666/MidiToolsTest

there is a UnitTest to play with

[dalewking]

OK, I am realizing now that the actual question is bogus. Part of the problem is the actual code that I am using to go from the sequencer (which is the javax.sound.midi port) and then I was just listening to events from its sequencer and forwarding them on to the MIDI port. However it is not valid to send META messages as runtime data. They do not by definition exist as runtime messages because the FF that signifies a META event is a runtime reset message.

So me sending the FF 59 02 00 00 was not really valid. It was not in fact the FF that was the problem, but your handling of running status when there actually has been no channel message before the data byte.

Here is the problem code:

        } else { // data byte
            if (!mInSysEx) {
                mBuffer[mCount++] = currentByte;
                if (--mNeeded == 0) {
                    if (mRunningStatus != 0) {
                        mBuffer[0] = mRunningStatus;
                    }
                    mReceiver.send(mBuffer, 0, mCount, timestamp);
                    mNeeded = MidiConstants.getBytesPerMessage(mBuffer[0]) - 1;
                    mCount = 1;
                }
            }
        }

This code will see the bytes 59, 02, 00, 00 but because there has been no channel message, mCount, mRunningStatus, and mNeeded are uninitialized so will be zero. This code will try to add bytes to mBuffer until mNeeded reaches zero, but it started at zero and wraps around. mBuffer is only 3 bytes so the 4th such byte causes an ArrayIndexOutOfBounds exception.

The reason it doesn't crash if I send a channel message first is because that initializes mCount, mRunningStatus, and mNeeded.

I think the easiest way to fix your code is just ignore the data byte unless mNeeded > 0.

Another issue is your handling of System Common messages being treated as having running status (i.e. allowing them to be sent again without a status byte). See the running status spec here. Quoting from that document:

System Common Category messages (ie, Status of 0xF0 to 0xF7) cancel any running status. In other words, the message after a System Common message must begin with a Status byte

I don't think you honor that. If I sent a System Common message and then followed that up with bytes < 0x80 that would cause the System Common message to be sent again. But I think that can be fixed like this:

        } else { // data byte
            if (!mInSysEx) {
                if(mNeeded > 0) {
                    mBuffer[mCount++] = currentByte;
                    if (--mNeeded == 0) {
                        if (mRunningStatus != 0) {
                            mBuffer[0] = mRunningStatus;
                            mNeeded = MidiConstants.getBytesPerMessage(mBuffer[0]) - 1;
                        }
                        mReceiver.send(mBuffer, 0, mCount, timestamp);
                        mCount = 1;
                    }
                }
            }
        }

I think you would also want some code in the F0 and F7 handling to set mNeeded to 0 so that it cancels the running status.

[dalewking]

I think there are issues with SysEx handling. Here is the spec, which says:

Furthermore, although the 0xF7 is supposed to mark the end of a SysEx message, in fact, any status (except for Realtime Category messages) will cause a SysEx message to be considered "done" (ie, actually "aborted" is a better description since such a scenario indicates an abnormal MIDI condition). For example, if a 0x90 happened to be sent sometime after a 0xF0 (but before the 0xF7), then the SysEx message would be considered aborted at that point.

Therefore this code:

            if (currentInt < 0xF0) { // channel message?
                mRunningStatus = currentByte;
                mCount = 1;
                mNeeded = MidiConstants.getBytesPerMessage(currentByte) - 1;
            } else if (currentInt < 0xF8) { // system common?

and this code:

                } else {
                    mBuffer[0] = currentByte;
                    mRunningStatus = 0;
                    mCount = 1;
                    mNeeded = MidiConstants.getBytesPerMessage(currentByte) - 1;
                }

should probably be setting mInSysEx to false.

[loki666]

well this parser is not mine, I took it from the Android MIDI sample code. But it is indeed a mess and should be rewritten...

You're welcomed to fork and PR the test project

[dalewking]

Do you have a link where you got that code?

[loki666]

https://github.com/philburk/android-midisuite/tree/master/MidiTools

[dalewking]

Unfortunately, just because it came from google it does not mean it isn't crappy. Luckily, it does work for normal midi message streams, but only has problems with somewhat malformed streams. Unfortunately there are also ways to see what looks malformed without the data being malformed (e.g. connecting to an ongoing stream already in progress)

Not the right place to ask it but I have an unrelated question. In the app you can select an output stream to connect to in the spinner. Is that in any way persisted such that it will maintain that connection, say across app restarts or rebooting the phone or is that just temporary? I ask because I am throwing together a little sequencer app to sequence midi files and I want to use your app to do the synthesis. My sequencer has an output port that I would like to always send to your app without my sequencer app having to know about the existence of your app. But that doesn't work if your connection to the output port is temporary.

[loki666]

I've published a fix in the open beta channel of FluidSynth MIDI (v1.1.35)

Let me know how it works

[dalewking]

Not a high priority for me any more since I was able to fix on my end by not sending you Meta messages any more which are not valid to send. And the issues only really happen when the framer sees malformed data (which could still occur naturally).

Still interested in an answer to my side question

[loki666]

The MIDI connection will stay alive if you put my app in background. But it won't reconnect automatically, for a simple reason, when the Synth is connected, it's running and takes a lot of resources (CPU and Audio). So you want to disconnect from it as soon as you are done with it (ie: when your app exit or goes to background) You can query the MidiManager about InputPorts available and connect to one when you app starts