mixedinkey-opensource / MIKMIDI

MIDI Library for Swift and Objective-C Mac and iOS apps.
Other
491 stars 95 forks source link

Issues with MIKMIDISequencer and notesOFF (plus potential solutions) #176

Open rrc2soft opened 7 years ago

rrc2soft commented 7 years ago

Hello everyone,

I am making a pet "simple composer" project for iOS using MIKMIDI (great library, btw), and while I was testing the underlying sequencer I found two issues related to noteOff:

1) If a note is very small (e.g. a demisemiquaver), the sequencer will not stop it - or even it will not play it. 2) MIKMIDINoteOffCommands seem to be triggered early by the underlying MIDI system. For example, if note 60 is supposed to finish at 1.5s, it will actually finish at 1.42s.

After various days of debugging, I think I found the underlying issues:

<506992297.688147>COMMAND: <MIKMIDINoteOffCommand: 0x7a6587d0> time: 00:11:37.278 command: 143 channel 0 note: 60 velocity: 0 data: <803c00> 2017-01-25 00:11:37.689 MUSIC Event:143 withP1: 60 withP2: 0

I think there are certain code changes that should be integrated into the current code base to solve these bugs: 1) The "pending Off events" management should be done after managing all other events (the 'for (MIKMIDITrack track in tracksToPlay)' loop). 2) Adding the 'note off' to the 'pending note off' list should be done when managing 'note on' events (within the 'for (MIKMIDITrack track in tracksToPlay)' loop), instead of inside the "scheduleEventWithDestination:" method. 3) Before sending the noteOff event to the "scheduleEventWithDestination:" method, a copy should be created with the correct timestamp, something like this:

noteOffEvent.event = [[MIKMIDINoteEvent alloc] initWithTimeStamp:((MIKMIDINoteEvent *)noteOffEvent.event).endTimeStamp midiEventType:noteOffEvent.event.eventType data:[noteOffEvent.event.data copy]]; [self scheduleEventWithDestination:noteOffEvent];

Sorry if I don't contribute the code to solve this (and for my not-so-good English), but it is late and I spent a lot of days debugging the code and trying to understand how MIKMIDISequencer works :-(. Also, I am afraid to introduce even more bugs, as I still do not understand why certain operations are performed.

rrc2soft commented 7 years ago

I made a couple of tests more and, if the "pending off events" are queued after managing all events (as I said in code change number 1), then we will have a problem with the order of contiguous notes, as notes will be triggered like this: ON---ON/OFF---ON/OFF---... therefore, contiguous notes will not be played after the first one. A solution might be to reorder the MIDI events before scheduling them, so OFF messages are sent first.

Sorry if I don't provide any code, but I still do not understand well how the class works, and I am afraid to break things even more :-/.

armadsen commented 6 years ago

Just a note to say that the second bug here was fixed by 242475f4 and the fix is in the 1.6.2 release. The first bug noted here is presumably still a problem. @cflesner is probably in a better position to fix this.

pnostudiodeveloper commented 5 years ago

Hi, I am still experiencing this bug using the MIKMIDI framework in an app - notes under 150 ms in length do not turn off when the MIKMIDISequence is played. I've been trying to work around it by manually increasing the note length in Logic, but its started to really be a problem. I am really hoping there might be some way to fix it, so that even very short notes still turn off? Thank you! Peter

armadsen commented 5 years ago

Sample project (provided by @pshannon1000). MIKMIDI Short Notes Sequence Workspace.zip

pnostudiodeveloper commented 5 years ago

Hello, I found that commenting out this single line from MIKMIDISequencer.m seems to fix the problem for me.

I don't understand all of the inner working of MIKMIDI, but as I've personally worked with MIDI, I've started to understand that what seems like a very certain and structured set of data over time, actually becomes more like target practice when you start trying to play back the data in time... the current time ends up being an interval based on your app's framerate, which is tricky for very fast on / off commands.

That's a tough idea to unpack, but it's why I think this section of code needs some clarification to work bug-free.

Maybe commenting out Line 332 of MIKMIDISequencer.m actually works, or maybe something else needs to be rethought.

For now I'm doing my build with line 332 of MIKMIDISequencer commented out...

MIKMIDISequencer.m 330 for (NSNumber *timeStampKey in [pendingNoteOffs copy]) { 331 MusicTimeStamp pendingNoteOffsMusicTimeStamp = timeStampKey.doubleValue; 332 //if (pendingNoteOffsMusicTimeStamp < fromMusicTimeStamp) continue; 333 if (pendingNoteOffsMusicTimeStamp > toMusicTimeStamp) continue;