mixedinkey-opensource / mikmidi Goto Github PK

See the MIDI Files Testbed project in the 1.1 branch to reproduce this. Maybe I've screwed something up...

My main concern here is that -noteEvents, etc. "pull" from the underlying data, and therefore aren't KVO compatible at all. Rather, I think we should read tracks at initialization (for sequences/tracks read from a file), and store them in our own property, then only allow inserts/removal through a well defined interface that maintains KVC/KVO compliance.

This will e.g. enable KVO to be used to easily trigger drawing updates for a view that draws a MIKMIDISequence/MIKMIDITracks.

I'm trying to write a MIDI app that would be visible via Network MIDI. From CoreMIDI documentation it appears that MIDINetworkSession has to be used for that. However MIKMIDI does does seem to have this implemented.
Any plans for adding network session support? Or is there a way to use MIDINetworkSession directly with MIKMIDI? Any help will be appreciated.

Currently, MIKMIDISequencer can only playback and record note events. It should be able to handle all MIDI event types.

Once MIKMIDISequencer is able to play back any event type MIKMIDIPlayer can be deprectated and eventually removed.

To reproduce, open MIDI Files Testbed, load a file, hit play, then click and hold on a button, or the devices dropdown, etc.

This would be useful for applications that wish to present MIDI source and destination endpoints to other applications. e.g. This could be used to create a virtual DJ controller application.

Right now, in order to take advantage of CoreMIDI's built in support for scheduling events in advance, we often create virtual endpoints (MIKMIDIClientDestinationEndpoints). This is easy, but it has the major disadvantage of "leaking" these endpoints to the outside world since they're visible to any app on the system.

In MIKMIDISequencer, we do have a need for advance scheduling, but hooking virtual endpoints up to MIKMIDIEndpointSynthesizer instances is clumsy.

I think we should create our own event scheduling code when events are just being used internally. Obviously, when we are actually sending events to an external endpoint (virtual or hardware), we want to continue using CoreMIDI's scheduling.

This thread has some useful information.

Right now, the API for selecting instruments is somewhat confusing, and not that well factored. For one thing, +availableInstruments probably shouldn't be a class method on MIKMIDISynthesizerInstrument, rather it should be an instance method on MIKMIDISynthesizer. Better documentation would help too.

Better documentation and/or example code for using MIKMIDIPlayer and related classes (e.g. MIKMIDISequence) would be very useful.

