"add/remove" events options added to midi event list class

[john-davies]

The new options need the index into the events list ( and the new event in the "add" case ). If the operation is successful then the new size of the list is returned otherwise -1 is returned.

[craigsapp]

I will think more about the add function later, particularly if people whine for it... They will have to do a lot more whining to get the remove function, as the MidiMessage::clear() is a sufficient substitution.

[john-davies]

Thanks for catching the memory leak.

A couple of questions on add/delete:

1. Add
   I'm concerned about the "adding at the end then sorting" approach because, as far as I can see, the original sort order is not guaranteed. This could be a problem for messages which have the same time stamp but where the order is important.

2. Delete
   Am I correct in thinking that by using the clear() method any iteration through the message list now has to handle potentially empty messages? Also the size() method is now going to contain the deleted messages as well.

As regards performance I wrote a very simple test program to check the add() method by just adding 5000 program changes and 5000 notes to the start of a track:

Key part of code is below, I don't think that there's any optimisation going on that wouldn't be seen in more typical usage but I would welcome your comments:

  // Insert new elements
  MidiEvent new_prog( PROGRAM_CHANGE, 66 );
  MidiEvent new_note( 0x90, 64, 64 );

  auto start = std::chrono::system_clock::now();

  int j = 5000;
  for( int i=0; i<j; i++ )
  {
    midifile[1].add( 0, new_prog );
    midifile[1].add( 0, new_note );
  }

  auto end = std::chrono::system_clock::now();

Timings came out as follows. I think that the performance is OK ( PC spec is a 3GHz, i5, 16Gbytes RAM ). I haven't checked the deletes but I don't think that they will be much slower.

MIDI Tester, V1.0
***** Before inserts *****
Track 0 : Size = 4
Track 1 : Size = 148 <---------
Track 2 : Size = 173
Track 3 : Size = 182
Track 4 : Size = 182
Track 5 : Size = 179
***** After inserts *****
Track 0 : Size = 4
Track 1 : Size = 10148 <---------
Track 2 : Size = 173
Track 3 : Size = 182
Track 4 : Size = 182
Track 5 : Size = 179
Elapsed time: 0.0164614s

[craigsapp]

Delete
Am I correct in thinking that by using the clear() method any iteration through the message list now has to handle potentially empty messages? Also the size() method is now going to contain the deleted messages as well.

Yes, but if one of these two cases causes concern, you would run the MidiFile::removeEmpties() or MidiEventList::removeEmpties() functions before further processing of the contents. For deleting a single MIDI message, your implementation and mine are equivalent. But for deleting more than one message at the same time, the implementation that I did is preferable.

Also, if you are using a indexed for-loop through the events, deleting a MidiEvent from the list would invalidate the indexing (or you would have to be careful about it), but clearing a message in the list will keep the same indexing of events during the loop. For example, I have already used this new feature to remove all tempo meta-messages from a track in a simple loop:

https://github.com/craigsapp/midiroll/blob/548dc379f626eccefbc43b2a7088001a5c63f242/src/MidiRoll.cpp#L300-L318

void MidiRoll::removeAcceleration (void) {
	MidiRoll& mr = *this;
	for (int i=0; i<mr[0].size(); i++) {
		if (!mr[0][i].isTempo()) {
			continue;
		}
		mr[0][i].clear();
	}
	MidiFile::addTempo(0, 0, 60.0);
	MidiFile::sortTrack(0);
}

I am just leaving the MidiEvents that used to be tempo messages in the list, since writing out the MidiFile will ignore them. But I could have run MidiFile::removeEmpties() after the loop as well.

I then insert a new tempo marking which gets added to the end of the track, which results in me having to re-sort the track to move the tempo marking to the start of the file...

[craigsapp]

Add
I'm concerned about the "adding at the end then sorting" approach because, as far as I can see, the original sort order is not guaranteed. This could be a problem for messages which have the same time stamp but where the order is important.

Yes, the current system of appending new messages to the end of the track list and then sorting by absolute ticks is not a complete solution. So definitely an insert function should be added to the MidiEventList class. If you are adding content to existing MIDI file rather than creating MIDI files, then inserting would be much safer than appending and then sorting. My initial viewpoint when creating the midifile library was for MIDI file creation and data extraction, not editing and then saving the edited files.

I can think of two possibilities which can both be done: (1) adding the function from your PR, although I would call it insert rather than add to avoid confusion with the current add* functions, and (2) implement a sorting function that preserves the order of MidiEvents in a particular track occurring at a particular time. I should also probably change the add* function names to something else like push* to make their behavior more obvious (but that would introduce a minor backwards compatibility issue). The initial implementation of the midifile library did not used the standard template libraries, so the function names are not always consistent with STL names...

(1) is a general solution that should work in all situations since it give the best control for the addition of the MidiMessage, so it should, of course, be implemented. It can be abused by a programmer by causing inefficiencies when adding events to the list (the larger the file, the more bytes have to be moved around in memory), but in certain cases it will be the best option (such as for my code example from the previous message).

