MidiMessageSequence copy constructor has incorrect hidden side-effect

I am manually managing the noteOffObject of the MidiEventHolder as made quite easy with the return value of addEvent (as mentioned in this post from 2012, MidiMessageSequence and updateMatchedPairs). But the copy constructor for MidiMessageSequence has a hidden side effect that breaks this, and actual produces a copy that may not be the same as the source (ie. has noteOffObject filled in, even if the source didn’t). The issue is that the copy constructor calls updateMatchedPairs(). I have a proposed change that will create a real copy, by preserving the note connections, as they were in the source:

MidiMessageSequence::MidiMessageSequence (const MidiMessageSequence& other)
{
    list.addCopiesOf (other.list);

    auto eh = list.begin();
    for (auto otherEh : other.list)
    {
        if (otherEh->message.isNoteOn())
        {
            const auto noteOffIndex = other.getIndexOf (otherEh->noteOffObject);
            if (noteOffIndex != -1)
                (*eh)->noteOffObject = getEventPointer (noteOffIndex);
        }
        ++eh;
    }
}

Any chance this change could be made?

1 Like

I don’t think the class was ever really designed to be used like that, and to change the copy constructor in that way could break other people’s code, so no, it’s not really a feasible suggestion.

Perhaps the thing to discuss is what you’re doing manually in terms of matching the pairs, and whether the class could be extended to handle what you really need? e.g. perhaps it could take a lambda to override the algorithm it uses to do the pair matching, or something like that?

I think a bigger problem is within updateMatchedPairs itself.

As of today if it encounters two consecutive noteOn events with same note number and midi channel without a corresponding noteoff between it creates a new noteOff object and puts it between. Thus screwing up perfectly valid midi sequences.

There’s nothing wrong with consecutive noteOn events (with the same note number and midi channel) although some ambiguity might occur as to which noteOn a certain noteOff belongs.

The midi specification (the “real” one, downloadable from midi.org) even states something along the lines, -it’s up to the instrument to decide what to do when consecutive noteOn’s with same note number is received. So it’s not exactly any unheard of behaviour.

What the midi spec states is merely that for every note on event there should be a corresponding note off event. E.g you could stuff all your noteOffs at the end of the sequence and it would still be valid midi. I’m not saying it would be particularly useful though…

So why would you want to have consecutive noteOn events with out noteoffs between? There’s plenty of reasons.

You might just want to beef up your sound by doubling the notes, making more voices sound at the same time, at best giving you a chorus like effect (if your synth permits this of course, but then again, it’s up to the synth to decide).

You might have a guitar synth you want to mimic a 12 string guitar by doubling the notes/voices/strings.

In your drum synth you might want to play a cymbal crescendo by feeding it a bunch of noteOns with increasing velocity. This will probably sound daft if there was any intervening noteOffs between the strokes. At least if the drum machine implemented noteOffs to mimic hand muting.

I believe for a few versions ago (talking about ver 3 or 4), there was no call of updateMatchedPairs in midi file open, giving you the opportunity to use your own updateMatchedPairs(). I wish we could bring back that behaviour. Not only for things mentioned above but also because it’s probably rather time consuming (although I haven’t benchmarked it myself, I must confess).

It might not be noticeably when opening a single file, but if you have a midi library you might want to open hundreds of midi files to collect things like length, bpm, authors and other meta data to show in a browser or whatever. This would probably take some time, especially since updateMatchedPairs is executed twice for every track due to copying of the midi sequences.

So I guess this ends up in two humble suggestions… :slight_smile:

  • Make updateMatchedPairs non-manadatory again or at least make it possible to furnish your own updateMatchedPairs() function.

  • Skip the insertion of noteOffs between consecutive noteOns with same note number and channel in updateMatchedPairs

OK, that all sounds sensible. And actually, thinking again about cpr’s original suggestion, I might be wrong about it changing the behaviour of code, because it’s only in the case where someone was copying a non-matched sequence and forgetting to match it themselves that it’d cause problems. I’ll take a look and maybe implement something…

1 Like

Sounds fine! Here’s my non-destructive updateMatchedPairs if you need some inspiration…