My MiniBrute is sending out Active Sense MIDI commands. They are recognised as such by MIDI Monitor (http://www.snoize.com/MIDIMonitor/), but when inspecting them in - (id)connectInput:(MIKMIDISourceEndpoint *)endpoint error:(NSError **)error eventHandler: (MIKMIDIEventHandlerBlock)eventHandler, they seem to have command type MIKMIDICommandTypeSystemMessage (0xff instead of 0xfe).

Other command types seem to be spot on (eg. CC, Pitch bend, Note On, etc).

Right now, for example, MIKMutableMIDIChannelVoiceCommand inherits from MIKMutableMIDICommand instead of MIKMIDIChannelVoiceCommand which means that the following code throws a warning (thought it works OK at runtime):

MIKMIDIChannelVoiceCommand *command = [[MIKMutableMIDIChannelVoiceCommand alloc] init];

First off, thanks for making this library available. It is so much nicer to use than the clunky CoreMIDI API!

The sendCommands:toDestination:error method in MIKMIDIOutputPort creates a MIDIPacketList using the default size (1 packet with 256 bytes of data). This causes an error if the commands passed in take up more memory than is available (either by having a lot of data in the packets or a lot of packets). The MIDIPacketList's size should be computed using the following computation:

ByteCount packetListSize = offsetof(MIDIPacketList, packet) + offsetof(MIDIPacket, data) * commands.count;

for (MIKMIDICommand* command in commands) {
    packetListSize += command.data.length;
}

packetListSize can then be used to allocate a block of memory to be used when copying data from the MIKMIDICommand objects to the MIDIPacketList.

The function that performs the copy (MIKMIDIPacketListFromCommands) isn't used anywhere else in the library so this function itself could be changed to actually allocate the MIDIPacketList. However, it's possible that others are making use of that function. In which case, a second function should be created that allocates a MIDIPacketList blob based on the contents of the array of MIKMIDICommands.

I can take a look into fixing this and submit a pull request. Let me know if you have any suggestions or preferences on the fix.

See the MIDI Files Testbed project in the 1.1 branch to reproduce this. Seems like it could be a pre-roll problem. Maybe I've screwed something up...

musicTimeStamp should just be timeStamp.

Also, because -initWithFileAtURL:error: calls through to it, currently it can/will return nil without setting its passed in error, which is plain wrong.

When playing notes through a software synth (via MIKMIDIEndpointSynthesizer), the sequencer should account for plugin latency times to ensure perfect timing in the audio output.

Ideally, the latency should be configurable so that any additional latency added by effects that are outside of the scope of this library could be included in the overall latency amount.

There are a few more 14-bit commands that MIKMIDI doesn't support coalescing. See http://www.midi.org/techspecs/midimessages.php#3 e.g. commands #98 and #99.

Even where MIKMIDI wraps a CoreMIDI function that returns a value passed by reference, unless there's a very pressing reason to do otherwise, I'd prefer to avoid pass by reference getters. This does of course require some thought about error handling. In cases where multiple specific errors may be generated, use a regular pass-by-reference NSError, but if possible returning a special/default value (or nil) and logging an error is preferable.

Some examples of this are -[MIKMIDITrack getTrackNumber:] and -[MIKMIDISequence getTimeSignature:atTimeStamp:].

This should be pretty straightforward. The main complication is implementing the initializer(s) correctly.

It might be advantageous for MIKMIDIObject to return an existing instance for a MIDIObjectRef that has already been used to create a MIKMIDIObject. That is, make it so there's a 1:1 correspondence between MIDIObjectRefs and MIKMIDIObject instances, rather than allowing multiple instances to represent the same underlying MIDI object.

The initial inspiration for this thought was an issue hit into in implementing Issue #6. The problem is that trying to create a MIKMIDIEndpoint instance from the object reference passed into the object removal notification when a virtual endpoint is removed fails. This is true even though there is an extant MIKMIDIEndpoint instance for that object ref.

I'm not sure yet if this makes sense, so it needs to be thought through more.

MIKMIDIMapping relies on NSXMLDocument, etc. for saving to/loading from disk, meaning it doesn't work on iOS. Either provide an alternate serialization strategy on iOS, or rewrite to remove the dependence on NSXMLDocument.

The following meta events still need classes:

Sequence Number
MIDI Channel Prefix
End Of Track
Set Tempo
SMTPE Offset
Sequencer Specific

Currently, when creating a new MIKMIDICommand programmatically (via +commandForCommandType:), the channel defaults to 15. This should be 0 instead.

The following Objective-C:

MIKMutableMIDIMetaTimeSignatureEvent *event = [[MIKMutableMIDIMetaTimeSignatureEvent alloc] init];
event.numerator = 4;

Yields the following warning in the console:

[NSConcreteMutableData replaceBytesInRange:withBytes:length:]: range {0, 1} exceeds data length 0

but does not crash. The following Swift:

let event = MIKMutableMIDIMetaTimeSignatureEvent()
event.numerator = UInt8(4)

Raises an NSRangeException:

*** Terminating app due to uncaught exception 'NSRangeException', reason: '*** -[NSConcreteMutableData subdataWithRange:]: range {8, 4026531840} exceeds data length 0'
*** First throw call stack:
(
    0   CoreFoundation                      0x000000010ad36b95 __exceptionPreprocess + 165
    1   libobjc.A.dylib                     0x000000010c88bbb7 objc_exception_throw + 45
    2   CoreFoundation                      0x000000010ad36acd +[NSException raise:format:] + 205
    3   Foundation                          0x000000010b1812b7 -[NSData(NSData) subdataWithRange:] + 252
    4   MIKMIDI                             0x000000010cacb24d -[MIKMIDIMetaEvent metaData] + 237
    5   MIKMIDI                             0x000000010cacdb53 -[MIKMIDIMetaTimeSignatureEvent setNumerator:] + 243

The length of the range (4026531840) is different every time I run it.

MIKMutableMIDIMetaTimeSignatureEvent, MIKMIDIMetaTimeSignatureEvent and MIKMIDIMetaEvent all don't have constructors, so it falls all the way back to MIKMIDIEvent, and initializes as a NULL event type with empty internalData. So when it accesses metadata, its dataLength is a garbage value. Or at least I think that's what's going on.

This causes a problem particularly for note messages where because the 3rd byte (dataByte2/velocity) doesn't exist, sending them to a MIDI endpoint may cause them to never be delivered.

This is just a nice to have. Instead of requiring the user of MIKMIDISequencer to connect to a device/endpoint and "manually" route messages to MIKMIDISequencer it would be cool to just give the sequencer the endpoint (or device) in question and have it connect to it. This should of course be optional in addition to the existing -recordMIDICommand: method.

Currently, MIKMIDIMappingManager uses <AppSupportFolder>/<AppBundleID>/MIDI Mappings to store and load user-supplied MIDI mapping files. This is customizable by subclassing MIKMIDIMappingManager, but it would be better if it could be customized without subclassing.

I'm not sure if this is actually practical, but I'd love to remove the mostly annoying distinction between MIDI messages (MIKMIDICommands) as used by devices, endpoints etc., and MIDI events (MIKMIDIEvents) as used by sequences, tracks, etc.

At the very least, it would be nice to make conversion between them easier. e.g. +[MIKMIDICommand commandsWithEvent:event] and vice versa.

Right now, internally, MIKMIDIDeviceManager supports multiple event handler blocks for a single connected input. This means that more than one object can connect to the same endpoint. However, the -disconnectInput: method removes all event handlers, so there's no way to selectively disconnect a single event handler while leaving other connected.

This needs to be fixed. It probably makes sense to return a token from -connectInput:error:eventHandler: that can be passed into the disconnect method to only disconnect that event handler.

The over-arching problem to #57. None of the subclasses, as far as I can tell, initialize with alloc/init of the proper eventType (and possibly other properties) because they all fall back to init in MIKMIDIEvent, which defaults to NULL. I'm not sure if this should be solved with a custom init in each subclass or a more unified approach like midiEventWithTimestamp:eventType:data:. Happy to help given some guidance on the preferred approach.

In particular, it would be good to have a high level overview of the MIDI responder architecture, MIDI routing, etc.

Right now, if you want to create e.g. an MIKMutableMIDIControlChangeCommand, you have to do this:

MIKMutableMIDIControlChangeCommand *newCommand = [MIKMutableMIDIControlChangeCommand commandForCommandType:MIKMIDICommandTypeControlChange];

It would be nicer to be able to do this:

MIKMutableMIDIControlChangeCommand *newCommand = [[MIKMutableMIDIControlChangeCommand alloc] init];

and have the resultant command simply be of the correct subclass and have its command type set automatically.

Unlike the availableDevices property, virtualSources and virtualDestinations don't update (and trigger KVO notifications) in response to new virtual endpoints being added to the system. This should be fixed.

This issue was raised by the fact that MIDIDestinationCreate() fails on iOS (6+) unless the audio key is set for UIBackgroundModes. See here:

Beginning in iOS 6, apps need to have the audio key in their UIBackgroundModes in order to use CoreMIDI’s MIDISourceCreate and MIDIDestinationCreate functions. These functions return kMIDINotPermitted (-10844) if the key is not set.

This also causes MIKMIDISequencer to fail on iOS unless that's set

We should make sure the error message for that particular case is descriptive to guide people toward the solution.

I'd like e.g. this code to just work:

NSError *error = nil;
MIKMIDISequence *sequence = [MIKMIDISequence sequenceWithFileAtURL:midiFileURL error:&error];
self.sequencer = [MIKMIDISequencer sequencerWithSequence:sequence];
[self.sequencer startPlayback];

On OS X 10.7, many classes that are likely to be the delegate of various MIKMIDI classes are likely to be classes that don't support weak references. In particular, NSViewController, NSWindow, and NSWindowController. Therefore, MIKMIDI uses unsafe_unretained references to delegates. If the app being built is to be deployed to 10.8 or higher, it would be better to automatically use weak references instead.

Xcode 5.1 extends support for Quick Look in the debugger to custom types. It'd be nice if at least MIKMIDICommand and its subclasses supported this. Other MIKMIDI classes might benefit from it too. See https://developer.apple.com/library/mac/documentation/IDEs/Conceptual/CustomClassDisplay_in_QuickLook/

The nullability annotations added to Objective-C with the introduction of Swift 1.2 should be added to MIKMIDI.

Especially important is discussion of what MIKMIDISequencer can be used for, how it differs from MIKMIDIPlayer, example code, etc.

Could probably even make it an NSEnumerator, though I'm not sure that really provides any benefit.