(2) would also solve the problem for most cases for which the current system will have problems. This solution would add an index variable to the MIDI event class. This index would be filled in before sorting, and during sorting this variable would be checked if there is a tie in the timestamp for two MidiMessages that occur at the same time. This would preserve the order of the events at the same time from before the sorting. I would also keep the previous sort method as one option for sorting:

midifile/src-library/MidiFile.cpp

Lines 2811 to 2872 in ccb93a2

	//////////////////////////////
	//
	// eventcompare -- Event comparison function for sorting tracks.
	//
	// Sorting rules:
	// (1) sort by (absolute) tick value; otherwise, if tick values are the same:
	// (2) end-of-track meta message is always last.
	// (3) other meta-messages come before regular MIDI messages.
	// (4) note-offs come after all other regular MIDI messages except note-ons.
	// (5) note-ons come after all other regular MIDI messages.
	//
	int eventcompare(const void* a, const void* b) {
	MidiEvent& aevent = **((MidiEvent**)a);
	MidiEvent& bevent = **((MidiEvent**)b);
	if (aevent.tick > bevent.tick) {
	// aevent occurs after bevent
	return +1;
	} else if (aevent.tick < bevent.tick) {
	// aevent occurs before bevent
	return -1;
	} else if (aevent.seq > bevent.seq) {
	// aevent sequencing state occurs after bevent
	// see MidiFile::markSequence()
	return +1;
	} else if (aevent.seq < bevent.seq) {
	// aevent sequencing state occurs before bevent
	// see MidiFile::markSequence()
	return -1;
	} else if (aevent[0] == 0xff && aevent[1] == 0x2f) {
	// end-of-track meta-message should always be last (but won't really
	// matter since the writing function ignores all end-of-track messages
	// and writes its own.
	return +1;
	} else if (bevent[0] == 0xff && bevent[1] == 0x2f) {
	// end-of-track meta-message should always be last (but won't really
	// matter since the writing function ignores all end-of-track messages
	// and writes its own.
	return -1;
	} else if (aevent[0] == 0xff && bevent[0] != 0xff) {
	// other meta-messages are placed before real MIDI messages
	return -1;
	} else if (aevent[0] != 0xff && bevent[0] == 0xff) {
	// other meta-messages are placed before real MIDI messages
	return +1;
	} else if (((aevent[0] & 0xf0) == 0x90) && (aevent[2] != 0)) {
	// note-ons come after all other types of MIDI messages
	return +1;
	} else if (((bevent[0] & 0xf0) == 0x90) && (bevent[2] != 0)) {
	// note-ons come after all other types of MIDI messages
	return -1;
	} else if (((aevent[0] & 0xf0) == 0x90) || ((aevent[0] & 0xf0) == 0x80)) {
	// note-offs come after all other MIDI messages (except note-ons)
	return +1;
	} else if (((bevent[0] & 0xf0) == 0x90) || ((bevent[0] & 0xf0) == 0x80)) {
	// note-offs come after all other MIDI messages (except note-ons)
	return -1;
	} else {
	return 0;
	}
	}

This sorting function tries to sort events occurring at the same time into a reasonable order, such as placing note-offs before note-ons when the pitch is the same. I have come across another case that is not yet handled properly: when a sustain pedal down and sustain pedal up occur at the same time, the up should be placed before the down. I will probably deal with such cases by sorting continuous controller messages by message number and then by data content to handle this cases and probably a few others like it.

[craigsapp]

Timings came out as follows. I think that the performance is OK ( PC spec is a 3GHz, i5, 16Gbytes RAM ). I haven't checked the deletes but I don't think that they will be much slower.

MIDI Tester, V1.0
***** Before inserts *****
Track 0 : Size = 4
Track 1 : Size = 148 <---------
Track 2 : Size = 173
Track 3 : Size = 182
Track 4 : Size = 182
Track 5 : Size = 179
***** After inserts *****
Track 0 : Size = 4
Track 1 : Size = 10148 <---------
Track 2 : Size = 173
Track 3 : Size = 182
Track 4 : Size = 182
Track 5 : Size = 179
Elapsed time: 0.0164614s

Computers are so fast and MIDI data is small (16 ms for 10000 added messages), so in principle the delete() function is not a problem. It is more of an aesthetic one. Also, if the inserted events are near the end of the list, then this will be more efficient than inserting at the beginning of the list each time.

It would be interesting to do a similar test by adding the messages to the end of the list and then sorting. Sorting is O(N * log N) while insertion is O(N). Multiple insertions would be O(N*N). For small N it will not matter (and the insertion method might be faster), but for large N, the sorting method would be better.

[craigsapp]

I see that I have already implemented case 2 in the previous-previous message in the last year or so, which is why it was on my mind...

Notice at the end of the MidiFile::read function there is this line:

midifile/src-library/MidiFile.cpp

