Midi still not on time


#1

It seems like sysex messages breaks midi file writing.


Look at the time stamp of the TimeSignature event at line 5 just after the sysex message, it have a time

stamp of 0.

=================
TextMetaEvent, kanal 0: Run For Your Life at 0, index: 0
TextMetaEvent, kanal 0: (C)1996 Eiko & Nobuo Takenaka at 0, index: 1
MetaEvent, kanal 0, typ 127 at 0, index: 2
MetaEvent, kanal 0, typ 33 at 0, index: 3
Sysex at 0, index: 4
-->TimeSignature kanal 0: 4/4 at 0, index: 5
SignatureNumberOfSharpsOrFlats, kanal0: 0 at 0, index: 6
Tempo, kanal 0: 87.0 at 0, index: 7
==================


After reading, then writing and then reading it again, the file will look like:

==================
TextMetaEvent, kanal 0: Run For Your Life at 0, index: 0
TextMetaEvent, kanal 0: (C)1996 Eiko & Nobuo Takenaka at 0, index: 1
MetaEvent, kanal 0, typ 127 at 0, index: 2
MetaEvent, kanal 0, typ 33 at 0, index: 3
Sysex at 0, index: 4
-->TimeSignature kanal 0: 4/4 at 20.8333, index: 5
SignatureNumberOfSharpsOrFlats, kanal0: 0 at 20.8333, index: 6
Tempo, kanal 0: 87.0 at 20.8333, index: 7
====================


See the Timsignature? It has now a time stamp of 20.8333 seconds. All subsequent events are postponed the same amount.

-Indeed a long intro...

Yes, and quite boring too. I believe the cause of this is the improper size of the sysex event given in the MidiMessage constructor when reading the midi file. The size includes the variable size bytes of the message, despite the fact they're removed in the reading process.

-You mean a sysex message that is read from the file as 0xf0 0x04 nn nn nn 0xf7, and is stored in juce as

0xf0 nn nn nn 0xf7 (omitting the size byte(s)) will have its size class member initialized to 6 instead of 5?

Exactly. And when the file later on is written back to file, the midiwriter will output the status byte (0xf7), decrement size to 5, store this as the byte count and finally output 5 sysex bytes although the original message only consisted of 4 sysex bytes.

-So the file will grow one byte?

Exactly. Or more if the varable length bytes are more than one.

And if the file then is read back with MidiFile::readNextTrack the sysex message...

-Will have grown one byte and when reading it the next time another one etc, etc?

No. It won't. In fact things would probably have been better if it had, but the MidiMessage constr ignores the sysex length byte(s) and just loops till it finds the sysex terminator, 0xf7, so the sysex message will stay intact after a read/write cycle (including its faulty size of 6 instead of 5).

-So what's the problem? Why the long intro?

The problem is the alien byte that was injected in the midi file when the sysex message, honoring its size member, saved 5 sysex bytes despite it only had 4 legitimate ones to save.

This extra byte will pop up as the first one of the next message now when the sysex message just have been read. If it's a byte >= 0x80 it will be interpreted as an additional length byte adding to the delta time of next message, hence all the remaining events will be delayed the same amount. That's the cause of the boring intro!

And if its < 0x80 or if the sysex length bytes were more than one, they will probably be interpreted as random messages and eventually lead to abortion of the reading when it no longer makes sense.

I've modified the MidiMessage constructor so it will size the sysex messages properly. After this the midifile reads and writes correctly. A least in this respect.

The midi annals also tells us about divided sysex messages, that is a sysex without a terminating 0xf7. Instead the 0xf7 is the start of next sys ex message, something like

0xf0 0x03 nn nn nn | 0xf7 0x03 nn nn nn | 0xf0 0x04 nn nn nn 0xf7

where the sysex bytes are spread out over 3 messages and only the last has a terminating 0xf7 byte. The current implementation of MidiMessage (which, as said, ignores the length byte(s)) would AFAIKT read up to and including the first 0xf7 found, which would be the status byte of the NEXT sysex message. It would then subsequently wrongly interpret the length byte of the second sysex message as the delta time byte of next event and most likely go silly after a while.