{
	int lastNoteOff[17][128] = { 0 };
	const auto len = list.size();
	
	for (auto i = 0; i < len; ++i)
	{
		MidiEventHolder* const meh = list.getUnchecked(i);
		const MidiMessage& m1 = meh->message;

		if (m1.isNoteOn())
		{
			meh->noteOffObject = nullptr;
			const auto note = m1.getNoteNumber();
			const auto chan = m1.getChannel();
			auto& lastOff = lastNoteOff[chan][note];

			for (auto j = jmax(i, lastOff) + 1; j < len; ++j)
			{
				auto meh2 = list.getUnchecked(j);
				auto& m = meh2->message;

				if (m.getNoteNumber() == note && m.getChannel() == chan)
				{
					if (m.isNoteOff())
					{
						meh->noteOffObject = meh2;
						lastOff = j;
						break;
					}
					else if (m.isNoteOn())
					{
						DBG(String("NoteOn ") << note << " @ " << i << " repeated at index " << j << " channel " << chan);
					}
				}
			}

			if (!meh->noteOffObject)
			{
				// Oops! sequence is missing a note off message. 
				jassertfalse;	
			}
		}

	}
}

My original version of the fix included handling the scenario for copying an unmatchedpair source, and producing a matchedpair destination. But once I saw that the original code was wrong (ie. matching pairs on the destination when the source didn’t have them matched) I removed that code. But here is what that looked like:

idiMessageSequence::MidiMessageSequence (const MidiMessageSequence& other)
{
    list.addCopiesOf (other.list);

    auto numMatchedPairs = 0;
    auto eh = list.begin();
    for (auto otherEh : other.list)
    {
        if (otherEh->message.isNoteOn())
        {
            const auto noteOffIndex = other.getIndexOf (otherEh->noteOffObject);
            if (noteOffIndex != -1)
            {
                ++numMatchedPairs;
                (*eh)->noteOffObject = getEventPointer (noteOffIndex);
            }
        }
        ++eh;
    }

    if (numMatchedPairs == 0)
        updateMatchedPairs();
}

Hey Oxxyyd, Why does your lastNoteOff have room for 17 channels?

Midi channels are numbered 1 to 16.

holy crud, I never noticed JUCE bumped them by 1… I mean low midi messages use 0-15 (as it’s 4bit value), which is why I was confused.

Would you both be happy with this?

MidiMessageSequence::MidiMessageSequence (const MidiMessageSequence& other)
{
    list.addCopiesOf (other.list);

    for (int i = 0; i < list.size(); ++i)
    {
        if (auto* noteOff = other.list.getUnchecked(i)->noteOffObject)
        {
            auto noteOffIndex = other.getIndexOf (noteOff);
            jassert (noteOffIndex >= 0); // somehow got a dangling pointer to a note-off object!
            list.getUnchecked(i)->noteOffObject = list.getUnchecked (noteOffIndex);
        }
    }
}
1 Like

looks good to me!

Please. No. I don’t think that’s a very good idea.

I fired up my Graphics 49 and played a simple improvisation for 20s, engaging the pitch bend now and then. Resulted in 2300 midi events. Mostly aftertouch, but only channel ones, if it had been polyphonic, i.e individual for every note, the no of events would’ve been a lot more. Imaging a more talented performer playing for some 10 minutes. It might easily result in over 100 000 events. Per hand.

You’ve got the picture? Your code would do a linear search, if I read it correctly, wading through on average 50 000 events for every single note just to open the file. And likewise for any subsequent midimessageSequence copying you’d might wan’t to do (e.g copies for undo stuff etc).

So unless I’m doing some faulty assumptions above, my suggestion is that you just remove the updateMatchedPairs() from the MidiMessageSequence constructor (and the newly proposed code) just leaving list.addCopiesOf (other.list); i.e as it was some Juce versions ago. And leave a note to do updateMatchedPairs() when applicable, just as you do in addEvent() et al. This will also make it easy for everyone to use their own updateMatchedPairs() function.

Or at least, make a more efficent implementation… :slight_smile:

OK, fair enough, it could be made slightly more efficient, but I don’t think it’s viable to just do nothing, as that’d remove all the existing matched pairs, meaning that the copy isn’t really going to behave the same way that the original one did, and that could be a problem for people.

BTW maybe you didn’t notice that I only made it attempt to find a matching event if there was one for the original sequence, so it doesn’t matter how many non-note-on events there are in the original.

If you have a proper match for all noteons in the original, you do a search through all events for every noteon, don’t you? I see your point though, if you haven’t done any updatematchedPairs() at all you in the original, then there’s no search for any noteoffs. But uptil now updateMatchedPairs() have been non-optional, (if you didn’t patch the juce code), so there would always be a match.

If you start the search for the noteoff from the index of the noteOn, it would be a lot more efficient, I would say.

Yep, that’s fine - I’ll push a version that does this but begins the search at the note-on index…

1 Like