Line 498 in 82baf1a

markSequence();

The markSequence function enumerates the MidiEvents in the MidiEventList:

midifile/src-library/MidiEventList.cpp

Lines 437 to 457 in 82baf1a

	//////////////////////////////
	//
	// MidiEventList::markSequence -- Assign a sequence serial number to
	// every MidiEvent in the event list. This is useful if you want
	// to preseve the order of MIDI messages in a track when they occur
	// at the same tick time. Particularly for use with joinTracks()
	// or sortTracks(). markSequence will be done automatically when
	// a MIDI file is read, in case the ordering of events occuring at
	// the same time is important. Use clearSequence() to use the
	// default sorting behavior of sortTracks() when events occur at the
	// same time. Returns the next serial number that has not yet been
	// used.
	// default value: sequence = 1.
	//
	int MidiEventList::markSequence(int sequence) {
	for (int i=0; i<getEventCount(); i++) {
	getEvent(i).seq = sequence++;
	}
	return sequence;
	}

This function stores a sequential serial number in the MidiEvent::seq variable that is used by the sort function to order events occurring at the same time in the sequence in originally read MidiFile.

The seq variable is the first feature that is used to sort the MidiEvents if the timestamp of events are the same:

midifile/src-library/MidiEventList.cpp

Lines 543 to 556 in 732faa3

	if (aevent.tick > bevent.tick) {
	// aevent occurs after bevent
	return +1;
	} else if (aevent.tick < bevent.tick) {
	// aevent occurs before bevent
	return -1;
	} else if (aevent.seq > bevent.seq) {
	// aevent sequencing state occurs after bevent
	// see MidiEventList::markSequence()
	return +1;
	} else if (aevent.seq < bevent.seq) {
	// aevent sequencing state occurs before bevent
	// see MidiEventList::markSequence()
	return -1;

So when calling MidiFile::sortTracks(), the sorting function will preserve the order of events occurring at the same time in a track by default, if the data was read from a file. When you do not want this behavior, then first call MidiFile::clearSequence before MidiFile::sortTracks. This will set all MidiEvent::seq variables to 0, and then the rest of the sorting rules will be used to order the events occurring at the same time:

midifile/src-library/MidiEventList.cpp

Lines 557 to 604 in 732faa3

	} else if (aevent.getP0() == 0xff && aevent.getP1() == 0x2f) {
	// end-of-track meta-message should always be last (but won't really
	// matter since the writing function ignores all end-of-track messages
	// and writes its own.
	return +1;
	} else if (bevent.getP0() == 0xff && bevent.getP1() == 0x2f) {
	// end-of-track meta-message should always be last (but won't really
	// matter since the writing function ignores all end-of-track messages
	// and writes its own.
	return -1;
	} else if (aevent.getP0() == 0xff && bevent.getP0() != 0xff) {
	// other meta-messages are placed before real MIDI messages
	return -1;
	} else if (aevent.getP0() != 0xff && bevent.getP0() == 0xff) {
	// other meta-messages are placed before real MIDI messages
	return +1;
	} else if (((aevent.getP0() & 0xf0) == 0x90) && (aevent.getP2() != 0)) {
	// note-ons come after all other types of MIDI messages
	return +1;
	} else if (((bevent.getP0() & 0xf0) == 0x90) && (bevent.getP2() != 0)) {
	// note-ons come after all other types of MIDI messages
	return -1;
	} else if (((aevent.getP0() & 0xf0) == 0x90) || ((aevent.getP0() & 0xf0) == 0x80)) {
	// note-offs come after all other MIDI messages (except note-ons)
	return +1;
	} else if (((bevent.getP0() & 0xf0) == 0x90) || ((bevent.getP0() & 0xf0) == 0x80)) {
	// note-offs come after all other MIDI messages (except note-ons)
	return -1;
	} else if (((aevent.getP0() & 0xf0) == 0xb0) && ((bevent.getP0() & 0xf0) == 0xb0)) {
	// both events are continuous controllers. Sort them by controller number
	if (aevent.getP1() > bevent.getP1()) {
	return +1;
	} if (aevent.getP1() < bevent.getP1()) {
	return -1;
	} else {
	// same controller number, so sort by data value
	if (aevent.getP2() > bevent.getP2()) {
	return +1;
	} if (aevent.getP2() < bevent.getP2()) {
	return -1;
	} else {
	return 0;
	}
	}
	} else {
	return 0;
	}
	}

There still is a need for MidiEventList::insert to insert a message at a specific position in the list (it would be possible with the sequence labeling method, but would be an inconvenience to set the seq variables for the file and coordinate with the new event that is to be added).

[john-davies]

Thanks for the very comprehensive replies. It all seems fine to me, a couple of minor comments:

1. I agree that insert is a better name than add. My use case for this one was that I wanted to insert a single program change event at the start of a track so adding at the end and then sorting was probably overkill.
2. You're correct about the deleting / for loop problem. Using an iterator would be better in this case.