MidiDataConcatenator issue with midi clock

#1

Hi Jules,

On macos, if I send this midi message :

0x90, 60, 0xF8, 48

(that is , note-on for note 60, velocity 48, with a midi clock message that happened inside the note-on)

The same bytes are received in the MidiPortCallback::handlePackets, in a single packet, which is sent into MidiDataConcatenator , which fails to deal with the midi clock message, and builds this single MidiMessage : 0x90 60 0xF8 ( instead of 0xF8, followed by 0x90 60 48)

This does not happen on windows, probably because somewhere inside the OS the midi packets are parsed and tokenized, while on macos you seem to get what you sent.

In fact it seems also that midi running status does not work, and MidiDataConcatenator should handle it:

if I send 0x90, 60, 48, 61, 49, 62, 50 (that is note-on 60 with vel 48, note-on 61 vel 49 and note-on 62 vel 50), on Windows the midi input callback ends up with

0x90 60 48
0x90 61 49
0x90 61 50

while on Macos, I only get 0x90 60 48 and then the MidiDataConcatenator throws up the rest of the packet

I must add that this is a not a purely theorical issue, since I have some users seem to run into this (when their midi keyboard sends midi clock messages over a good old midi cable)

So, to summarize:

#2

Surely 0x90, 60, 0xF8, 48 is illegal? How could I possibly make the parser smart enough to understand messages that are jammed inside other ones??

#3

the “realtime” 1-byte messages, 0xF8…0xFe , are allowed to appear anywhere inside another message , that is a legacy from the time where midi was slow I think :slight_smile: Just like this running status thing.

I’ll suggest a patch to midi concatenator if you are interested – I’m working on it right now

#4

Damn, that’s a pain to deal with! Thanks, I’d be keen to see what you come up with, I’m not really sure where the best place would be to put the logic to handle that.

#5

Here is my version for MidiDataConcatenator :

    void pushMidiData (const void* data, int numBytes, double time,
                       MidiInput* input, MidiInputCallback& callback)
    {
        const uint8* d = static_cast <const uint8*> (data);

        while (numBytes > 0)
        {
            if (pendingBytes > 0 || d[0] == 0xf0)
            {
                processSysex (d, numBytes, time, input, callback);
                running_status = 0;
            }
            else
            {
              /* handle a single non-realtime message, with any number of
                 of one-byte realtime messages that can be embedded in the non-realtime message 
              */
              int len = 0;
              uint8 data[3]; 
              while (numBytes > 0) {
                if (d[0] >= 0xf8 && d[0] <= 0xfe) {
                  const MidiMessage m (d[0], time);
                  callback.handleIncomingMidiMessage(input, m);
                  ++d; --numBytes;
                } else {
                  if (len == 0 && d[0] < 0x80 && running_status >= 0x80) { // use running status
                    data[0] = running_status;
                    data[1] = d[0]; 
                    len = 2;                    
                  } else {
                    jassert(len <= 3); // should never happen since getMessageLengthFromFirstByte <= 3
                    data[len++] = d[0]; 
                  }
                  ++d; --numBytes;
                  if (len == MidiMessage::getMessageLengthFromFirstByte(data[0])) {
                    break;
                  }
                }
              }
              if (len) {
                int used = 0;
                const MidiMessage m (data, len, used, 0, time);

                if (used <= 0)
                    break; // malformed message..
                jassert(used == len);
                callback.handleIncomingMidiMessage (input, m);
                running_status = data[0];
              }
            }
        }
    }

It handles midi realtime messages, and running status. The part that deals with sysex messages already handles midi realtime messages 0xF8 and 0xFA…0xFE , but I’m not sure why 0xF9 is not included, so maybe you should add it.

I’m testing all this by opening a MidiOutput and sending midi packets with:

uint8_t r[] = { 0x90, 60, 0xf8, 48, 61, 49, 62, 0xf9, 50 };
m = MidiMessage(r, sizeof r); 
midi_output->sendMessageNow(m);

However, there is another issue with CoreMidi when sending large packets that are not sysex. In MidiOutput::sendMessageNow , the non-sysex messages are sent with:

  else {
        MIDIPacketList packets;
        packets.numPackets = 1;
        packets.packet[0].timeStamp = timeStamp;
        packets.packet[0].length = message.getRawDataSize();
        *(int*) (packets.packet[0].data) = *(const int*) message.getRawData();
        mpe->send (&packets);
  }

So they only send correctly the 4 first bytes and the rest is garbage. You should change it to

     else if (message.getRawDataSize() <= 65536) // max packet size
    {
      MIDIPacketList packets_;
      int default_packet_capacity = (int)sizeof packets_.packet[0].data; 
      jassert(default_packet_capacity == 256);
      MIDIPacketList *p = &packets_;
      if (message.getRawDataSize() > default_packet_capacity) {
        p = (MIDIPacketList*)malloc(sizeof(MIDIPacketList) + (message.getRawDataSize() - default_packet_capacity));
      }
      p->numPackets = 1;
      p->packet[0].timeStamp = timeStamp;
      p->packet[0].length = message.getRawDataSize();
      memcpy(p->packet[0].data, message.getRawData(), message.getRawDataSize());
      mpe->send (p);
      if (p != &packets_) free(p);
    } else jassert(0); // GIANT PACKET cannot be sent
}
#6

Ok, thanks, I’ll take a look!

#7

A similar change can be done to the linux ALSA midi output code, not very important but it is nice to have the same behaviour on all 3 platforms. For now, it sends only the first midi event in a “MidiMessage”, even if the MidiMessage raw bytes hold more than one event. The change to make it work is:

         snd_seq_event_t event;
         snd_seq_ev_clear (&event);

-        snd_midi_event_encode (midiParser,
-                               message.getRawData(),
-                               message.getRawDataSize(),
-                               &event);
+        long numBytes = message.getRawDataSize();
+        const uint8 *data = message.getRawData();
+
+        while (numBytes > 0) {
+          long nb = snd_midi_event_encode (midiParser,
+                                           data, numBytes,
+                                           &event);
+
+          if (nb <= 0) break;
+          else { numBytes -= nb; data += nb; }
+
+          snd_seq_ev_set_source (&event, 0);
+          snd_seq_ev_set_subs (&event);
+          snd_seq_ev_set_direct (&event);
+
+          snd_seq_event_output (seqHandle, &event);
+        }
+        snd_seq_drain_output (seqHandle);

         snd_midi_event_reset_encode (midiParser);
-
-        snd_seq_ev_set_source (&event, 0);
-        snd_seq_ev_set_subs (&event);
-        snd_seq_ev_set_direct (&event);
-
-        snd_seq_event_output (seqHandle, &event);
-        snd_seq_drain_output (seqHandle);
     }
#8

Excellent, thanks!

#9

Oops, just noticed that a side effect of the change to MidiDataConcatenator is to send some spurious midi events on win32:

void handleMessage (const uint32 message, const uint32 timeStamp) { if ((message & 0xff) >= 0x80 && isStarted) { concatenator.pushMidiData (&message, 3, convertTimeStamp (timeStamp), input, callback); writeFinishedBlocks(); } }

this code assumes that the midi message is always 3 bytes long – in the case of AfterTouch which is two bytes, it sends a spurious aftertouch(0) (because of running status)

The fix is to replace the “3” here with: MidiMessage::getMessageLengthFromFirstByte ((message & 0xff)

#10

Ah… ok, thanks!