The modified MidiMessage::MidiMessage() won't read more bytes than what the length byte(s) stipulates, and hopefully it will now accept divided sysex messages, although I haven't yet tested it in real life.

Finally, line 402 in MidiFile::writeTrack will have to be updated to

else if (statusByte == 0xf0 || statusByte == 0xf7)

to allow for reading the divided sys ex messages.

==============================================================================================

MidiMessage::MidiMessage (const void* src_, int sz, int& numBytesUsed, const uint8 lastStatusByte, double t)
    : timeStamp (t),
      data (static_cast<uint8*> (preallocatedData.asBytes))
{
    const uint8* src = static_cast <const uint8*> (src_);
    unsigned int byte = (unsigned int) *src;

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

    if (byte >= 0x80)
    {
          int numVariableLengthSysexBytes = 0;

        if (byte == 0xf0 || byte == 0xf7)    //if prev sysex (0x7f0) was not terminated, assume it continues w/ a divided sysex message starting w/ 0xf7
        {
            const uint8* d = src;
            bool haveReadAllLengthBytes = false;
                //int numVariableLengthSysexBytes = 0; moved outside

                int numLengthBytes;
                const int sizeAsReadFromMessage = readVariableLengthVal (src, numLengthBytes);
                sz = jmin(sizeAsReadFromMessage + numLengthBytes, sz);    //limit the sys ex size to what it's length bytes say, allowing for divided sys ex messages w/o terminating 0xf7 byte

                for (; d < src + sz; ++d)
            {
                if (*d >= 0x80)
                {
                    if (*d == 0xf7)
                    {
                        ++d;  // include the trailing 0xf7 when we hit it
                        break;
                    }

                    if (haveReadAllLengthBytes) // if we see a 0x80 bit set after the initial data length
                        break;                  // bytes, assume it's the end of the sysex

                    ++numVariableLengthSysexBytes;
                }
                     else if (! haveReadAllLengthBytes)
                {
                    haveReadAllLengthBytes = true;
                    ++numVariableLengthSysexBytes;
                }
            }

                jassert (numLengthBytes == numVariableLengthSysexBytes)

            //size = 1 + (int) (d - src);    //wrong size
                size = 1 + (int) (d - src) - numVariableLengthSysexBytes;    //correct message size

                if (sizeAsReadFromMessage != size -1)
                {
                    DBG("Erroneous sys ex message size; announced size of sysex bytes: " << sizeAsReadFromMessage << ", actual found: " << size -1)
                }

            //data = new uint8 [size - numVariableLengthSysexBytes];
                data = new uint8 [size];
            *data = (uint8) byte;
            //memcpy (data + 1, src + numVariableLengthSysexBytes, (size_t) (size - numVariableLengthSysexBytes - 1));
                memcpy (data + 1, src + numVariableLengthSysexBytes, (size_t) (size - 1));
        }
        else if (byte == 0xff)
        {
            int n;
            const int bytesLeft = readVariableLengthVal (src + 1, n);
            size = jmin (sz + 1, n + 2 + bytesLeft);

            data = new uint8 [size];
            *data = (uint8) byte;
            memcpy (data + 1, src, (size_t) size - 1);
        }
        else
        {
            preallocatedData.asInt32 = 0;
            size = getMessageLengthFromFirstByte ((uint8) byte);
            data[0] = (uint8) byte;

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

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

        //numBytesUsed += size;
          numBytesUsed += size + numVariableLengthSysexBytes;    //adjust for the not stored but consumed size bytes
    }
    else
    {
        preallocatedData.asInt32 = 0;
        size = 0;
    }
}

 

 

 

 

 

 

 

 

 

 


#2

and if someone tells me how to do it, I'm perfectly willing to put the code in a code window.


#3

I actually didn't see this message until just now, but coincidentally I hit the same sysex length bug yesterday and fixed it - perhaps have a look at my changes and see if it has the same effect!

(And for pasting code, you want the "code block" option on the "paragraph" menu)


#4

Now the silence appears already when reading the file the first time. Prior to yesterday you at least had to save and reopen it to catch the bug.

Perhaps I wasn't clear but the problem is/was twofold; wrong size and the wrong number of bytes read. You cant just do

numBytesUsed += size;

You must do

numBytesUsed += size + numVariableLengthSysexBytes;

Previously size wrongly included numVariableLengthSysexBytes, therefore numBytesUsed was incremented correctly. But the wrong size lead to an incorrect saving which lead to subseq incorr reading.

The latest version has got the size ok (thanks) but now numBytesUsed is incorrectly increased, so the the next message read by readTrack will wrongly begin with the last byte of the sysex message -> boring intro already at first file read.

And the new version doesn't yet handle partial sysex messages. Why? You seem to be aware of them in MidiConcatenator::processSysex(). Are they banned from files and considered bad and only allowed in real time playing? Pardon me if that's the case. I'm no midi expert.

 

I've tried the code option. If you paste something in the single white line that appears in firefox more or less all spills over to the non code area. If you enter code manually you can't make a line break without using shift. In IE10 a resizable text area appears in which you can paste some code that will have double linebreaks. But even there if you preview the result, all code have poured out of the text area into the non code part. You get an error both in IE and in firefox so the problem shouldn't be that hard to catch. Tested in Windows 7.


#5

thanks, I've sorted out the numBytesUsed thing now.

I don't really see your point about trying to push all that other stuff into the MidiMessage constructor - it's just a constructor, not a stream-reader, and was only intended to parse a single event. I don't think any partial sysex or error-recovery stuff belongs in there, that kind of thing is generally done before the data gets as far as being turned into an actual MidiMessage.


#6

...but I get nevertheless some inconsistencies between my previous working solution (based on my MidiMessage() patch and a few weeks old juce) and the latest juce, which prevents some of the midi tracks from being saved correctly. Some small meta events with bytes 0 and 1 set to zeroo seem to turn up. I'll try to get some time to sort that out the coming days.

As for the sysex messages (divided or not) my only point is that its variable byte length field is there for a reason, and if using that length info, just as you do with the meta events, to limit the number of read bytes, will enable you to read files which would otherwise be unreadable, I'd be pragmatic enough to add one or two lines of code to MidiMessage(), rather than add an extra sysex preprocessor stage in the midifile read code. If that's the options.


#7

In readNextTrack a message like

0xff | 0x20 | 0x01 | 0x00

will be parsed and stored in MidiMessage.allocatedData and have a size of 4

Then this MidiMessage is added/copied to the MidiMessageSequence via

MidiMessage::MidiMessage (const MidiMessage& other)
   : timeStamp (other.timeStamp), size (other.size)
{
    if (size > 4)
    {
        allocatedData.malloc (size);
        memcpy (allocatedData, other.allocatedData, (size_t) size);
    }
    else
    {
        preallocatedData.asInt32 = other.preallocatedData.asInt32;
    }
}

As you can see nothing will be copied because this message wasn't stored in preallocatedData but rather  in allocatedData. That's why all these new empty midi messages appears.

You'll have to do something like

MidiMessage::MidiMessage (const MidiMessage& other)
   : timeStamp (other.timeStamp), size (other.size)
{
    if (size > 4)
    {
        allocatedData.malloc (size);
        memcpy (allocatedData, other.allocatedData, (size_t) size);
    }
    else
    {
        if (other.allocatedData != nullptr)
           memcpy (&preallocatedData.asInt32, other.allocatedData, (size_t) size);
        else
           preallocatedData.asInt32 = other.preallocatedData.asInt32;
    }
}

not only here but probably in many other places where you formerly used setToUseInternalData(). I haven't dived any further into the intricacies of the midimessage handling so I can't say if this change will break any other things.

 


#8

Ah, excellent catch, many thanks!

I've fixed it up now - I'm sure you'll let me know if you spot any more bloopers!


#9

No shit, man!


#10

At least as I expect. Thanks, Jules for a fantastic state-of-the-art framework and examplary fast bug response. And I'll sure let you know when midi breaks again...


#11

Nice to hear that you are managed to catch and fix a midi problem in this absolutly fantastic state-of-the-art framework!!!

As I'am trying to write a midi processing plugin with it. :)