From afc9f20da51170b1639ed3e3b05c05a562f4a641 Mon Sep 17 00:00:00 2001 From: Patrick Reisert Date: Tue, 18 Jul 2017 17:29:18 +0200 Subject: [PATCH 1/9] Use upgraded version of coremidi-sys --- Cargo.toml | 1 - src/client.rs | 8 ++-- src/coremidi_sys_ext.rs | 79 ---------------------------------------- src/endpoints/sources.rs | 6 +-- src/lib.rs | 15 ++------ src/notifications.rs | 4 +- src/packets.rs | 11 ++---- src/ports.rs | 2 +- 8 files changed, 16 insertions(+), 110 deletions(-) delete mode 100644 src/coremidi_sys_ext.rs diff --git a/Cargo.toml b/Cargo.toml index 3d52e4743..350ffe256 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,4 +17,3 @@ core-foundation = "0.2" coremidi-sys = "1.0.0" # coremidi-sys = { git = "https://github.com/chris-zen/coremidi-sys", branch="fix-packed-structs" } time = "0.1" -libc = "0.2" diff --git a/src/client.rs b/src/client.rs index 135715042..a65baa603 100644 --- a/src/client.rs +++ b/src/client.rs @@ -6,7 +6,7 @@ use coremidi_sys::{ MIDIPortRef, MIDIOutputPortCreate, MIDIEndpointRef, MIDISourceCreate }; -use coremidi_sys_ext::{ +use coremidi_sys::{ MIDIPacketList, MIDIInputPortCreate, MIDIDestinationCreate }; @@ -151,7 +151,7 @@ impl Client { extern "C" fn notify_proc( notification_ptr: *const MIDINotification, - ref_con: *mut ::libc::c_void) { + ref_con: *mut ::std::os::raw::c_void) { let _ = ::std::panic::catch_unwind(|| unsafe { match Notification::from(&*notification_ptr) { @@ -165,8 +165,8 @@ impl Client { extern "C" fn read_proc( pktlist: *const MIDIPacketList, - read_proc_ref_con: *mut ::libc::c_void, - _: *mut ::libc::c_void) { //srcConnRefCon + read_proc_ref_con: *mut ::std::os::raw::c_void, + _: *mut ::std::os::raw::c_void) { //srcConnRefCon let _ = ::std::panic::catch_unwind(|| unsafe { let packet_list = PacketList(pktlist); diff --git a/src/coremidi_sys_ext.rs b/src/coremidi_sys_ext.rs deleted file mode 100644 index ec21e8ed1..000000000 --- a/src/coremidi_sys_ext.rs +++ /dev/null @@ -1,79 +0,0 @@ -#![allow(non_snake_case, non_upper_case_globals, non_camel_case_types)] - -use core_foundation_sys::base::OSStatus; -use core_foundation_sys::string::CFStringRef; - -use coremidi_sys::{ - UInt16, UInt32, Byte, MIDITimeStamp, - MIDIClientRef, MIDIEndpointRef, MIDIPortRef -}; - -use std::mem; - -pub type MIDIReadProc = - ::std::option::Option ()>; - -pub const MAX_PACKET_DATA_LENGTH: usize = 0xffffusize; - -#[repr(C)] -#[repr(packed)] -#[derive(Copy)] -pub struct Struct_MIDIPacket { - pub timeStamp: MIDITimeStamp, - pub length: UInt16, - pub data: [Byte; MAX_PACKET_DATA_LENGTH], -} -impl ::std::clone::Clone for Struct_MIDIPacket { - fn clone(&self) -> Self { *self } -} -impl ::std::default::Default for Struct_MIDIPacket { - fn default() -> Self { unsafe { ::std::mem::zeroed() } } -} - -pub type MIDIPacket = Struct_MIDIPacket; - -#[repr(C)] -#[repr(packed)] -#[derive(Copy)] -pub struct Struct_MIDIPacketList { - pub numPackets: UInt32, - pub packet: [MIDIPacket; 1usize], -} -impl ::std::clone::Clone for Struct_MIDIPacketList { - fn clone(&self) -> Self { *self } -} -impl ::std::default::Default for Struct_MIDIPacketList { - fn default() -> Self { unsafe { ::std::mem::zeroed() } } -} - -pub type MIDIPacketList = Struct_MIDIPacketList; - -extern "C" { - pub fn MIDISend(port: MIDIPortRef, dest: MIDIEndpointRef, - pktlist: *const MIDIPacketList) -> OSStatus; - - pub fn MIDIReceived(src: MIDIEndpointRef, pktlist: *const MIDIPacketList) -> OSStatus; - - pub fn MIDIInputPortCreate(client: MIDIClientRef, portName: CFStringRef, - readProc: MIDIReadProc, - refCon: *mut ::libc::c_void, - outPort: *mut MIDIPortRef) -> OSStatus; - - pub fn MIDIDestinationCreate(client: MIDIClientRef, name: CFStringRef, - readProc: MIDIReadProc, - refCon: *mut ::libc::c_void, - outDest: *mut MIDIEndpointRef) -> OSStatus; - - pub fn MIDIPacketListInit(pktlist: *mut MIDIPacketList) -> *mut MIDIPacket; -} - -pub unsafe fn MIDIPacketNext(packet: *const MIDIPacket) -> *const MIDIPacket { - let ptr = packet as *const u8; - let offset = mem::size_of::() as isize - + mem::size_of::() as isize - + (*packet).length as isize; - ptr.offset(offset) as *const MIDIPacket -} diff --git a/src/endpoints/sources.rs b/src/endpoints/sources.rs index 70c7781d3..d3d30ab0b 100644 --- a/src/endpoints/sources.rs +++ b/src/endpoints/sources.rs @@ -1,11 +1,7 @@ use core_foundation_sys::base::OSStatus; use coremidi_sys::{ - MIDIGetNumberOfSources, MIDIGetSource, MIDIEndpointDispose, ItemCount -}; - -use coremidi_sys_ext::{ - MIDIReceived + MIDIGetNumberOfSources, MIDIGetSource, MIDIReceived, MIDIEndpointDispose, ItemCount }; use std::ops::Deref; diff --git a/src/lib.rs b/src/lib.rs index 6223b3538..e83cebdf9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -41,16 +41,11 @@ For handling low level MIDI data you may look into: extern crate core_foundation_sys; extern crate core_foundation; extern crate coremidi_sys; -extern crate libc; use core_foundation_sys::base::OSStatus; use coremidi_sys::{ - MIDIObjectRef, MIDIFlushOutput, MIDIRestart -}; - -use coremidi_sys_ext::{ - MIDIPacketList + MIDIObjectRef, MIDIFlushOutput, MIDIRestart, MIDIPacketList }; /// A [MIDI Object](https://developer.apple.com/reference/coremidi/midiobjectref). @@ -89,12 +84,12 @@ impl BoxedCallback { BoxedCallback(::std::ptr::null_mut()) } - fn raw_ptr(&mut self) -> *mut ::libc::c_void { - self.0 as *mut ::libc::c_void + fn raw_ptr(&mut self) -> *mut ::std::os::raw::c_void { + self.0 as *mut ::std::os::raw::c_void } // must not be null - unsafe fn call_from_raw_ptr(raw_ptr: *mut ::libc::c_void, arg: &T) { + unsafe fn call_from_raw_ptr(raw_ptr: *mut ::std::os::raw::c_void, arg: &T) { let callback = &mut *(raw_ptr as *mut Box); callback(arg); } @@ -222,8 +217,6 @@ pub struct Device { object: Object } #[derive(PartialEq)] pub struct PacketList(*const MIDIPacketList); -mod coremidi_sys_ext; - mod object; mod devices; mod client; diff --git a/src/notifications.rs b/src/notifications.rs index 8b1fbf4e6..24bf22135 100644 --- a/src/notifications.rs +++ b/src/notifications.rs @@ -62,7 +62,7 @@ pub enum Notification { impl Notification { pub fn from(notification: &MIDINotification) -> Result { - match notification.messageID as ::libc::c_uint { + match notification.messageID as ::std::os::raw::c_uint { kMIDIMsgSetupChanged => Ok(Notification::SetupChanged), kMIDIMsgObjectAdded | kMIDIMsgObjectRemoved => Self::from_object_added_removed(notification), kMIDIMsgPropertyChanged => Self::from_property_changed(notification), @@ -84,7 +84,7 @@ impl Notification { child: Object(add_remove_notification.child), child_type: child_type.unwrap() }; - match notification.messageID as ::libc::c_uint { + match notification.messageID as ::std::os::raw::c_uint { kMIDIMsgObjectAdded => Ok(Notification::ObjectAdded(add_remove_info)), kMIDIMsgObjectRemoved => Ok(Notification::ObjectRemoved(add_remove_info)), _ => Err(0) // Never reached diff --git a/src/packets.rs b/src/packets.rs index a9bdaad77..9813028ab 100644 --- a/src/packets.rs +++ b/src/packets.rs @@ -1,9 +1,5 @@ use coremidi_sys::{ - MIDITimeStamp, UInt16 -}; - -use coremidi_sys_ext::{ - MIDIPacketList, MIDIPacket, MIDIPacketListInit, MIDIPacketNext, MAX_PACKET_DATA_LENGTH + MIDITimeStamp, UInt16, MIDIPacketList, MIDIPacket, MIDIPacketListInit, MIDIPacketNext }; use std::fmt; @@ -14,6 +10,8 @@ use PacketList; pub type Timestamp = u64; +const MAX_PACKET_DATA_LENGTH: usize = 0xffffusize; + /// A collection of simultaneous MIDI events. /// See [MIDIPacket](https://developer.apple.com/reference/coremidi/midipacket). /// @@ -269,8 +267,7 @@ impl Deref for PacketBuffer { #[cfg(test)] mod tests { - use coremidi_sys::MIDITimeStamp; - use coremidi_sys_ext::MIDIPacketList; + use coremidi_sys::{MIDITimeStamp, MIDIPacketList}; use PacketList; use PacketBuffer; diff --git a/src/ports.rs b/src/ports.rs index e20a9475f..2e209ac55 100644 --- a/src/ports.rs +++ b/src/ports.rs @@ -4,7 +4,7 @@ use coremidi_sys::{ MIDIPortConnectSource, MIDIPortDisconnectSource, MIDIPortDispose }; -use coremidi_sys_ext::{ +use coremidi_sys::{ MIDISend }; From fbfdae76ea28585193a607a6d7107dd36d441c86 Mon Sep 17 00:00:00 2001 From: Patrick Reisert Date: Wed, 19 Jul 2017 17:25:30 +0200 Subject: [PATCH 2/9] Depend on now-published version of sys crate --- Cargo.toml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 350ffe256..e4aa42188 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,6 +14,5 @@ keywords = ["CoreMIDI", "MIDI", "OSX", "music"] [dependencies] core-foundation-sys = "0.2" core-foundation = "0.2" -coremidi-sys = "1.0.0" -# coremidi-sys = { git = "https://github.com/chris-zen/coremidi-sys", branch="fix-packed-structs" } +coremidi-sys = "2.0" time = "0.1" From 45d004457aa13c55cb4ebef49cbcf8ed9fd5f797 Mon Sep 17 00:00:00 2001 From: Patrick Reisert Date: Wed, 19 Jul 2017 17:25:55 +0200 Subject: [PATCH 3/9] Add more packet tests --- src/packets.rs | 76 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 70 insertions(+), 6 deletions(-) diff --git a/src/packets.rs b/src/packets.rs index 9813028ab..afbdc4c88 100644 --- a/src/packets.rs +++ b/src/packets.rs @@ -55,6 +55,7 @@ impl<'a> Packet<'a> { } #[inline] + // TODO: this is wrong, a &MIDIPacket must not exist if the length is smaller than 256 bytes fn packet(&self) -> &MIDIPacket { unsafe { &*self.inner } } @@ -162,9 +163,9 @@ impl<'a> Iterator for PacketListIterator<'a> { } } -const PACKET_LIST_SIZE: usize = 4; // MIDIPacketList::numPackets: UInt32 -const PACKET_SIZE: usize = 8 + // MIDIPacket::timeStamp: MIDITimeStamp/UInt64 - 2; // MIDIPacket::length: UInt16 +const PACKET_LIST_HEADER_SIZE: usize = 4; // MIDIPacketList::numPackets: UInt32 +const PACKET_HEADER_SIZE: usize = 8 + // MIDIPacket::timeStamp: MIDITimeStamp/UInt64 + 2; // MIDIPacket::length: UInt16 /// A mutable `PacketList` builder. /// @@ -181,9 +182,9 @@ impl PacketBuffer { /// Create an empty `PacketBuffer`. /// pub fn new() -> PacketBuffer { - let capacity = PACKET_LIST_SIZE + PACKET_SIZE + 3; + let capacity = PACKET_LIST_HEADER_SIZE + PACKET_HEADER_SIZE + 3; let mut data = Vec::::with_capacity(capacity); - unsafe { data.set_len(PACKET_LIST_SIZE) }; + unsafe { data.set_len(PACKET_LIST_HEADER_SIZE) }; let pkt_list_ptr = data.as_mut_ptr() as *mut MIDIPacketList; let _ = unsafe { MIDIPacketListInit(pkt_list_ptr) }; PacketBuffer { @@ -236,7 +237,7 @@ impl PacketBuffer { "The maximum allowed size for a packet is {}, but found {}.", MAX_PACKET_DATA_LENGTH, data_len); - let additional_size = PACKET_SIZE + data_len; + let additional_size = PACKET_HEADER_SIZE + data_len; self.data.reserve(additional_size); let mut pkt = unsafe { @@ -307,4 +308,67 @@ mod tests { .with_data(0, vec![0x81u8, 0x40, 0x7f]); assert_eq!(packet_buf.length(), 4); } + + #[test] + fn compare_with_native1() { + unsafe { build_packet_list(vec![ + (0, vec![0x90, 0x40, 0x7f]), + (0, vec![0x90, 0x41, 0x7f]), + (0, vec![0x90, 0x42, 0x7f]) + ]) } + } + + #[test] + fn compare_with_native2() { + unsafe { build_packet_list(vec![ + (0, vec![0x90, 0x40, 0x7f]), + (1, vec![0x90, 0x40, 0x7f]), + (2, vec![0x90, 0x40, 0x7f]) + ]) } + } + + #[test] + fn compare_with_native3() { + let mut sysex = vec![0xF0]; + for _ in 0..300 { + sysex.push(0x00); + } + sysex.push(0xF7); + unsafe { build_packet_list(vec![ + (0, vec![0x90, 0x40, 0x7f]), + (0, vec![0x90, 0x41, 0x7f]), + (0, sysex) + ]) } + } + + unsafe fn build_packet_list(packets: Vec<(MIDITimeStamp, Vec)>) { + use coremidi_sys::{MIDIPacketList, MIDIPacketListInit, MIDIPacketListAdd}; + + // allocate a buffer on the stack for building the list using native methods + const BUFFER_SIZE: usize = 65536; // maximum allowed size + let buffer: &mut [u8] = &mut [0; BUFFER_SIZE]; + let pkt_list_ptr = buffer.as_mut_ptr() as *mut MIDIPacketList; + + // build the list + let mut pkt_ptr = MIDIPacketListInit(pkt_list_ptr); + for pkt in &packets { + pkt_ptr = MIDIPacketListAdd(pkt_list_ptr, BUFFER_SIZE as u64, pkt_ptr, pkt.0, pkt.1.len() as u64, pkt.1.as_ptr()); + assert!(!pkt_ptr.is_null()); + } + let list_native = PacketList(pkt_list_ptr); + + // build the PacketBuffer, containing the same packets + /*let mut packet_buf = PacketBuffer::new(); + for pkt in &packets { + packet_buf = packet_buf.with_data(pkt.0, pkt.1.clone()); + } + let list: &PacketList = &packet_buf; + + // check if the contents match + assert_eq!(list_native.length(), list.length()); + // TODO: check if data matches + */ + + assert_eq!(packets.len(), list_native.length()); + } } From 30f40546197df7c8a46e846d1eeedee584db6e43 Mon Sep 17 00:00:00 2001 From: Patrick Reisert Date: Wed, 19 Jul 2017 18:38:04 +0200 Subject: [PATCH 4/9] Change semantics of PacketList type Instead of storing a pointer, use references to PacketType directly, and disallow the creation of PacketTpye values. --- src/client.rs | 4 ++-- src/endpoints/sources.rs | 2 +- src/lib.rs | 16 +++++++++++++++- src/packets.rs | 18 ++++++++---------- src/ports.rs | 2 +- 5 files changed, 27 insertions(+), 15 deletions(-) diff --git a/src/client.rs b/src/client.rs index a65baa603..c84ea9610 100644 --- a/src/client.rs +++ b/src/client.rs @@ -169,8 +169,8 @@ impl Client { _: *mut ::std::os::raw::c_void) { //srcConnRefCon let _ = ::std::panic::catch_unwind(|| unsafe { - let packet_list = PacketList(pktlist); - BoxedCallback::call_from_raw_ptr(read_proc_ref_con, &packet_list); + let packet_list = &*(pktlist as *const PacketList); + BoxedCallback::call_from_raw_ptr(read_proc_ref_con, packet_list); }); } } diff --git a/src/endpoints/sources.rs b/src/endpoints/sources.rs index d3d30ab0b..5b99cd38a 100644 --- a/src/endpoints/sources.rs +++ b/src/endpoints/sources.rs @@ -96,7 +96,7 @@ impl VirtualSource { pub fn received(&self, packet_list: &PacketList) -> Result<(), OSStatus> { let status = unsafe { MIDIReceived( self.endpoint.object.0, - packet_list.0) + packet_list.as_ptr()) }; if status == 0 { Ok(()) } else { Err(status) } } diff --git a/src/lib.rs b/src/lib.rs index e83cebdf9..976952957 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -215,7 +215,21 @@ pub struct Device { object: Object } /// A [list of MIDI events](https://developer.apple.com/reference/coremidi/midipacketlist) being received from, or being sent to, one endpoint. /// #[derive(PartialEq)] -pub struct PacketList(*const MIDIPacketList); +#[repr(C)] +// This type must only exist in the form of references and always point +// to a valid instance of MIDIPacketList. +// `u32` is the fixed-size header (numPackages) that every instance has. +// Everything after that is dynamically sized (but not in the sense of Rust DST). +// This type must NOT implement `Copy`! +pub struct PacketList { _do_not_construct: u32 } + +impl PacketList { + /// For internal usage only. + /// Requires this instance to actually point to a valid MIDIPacketList + unsafe fn as_ptr(&self) -> *mut MIDIPacketList { + self as *const PacketList as *mut PacketList as *mut MIDIPacketList + } +} mod object; mod devices; diff --git a/src/packets.rs b/src/packets.rs index afbdc4c88..01a99b841 100644 --- a/src/packets.rs +++ b/src/packets.rs @@ -109,14 +109,15 @@ impl PacketList { } #[inline] + // TODO: This must be removed fn packet_list(&self) -> &MIDIPacketList { - unsafe { &*self.0 } + unsafe { &*(self.as_ptr() as *const MIDIPacketList) } } } impl fmt::Debug for PacketList { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let result = write!(f, "PacketList(ptr={:x}, packets=[", &self.0 as *const _ as usize); + let result = write!(f, "PacketList(ptr={:x}, packets=[", unsafe { self.as_ptr() as usize }); self.iter().enumerate().fold(result, |prev_result, (i, packet)| { match prev_result { Err(err) => Err(err), @@ -174,8 +175,7 @@ const PACKET_HEADER_SIZE: usize = 8 + // MIDIPacket::timeStamp: MIDITimeSta /// It dereferences to a `PacketList`, so it can be used whenever a `PacketList` is needed. /// pub struct PacketBuffer { - data: Vec, - packet_list: PacketList + data: Vec } impl PacketBuffer { @@ -188,8 +188,7 @@ impl PacketBuffer { let pkt_list_ptr = data.as_mut_ptr() as *mut MIDIPacketList; let _ = unsafe { MIDIPacketListInit(pkt_list_ptr) }; PacketBuffer { - data: data, - packet_list: PacketList(pkt_list_ptr) + data: data } } @@ -252,7 +251,6 @@ impl PacketBuffer { let mut pkt_list = unsafe { &mut *(self.data.as_mut_ptr() as *mut MIDIPacketList) }; pkt_list.numPackets += 1; - self.packet_list = PacketList(pkt_list); self } @@ -262,7 +260,7 @@ impl Deref for PacketBuffer { type Target = PacketList; fn deref(&self) -> &PacketList { - &self.packet_list + unsafe { &*(self.data.as_ptr() as *const PacketList) } } } @@ -296,7 +294,7 @@ mod tests { fn packet_buffer_deref() { let packet_buf = PacketBuffer::new(); let packet_list: &PacketList = &packet_buf; - assert_eq!(packet_list.0, &packet_buf.data[0] as *const _ as *const MIDIPacketList); + assert_eq!(unsafe { packet_list.as_ptr() as *const MIDIPacketList }, &packet_buf.data[0] as *const _ as *const MIDIPacketList); } #[test] @@ -355,7 +353,7 @@ mod tests { pkt_ptr = MIDIPacketListAdd(pkt_list_ptr, BUFFER_SIZE as u64, pkt_ptr, pkt.0, pkt.1.len() as u64, pkt.1.as_ptr()); assert!(!pkt_ptr.is_null()); } - let list_native = PacketList(pkt_list_ptr); + let list_native = &*(pkt_list_ptr as *const _ as *const PacketList); // build the PacketBuffer, containing the same packets /*let mut packet_buf = PacketBuffer::new(); diff --git a/src/ports.rs b/src/ports.rs index 2e209ac55..4466ddda8 100644 --- a/src/ports.rs +++ b/src/ports.rs @@ -41,7 +41,7 @@ impl OutputPort { let status = unsafe { MIDISend( self.port.object.0, destination.endpoint.object.0, - packet_list.0) + packet_list.as_ptr()) }; if status == 0 { Ok(()) } else { Err(status) } } From effc20244b854d91ff7b690e6829bb70002e3879 Mon Sep 17 00:00:00 2001 From: Patrick Reisert Date: Wed, 19 Jul 2017 18:59:20 +0200 Subject: [PATCH 5/9] Change semantics of Packet in the same way as PacketList --- src/packets.rs | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/src/packets.rs b/src/packets.rs index 01a99b841..c479c759e 100644 --- a/src/packets.rs +++ b/src/packets.rs @@ -4,7 +4,6 @@ use coremidi_sys::{ use std::fmt; use std::ops::Deref; -use std::marker::PhantomData; use PacketList; @@ -15,16 +14,25 @@ const MAX_PACKET_DATA_LENGTH: usize = 0xffffusize; /// A collection of simultaneous MIDI events. /// See [MIDIPacket](https://developer.apple.com/reference/coremidi/midipacket). /// -pub struct Packet<'a> { - inner: *const MIDIPacket, - _phantom: PhantomData<&'a MIDIPacket>, +pub struct Packet { + // NOTE: At runtime this type must only be used behind references + // that point to valid instances of MIDIPacket. + inner: PacketInner, + _do_not_construct: [u32; 0] // u32 also guarantees at least 4-byte alignment } -impl<'a> Packet<'a> { +#[repr(packed)] +struct PacketInner { + timestamp: MIDITimeStamp, + length: u16, + data: [u8; 0], // zero-length, because we cannot make this type bigger without knowing how much data there actually is +} + +impl Packet { /// Get the packet timestamp. /// pub fn timestamp(&self) -> Timestamp { - self.packet().timeStamp as Timestamp + self.inner.timestamp as Timestamp } /// Get the packet data. This method just gives raw MIDI bytes. You would need another @@ -49,22 +57,22 @@ impl<'a> Packet<'a> { /// ``` pub fn data(&self) -> &[u8] { let packet = self.packet(); - let data_ptr = &packet.data as *const u8; - let data_len = packet.length as usize; + let data_ptr = packet.inner.data.as_ptr(); + let data_len = packet.inner.length as usize; unsafe { ::std::slice::from_raw_parts(data_ptr, data_len) } } #[inline] // TODO: this is wrong, a &MIDIPacket must not exist if the length is smaller than 256 bytes - fn packet(&self) -> &MIDIPacket { - unsafe { &*self.inner } + fn packet(&self) -> &Packet { + self } } -impl<'a> fmt::Debug for Packet<'a> { +impl fmt::Debug for Packet { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let result = write!(f, "Packet(ptr={:x}, ts={:016x}, data=[", - self.inner as usize, self.timestamp() as u64); + self as *const _ as usize, self.timestamp() as u64); let result = self.data().iter().enumerate().fold(result, |prev_result, (i, b)| { match prev_result { Err(err) => Err(err), @@ -78,7 +86,7 @@ impl<'a> fmt::Debug for Packet<'a> { } } -impl<'a> fmt::Display for Packet<'a> { +impl fmt::Display for Packet { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let result = write!(f, "{:016x}:", self.timestamp()); self.data().iter().fold(result, |prev_result, b| { @@ -145,15 +153,15 @@ impl fmt::Display for PacketList { pub struct PacketListIterator<'a> { count: usize, packet_ptr: *const MIDIPacket, - _phantom: ::std::marker::PhantomData<&'a MIDIPacket>, + _phantom: ::std::marker::PhantomData<&'a Packet>, } impl<'a> Iterator for PacketListIterator<'a> { - type Item = Packet<'a>; + type Item = &'a Packet; - fn next(&mut self) -> Option> { + fn next(&mut self) -> Option<&'a Packet> { if self.count > 0 { - let packet = Packet { inner: self.packet_ptr, _phantom: PhantomData::default() }; + let packet = unsafe { &*(self.packet_ptr as *const Packet) }; self.count -= 1; self.packet_ptr = unsafe { MIDIPacketNext(self.packet_ptr) }; Some(packet) From f554dd34b1b2cde76b248eddc5e327a067ddad75 Mon Sep 17 00:00:00 2001 From: Patrick Reisert Date: Wed, 19 Jul 2017 19:09:47 +0200 Subject: [PATCH 6/9] Simplify access to PacketList fields and validate layout --- src/lib.rs | 29 ++++++++++++++++++++-------- src/packets.rs | 51 +++++++++++++++++++++++++++++--------------------- 2 files changed, 51 insertions(+), 29 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 976952957..ad112407b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -45,7 +45,7 @@ extern crate coremidi_sys; use core_foundation_sys::base::OSStatus; use coremidi_sys::{ - MIDIObjectRef, MIDIFlushOutput, MIDIRestart, MIDIPacketList + MIDIObjectRef, MIDIFlushOutput, MIDIRestart, MIDIPacket, MIDIPacketList }; /// A [MIDI Object](https://developer.apple.com/reference/coremidi/midiobjectref). @@ -212,16 +212,29 @@ pub struct VirtualDestination { #[derive(PartialEq)] pub struct Device { object: Object } +#[cfg(any(target_arch = "arm", target_arch = "aarch64"))] +type AlignmentMarker = [u32; 0]; // ensures 4-byte alignment (on ARM) + + +#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] +type AlignmentMarker = [u8; 0]; // unaligned + /// A [list of MIDI events](https://developer.apple.com/reference/coremidi/midipacketlist) being received from, or being sent to, one endpoint. /// -#[derive(PartialEq)] #[repr(C)] -// This type must only exist in the form of references and always point -// to a valid instance of MIDIPacketList. -// `u32` is the fixed-size header (numPackages) that every instance has. -// Everything after that is dynamically sized (but not in the sense of Rust DST). -// This type must NOT implement `Copy`! -pub struct PacketList { _do_not_construct: u32 } +pub struct PacketList { + // NOTE: This type must only exist in the form of immutable references + // pointing to valid instances of MIDIPacketList. + // This type must NOT implement `Copy`! + inner: PacketListInner, + _do_not_construct: AlignmentMarker +} + +#[repr(packed)] +struct PacketListInner { + num_packets: u32, + data: [MIDIPacket; 0] +} impl PacketList { /// For internal usage only. diff --git a/src/packets.rs b/src/packets.rs index c479c759e..fb7fb5763 100644 --- a/src/packets.rs +++ b/src/packets.rs @@ -6,6 +6,7 @@ use std::fmt; use std::ops::Deref; use PacketList; +use AlignmentMarker; pub type Timestamp = u64; @@ -14,11 +15,14 @@ const MAX_PACKET_DATA_LENGTH: usize = 0xffffusize; /// A collection of simultaneous MIDI events. /// See [MIDIPacket](https://developer.apple.com/reference/coremidi/midipacket). /// +#[repr(C)] pub struct Packet { - // NOTE: At runtime this type must only be used behind references - // that point to valid instances of MIDIPacket. + // NOTE: At runtime this type must only be used behind immutable references + // that point to valid instances of MIDIPacket (mutable references would allow mem::swap). + // This type must NOT implement `Copy`! + // On ARM, this must be 4-byte aligned. inner: PacketInner, - _do_not_construct: [u32; 0] // u32 also guarantees at least 4-byte alignment + _do_not_construct: AlignmentMarker } #[repr(packed)] @@ -56,17 +60,10 @@ impl Packet { /// 90 40 7f /// ``` pub fn data(&self) -> &[u8] { - let packet = self.packet(); - let data_ptr = packet.inner.data.as_ptr(); - let data_len = packet.inner.length as usize; + let data_ptr = self.inner.data.as_ptr(); + let data_len = self.inner.length as usize; unsafe { ::std::slice::from_raw_parts(data_ptr, data_len) } } - - #[inline] - // TODO: this is wrong, a &MIDIPacket must not exist if the length is smaller than 256 bytes - fn packet(&self) -> &Packet { - self - } } impl fmt::Debug for Packet { @@ -103,7 +100,7 @@ impl PacketList { /// Get the number of packets in the list. /// pub fn length(&self) -> usize { - self.packet_list().numPackets as usize + self.inner.num_packets as usize } /// Get an iterator for the packets in the list. @@ -111,16 +108,10 @@ impl PacketList { pub fn iter<'a>(&'a self) -> PacketListIterator<'a> { PacketListIterator { count: self.length(), - packet_ptr: &self.packet_list().packet[0], + packet_ptr: self.inner.data.as_ptr(), _phantom: ::std::marker::PhantomData::default(), } } - - #[inline] - // TODO: This must be removed - fn packet_list(&self) -> &MIDIPacketList { - unsafe { &*(self.as_ptr() as *const MIDIPacketList) } - } } impl fmt::Debug for PacketList { @@ -140,7 +131,7 @@ impl fmt::Debug for PacketList { impl fmt::Display for PacketList { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let result = write!(f, "PacketList(len={})", self.packet_list().numPackets); + let result = write!(f, "PacketList(len={})", self.inner.num_packets); self.iter().fold(result, |prev_result, packet| { match prev_result { Err(err) => Err(err), @@ -274,9 +265,27 @@ impl Deref for PacketBuffer { #[cfg(test)] mod tests { + use std::mem; use coremidi_sys::{MIDITimeStamp, MIDIPacketList}; use PacketList; use PacketBuffer; + use Packet; + use super::{PACKET_HEADER_SIZE, PACKET_LIST_HEADER_SIZE}; + + #[test] + pub fn packet_struct_layout() { + let expected_align = if cfg!(any(target_arch = "arm", target_arch = "aarch64")) { 4 } else { 1 }; + assert_eq!(expected_align, mem::align_of::()); + assert_eq!(expected_align, mem::align_of::()); + + let dummy_packet: Packet = unsafe { mem::zeroed() }; + let ptr = &dummy_packet as *const _ as *const u8; + assert_eq!(PACKET_HEADER_SIZE, dummy_packet.inner.data.as_ptr() as usize - ptr as usize); + + let dummy_packet_list: PacketList = unsafe { mem::zeroed() }; + let ptr = &dummy_packet_list as *const _ as *const u8; + assert_eq!(PACKET_LIST_HEADER_SIZE, dummy_packet_list.inner.data.as_ptr() as usize - ptr as usize); + } #[test] pub fn packet_buffer_new() { From 07983d5f637a88251e9277c4c792280d652df4c8 Mon Sep 17 00:00:00 2001 From: Patrick Reisert Date: Thu, 20 Jul 2017 00:14:03 +0200 Subject: [PATCH 7/9] PacketBuffer rewrite Main improvement is that small packet lists are allocated directly on the stack. When adding packets to a list, it is now also possible for them to be merged with previous packets (same behaviour as MIDIPacketListAdd). --- examples/send.rs | 8 +- examples/virtual-source.rs | 8 +- src/lib.rs | 6 +- src/packets.rs | 381 +++++++++++++++++++++++++++---------- 4 files changed, 291 insertions(+), 112 deletions(-) diff --git a/examples/send.rs b/examples/send.rs index d255d848e..3d4e00204 100644 --- a/examples/send.rs +++ b/examples/send.rs @@ -68,17 +68,17 @@ fn print_destinations() { } fn create_note_on(channel: u8, note: u8, velocity: u8) -> coremidi::PacketBuffer { - let data = vec![ + let data = &[ 0x90 | (channel & 0x0f), note & 0x7f, velocity & 0x7f]; - coremidi::PacketBuffer::from_data(0, data) + coremidi::PacketBuffer::new(0, data) } fn create_note_off(channel: u8, note: u8, velocity: u8) -> coremidi::PacketBuffer { - let data = vec![ + let data = &[ 0x80 | (channel & 0x0f), note & 0x7f, velocity & 0x7f]; - coremidi::PacketBuffer::from_data(0, data) + coremidi::PacketBuffer::new(0, data) } diff --git a/examples/virtual-source.rs b/examples/virtual-source.rs index 07e7c146d..7affe4600 100644 --- a/examples/virtual-source.rs +++ b/examples/virtual-source.rs @@ -22,17 +22,17 @@ fn main() { } fn create_note_on(channel: u8, note: u8, velocity: u8) -> coremidi::PacketBuffer { - let data = vec![ + let data = &[ 0x90 | (channel & 0x0f), note & 0x7f, velocity & 0x7f]; - coremidi::PacketBuffer::from_data(0, data) + coremidi::PacketBuffer::new(0, data) } fn create_note_off(channel: u8, note: u8, velocity: u8) -> coremidi::PacketBuffer { - let data = vec![ + let data = &[ 0x80 | (channel & 0x0f), note & 0x7f, velocity & 0x7f]; - coremidi::PacketBuffer::from_data(0, data) + coremidi::PacketBuffer::new(0, data) } diff --git a/src/lib.rs b/src/lib.rs index ad112407b..77f724965 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -17,8 +17,8 @@ use std::thread; let client = coremidi::Client::new("example-client").unwrap(); let output_port = client.output_port("example-port").unwrap(); let destination = coremidi::Destination::from_index(0).unwrap(); -let note_on = coremidi::PacketBuffer::from_data(0, vec![0x90, 0x40, 0x7f]); -let note_off = coremidi::PacketBuffer::from_data(0, vec![0x80, 0x40, 0x7f]); +let note_on = coremidi::PacketBuffer::new(0, &[0x90, 0x40, 0x7f]); +let note_off = coremidi::PacketBuffer::new(0, &[0x80, 0x40, 0x7f]); output_port.send(&destination, ¬e_on).unwrap(); thread::sleep(Duration::from_millis(1000)); output_port.send(&destination, ¬e_off).unwrap(); @@ -121,7 +121,7 @@ pub struct Port { object: Object } /// let client = coremidi::Client::new("example-client").unwrap(); /// let output_port = client.output_port("example-port").unwrap(); /// let destination = coremidi::Destination::from_index(0).unwrap(); -/// let packets = coremidi::PacketBuffer::from_data(0, vec![0x90, 0x40, 0x7f]); +/// let packets = coremidi::PacketBuffer::new(0, &[0x90, 0x40, 0x7f]); /// output_port.send(&destination, &packets).unwrap(); /// ``` #[derive(Debug)] diff --git a/src/packets.rs b/src/packets.rs index fb7fb5763..412579f7b 100644 --- a/src/packets.rs +++ b/src/packets.rs @@ -1,9 +1,10 @@ use coremidi_sys::{ - MIDITimeStamp, UInt16, MIDIPacketList, MIDIPacket, MIDIPacketListInit, MIDIPacketNext + MIDITimeStamp, MIDIPacket, MIDIPacketNext }; use std::fmt; -use std::ops::Deref; +use std::slice; +use std::ops::{Deref, DerefMut}; use PacketList; use AlignmentMarker; @@ -46,7 +47,7 @@ impl Packet { /// The following example: /// /// ``` - /// let packet_list = &coremidi::PacketBuffer::from_data(0, vec![0x90, 0x40, 0x7f]); + /// let packet_list = &coremidi::PacketBuffer::new(0, &[0x90, 0x40, 0x7f]); /// for packet in packet_list.iter() { /// for byte in packet.data() { /// print!(" {:x}", byte); @@ -62,7 +63,7 @@ impl Packet { pub fn data(&self) -> &[u8] { let data_ptr = self.inner.data.as_ptr(); let data_len = self.inner.length as usize; - unsafe { ::std::slice::from_raw_parts(data_ptr, data_len) } + unsafe { slice::from_raw_parts(data_ptr, data_len) } } } @@ -167,6 +168,106 @@ const PACKET_LIST_HEADER_SIZE: usize = 4; // MIDIPacketList::numPackets: UInt32 const PACKET_HEADER_SIZE: usize = 8 + // MIDIPacket::timeStamp: MIDITimeStamp/UInt64 2; // MIDIPacket::length: UInt16 +const INLINE_PACKET_BUFFER_SIZE: usize = 28; // must be divisible by 4 + +enum PacketBufferStorage { + // using u32 ensures correct alignment (required on ARM) + Inline([u32; INLINE_PACKET_BUFFER_SIZE / 4]), + External(Vec) +} + +impl PacketBufferStorage { + #[inline] + fn get_slice(&self) -> &[u8] { + unsafe { + match *self { + PacketBufferStorage::Inline(ref inline) => + slice::from_raw_parts(inline.as_ptr() as *const u8, inline.len() * 4), + PacketBufferStorage::External(ref vec) => + slice::from_raw_parts(vec.as_ptr() as *const u8, vec.len() * 4) + } + } + } + + #[inline] + fn get_slice_mut(&mut self) -> &mut [u8] { + unsafe { + match *self { + PacketBufferStorage::Inline(ref mut inline) => + slice::from_raw_parts_mut(inline.as_mut_ptr() as *mut u8, inline.len() * 4), + PacketBufferStorage::External(ref mut vec) => + slice::from_raw_parts_mut(vec.as_mut_ptr() as *mut u8, vec.len() * 4) + } + } + } + + unsafe fn assign_packet(&mut self, offset: usize, time: MIDITimeStamp, data: &[u8]) { + assert!(data.len() <= MAX_PACKET_DATA_LENGTH, "packet data too long"); // cannot store longer size in u16 + if cfg!(any(target_arch = "arm", target_arch = "aarch64")) { + debug_assert!(offset & 0b11 == 0); + } + let slice = self.get_slice_mut(); + let mut ptr = slice[offset..].as_mut_ptr() as *mut Packet; + (*ptr).inner.timestamp = time; + (*ptr).inner.length = data.len() as u16; + let packet_data_start = offset + PACKET_HEADER_SIZE; + slice[packet_data_start..(packet_data_start + data.len())].copy_from_slice(data); + } + + /// Requires that there is a valid Packet at `offset`, which has enough space for `data` + unsafe fn extend_packet(&mut self, offset: usize, data: &[u8]) { + let slice = self.get_slice_mut(); + let mut ptr = slice[offset..].as_mut_ptr() as *mut Packet; + let packet_data_start = offset + PACKET_HEADER_SIZE + (*ptr).inner.length as usize; + (*ptr).inner.length += data.len() as u16; + slice[packet_data_start..(packet_data_start + data.len())].copy_from_slice(data); + } + + /// Call this only with larger length values (won't make the buffer smaller) + unsafe fn set_len(&mut self, new_length: usize) { + if new_length < INLINE_PACKET_BUFFER_SIZE { return; } + if new_length < self.get_slice().len() { return; } + let u32_len = ((new_length - 1) / 4) + 1; + let vec: Option> = match *self { + PacketBufferStorage::Inline(ref inline) => { + let mut v = Vec::with_capacity(u32_len); + v.extend_from_slice(inline); + v.set_len(u32_len); + Some(v) + }, + PacketBufferStorage::External(ref mut vec) => { + let current_len = vec.len(); + vec.reserve(u32_len - current_len); + vec.set_len(u32_len); + None + } + }; + + // to prevent borrowcheck errors, this must come after the `match` + if let Some(v) = vec { + *self = PacketBufferStorage::External(v); + } + } +} + +impl Deref for PacketBufferStorage { + type Target = PacketList; + + #[inline] + fn deref(&self) -> &PacketList { + unsafe { &*(self.get_slice().as_ptr() as *const PacketList) } + } +} + +impl DerefMut for PacketBufferStorage { + // NOTE: Mutable references `&mut PacketList` must not be exposed in the public API! + // The user could use mem::swap to modify the header without modifying the packets that follow. + #[inline] + fn deref_mut(&mut self) -> &mut PacketList { + unsafe { &mut *(self.get_slice_mut().as_mut_ptr() as *mut PacketList) } + } +} + /// A mutable `PacketList` builder. /// /// A `PacketList` is an inmmutable reference to a [MIDIPacketList](https://developer.apple.com/reference/coremidi/midipacketlist) structure, @@ -174,23 +275,20 @@ const PACKET_HEADER_SIZE: usize = 8 + // MIDIPacket::timeStamp: MIDITimeSta /// It dereferences to a `PacketList`, so it can be used whenever a `PacketList` is needed. /// pub struct PacketBuffer { - data: Vec + storage: PacketBufferStorage, + last_written_pkt_offset: usize } -impl PacketBuffer { - /// Create an empty `PacketBuffer`. - /// - pub fn new() -> PacketBuffer { - let capacity = PACKET_LIST_HEADER_SIZE + PACKET_HEADER_SIZE + 3; - let mut data = Vec::::with_capacity(capacity); - unsafe { data.set_len(PACKET_LIST_HEADER_SIZE) }; - let pkt_list_ptr = data.as_mut_ptr() as *mut MIDIPacketList; - let _ = unsafe { MIDIPacketListInit(pkt_list_ptr) }; - PacketBuffer { - data: data - } +impl Deref for PacketBuffer { + type Target = PacketList; + + #[inline] + fn deref(&self) -> &PacketList { + self.storage.deref() } +} +impl PacketBuffer { /// Create a `PacketBuffer` with a single packet containing the provided timestamp and data. /// /// According to the official documentation for CoreMIDI, the timestamp represents @@ -200,25 +298,46 @@ impl PacketBuffer { /// Example on how to create a `PacketBuffer` with a single packet for a MIDI note on for C-5: /// /// ``` - /// let note_on = coremidi::PacketBuffer::from_data(0, vec![0x90, 0x3c, 0x7f]); + /// let note_on = coremidi::PacketBuffer::new(0, &[0x90, 0x3c, 0x7f]); /// ``` - #[inline] - pub fn from_data(timestamp: Timestamp, data: Vec) -> PacketBuffer { - Self::new().with_data(timestamp, data) + pub fn new(time: MIDITimeStamp, data: &[u8]) -> PacketBuffer { + let len = data.len() + PACKET_LIST_HEADER_SIZE + PACKET_HEADER_SIZE; + let mut storage = if len <= INLINE_PACKET_BUFFER_SIZE { + PacketBufferStorage::Inline([0; INLINE_PACKET_BUFFER_SIZE / 4]) + } else { + PacketBufferStorage::External( unsafe { + let u32_len = ((len - 1) / 4) + 1; + let mut v = Vec::with_capacity(u32_len); + v.set_len(u32_len); + v + }) + }; + + unsafe { + storage.assign_packet(PACKET_LIST_HEADER_SIZE, time, data); + storage.deref_mut().inner.num_packets = 1; + } + + PacketBuffer { + storage: storage, + last_written_pkt_offset: PACKET_LIST_HEADER_SIZE + } } - + /// Add a new packet containing the provided timestamp and data. /// /// According to the official documentation for CoreMIDI, the timestamp represents /// the time at which the events are to be played, where zero means "now". /// The timestamp applies to the first MIDI byte in the packet. /// + /// A packet must not have a timestamp that is smaller than that of a previous packet + /// in the same `PacketList` + /// /// Example: /// /// ``` - /// let chord = coremidi::PacketBuffer::new() - /// .with_data(0, vec![0x90, 0x3c, 0x7f]) - /// .with_data(0, vec![0x90, 0x40, 0x7f]); + /// let mut chord = coremidi::PacketBuffer::new(0, &[0x90, 0x3c, 0x7f]); + /// chord.push_packet(0, &[0x90, 0x40, 0x7f]); /// println!("{}", &chord as &coremidi::PacketList); /// ``` /// @@ -229,37 +348,57 @@ impl PacketBuffer { /// 0000000000000000: 90 3c 7f /// 0000000000000000: 90 40 7f /// ``` - pub fn with_data(mut self, timestamp: Timestamp, data: Vec) -> Self { - let data_len = data.len(); - assert!(data_len < MAX_PACKET_DATA_LENGTH, - "The maximum allowed size for a packet is {}, but found {}.", - MAX_PACKET_DATA_LENGTH, data_len); - - let additional_size = PACKET_HEADER_SIZE + data_len; - self.data.reserve(additional_size); - - let mut pkt = unsafe { - let total_len = self.data.len(); - self.data.set_len(total_len + additional_size); - &mut *(&self.data[total_len] as *const _ as *mut MIDIPacket) - }; - - pkt.timeStamp = timestamp as MIDITimeStamp; - pkt.length = data_len as UInt16; - pkt.data[0..data_len].clone_from_slice(&data); + pub fn push_packet(&mut self, time: MIDITimeStamp, data: &[u8]) -> &mut Self { + let can_merge; + let previous_data_len; + { + // new scope to please the borrow checker + let previous_packet = self.last_written_packet(); + previous_data_len = previous_packet.data().len(); + can_merge = + previous_packet.timestamp() == time && // timestamps match + data[0] != 0xF0 && // not a sysex + data[0] & 0b10000000 != 0 && // but first byte is a status byte + previous_packet.data()[0] != 0xF0 && // previous packet not a sysex + previous_packet.data()[0] & 0b10000000 != 0 && // but first byte is a status byte + previous_data_len + data.len() < MAX_PACKET_DATA_LENGTH; // enough space left in the packet + } - let mut pkt_list = unsafe { &mut *(self.data.as_mut_ptr() as *mut MIDIPacketList) }; - pkt_list.numPackets += 1; + if can_merge { + // write the data into the previous packet + unsafe { + self.storage.set_len(self.last_written_pkt_offset + PACKET_HEADER_SIZE + previous_data_len + data.len()); + self.storage.extend_packet(self.last_written_pkt_offset, data); + } + } else { + let next_offset = self.get_next_offset(); + unsafe { + self.storage.set_len(next_offset + PACKET_HEADER_SIZE + data.len()); + self.storage.assign_packet(next_offset, time, data); + self.storage.deref_mut().inner.num_packets += 1; + } + self.last_written_pkt_offset = next_offset; + } self } -} - -impl Deref for PacketBuffer { - type Target = PacketList; - fn deref(&self) -> &PacketList { - unsafe { &*(self.data.as_ptr() as *const PacketList) } + #[inline] + fn last_written_packet(&self) -> &Packet { + // NOTE: This requires that there always is at least one packet in the buffer + // (which is okay because we do not provide an empty constructor) + unsafe { &*(self.storage.get_slice()[self.last_written_pkt_offset..].as_ptr() as *const Packet) } + } + + #[inline] + fn get_next_offset(&self) -> usize { + let length = self.last_written_packet().inner.length as usize; + let next_unadjusted = self.last_written_pkt_offset + PACKET_HEADER_SIZE + length; + if cfg!(any(target_arch = "arm", target_arch = "aarch64")) { + (next_unadjusted + 3) & !(3usize) + } else { + next_unadjusted + } } } @@ -270,7 +409,7 @@ mod tests { use PacketList; use PacketBuffer; use Packet; - use super::{PACKET_HEADER_SIZE, PACKET_LIST_HEADER_SIZE}; + use super::{PACKET_HEADER_SIZE, PACKET_LIST_HEADER_SIZE, PacketBufferStorage}; #[test] pub fn packet_struct_layout() { @@ -288,75 +427,110 @@ mod tests { } #[test] - pub fn packet_buffer_new() { - let packet_buf = PacketBuffer::new(); - assert_eq!(packet_buf.data.len(), 4); - assert_eq!(packet_buf.data, vec![0x00, 0x00, 0x00, 0x00]); - } - - #[test] - pub fn packet_buffer_with_data() { - let packet_buf = PacketBuffer::new() - .with_data(0x0102030405060708 as MIDITimeStamp, vec![0x90u8, 0x40, 0x7f]); - assert_eq!(packet_buf.data.len(), 17); - // FIXME This is platform endianess dependent - assert_eq!(packet_buf.data, vec![ - 0x01, 0x00, 0x00, 0x00, - 0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01, - 0x03, 0x00, - 0x90, 0x40, 0x7f]); + pub fn single_packet_alloc_inline() { + let packet_buf = PacketBuffer::new(42, &[0x90u8, 0x40, 0x7f]); + if let PacketBufferStorage::External(_) = packet_buf.storage { + assert!(false, "A single 3-byte message must not be allocated externally") + } } #[test] fn packet_buffer_deref() { - let packet_buf = PacketBuffer::new(); + let packet_buf = PacketBuffer::new(42, &[0x90u8, 0x40, 0x7f]); let packet_list: &PacketList = &packet_buf; - assert_eq!(unsafe { packet_list.as_ptr() as *const MIDIPacketList }, &packet_buf.data[0] as *const _ as *const MIDIPacketList); + assert_eq!(unsafe { packet_list.as_ptr() as *const MIDIPacketList }, packet_buf.storage.get_slice().as_ptr() as *const _ as *const MIDIPacketList); } #[test] fn packet_list_length() { - let packet_buf = PacketBuffer::new() - .with_data(0, vec![0x90u8, 0x40, 0x7f]) - .with_data(0, vec![0x91u8, 0x40, 0x7f]) - .with_data(0, vec![0x80u8, 0x40, 0x7f]) - .with_data(0, vec![0x81u8, 0x40, 0x7f]); + let mut packet_buf = PacketBuffer::new(42, &[0x90u8, 0x40, 0x7f]); + packet_buf.push_packet(43, &[0x91u8, 0x40, 0x7f]); + packet_buf.push_packet(44, &[0x80u8, 0x40, 0x7f]); + packet_buf.push_packet(45, &[0x81u8, 0x40, 0x7f]); assert_eq!(packet_buf.length(), 4); } #[test] - fn compare_with_native1() { - unsafe { build_packet_list(vec![ - (0, vec![0x90, 0x40, 0x7f]), - (0, vec![0x90, 0x41, 0x7f]), - (0, vec![0x90, 0x42, 0x7f]) + fn compare_equal_timestamps() { + // these messages should be merged into a single packet + unsafe { compare_packet_list(vec![ + (42, vec![0x90, 0x40, 0x7f]), + (42, vec![0x90, 0x41, 0x7f]), + (42, vec![0x90, 0x42, 0x7f]) + ]) } + } + + #[test] + fn compare_unequal_timestamps() { + unsafe { compare_packet_list(vec![ + (42, vec![0x90, 0x40, 0x7f]), + (43, vec![0x90, 0x40, 0x7f]), + (44, vec![0x90, 0x40, 0x7f]) + ]) } + } + + #[test] + fn compare_sysex() { + // the sysex must not be merged with the surrounding packets + unsafe { compare_packet_list(vec![ + (42, vec![0x90, 0x40, 0x7f]), + (42, vec![0xF0, 0x01, 0x01, 0x01, 0x01, 0x01, 0xF7]), // sysex + (42, vec![0x90, 0x41, 0x7f]) ]) } } #[test] - fn compare_with_native2() { - unsafe { build_packet_list(vec![ - (0, vec![0x90, 0x40, 0x7f]), - (1, vec![0x90, 0x40, 0x7f]), - (2, vec![0x90, 0x40, 0x7f]) + fn compare_sysex_split() { + // the sysex must not be merged with the surrounding packets + unsafe { compare_packet_list(vec![ + (42, vec![0x90, 0x40, 0x7f]), + (42, vec![0xF0, 0x01, 0x01, 0x01, 0x01]), // sysex part 1 + (42, vec![0x01, 0xF7]), // sysex part 2 + (42, vec![0x90, 0x41, 0x7f]) ]) } } #[test] - fn compare_with_native3() { + fn compare_sysex_split2() { + // the sysex must not be merged with the surrounding packets + unsafe { compare_packet_list(vec![ + (42, vec![0x90, 0x40, 0x7f]), + (42, vec![0xF0, 0x01, 0x01, 0x01, 0x01]), // sysex part 1 + (42, vec![0x01, 0x01, 0x01]), // sysex part 2 + (42, vec![0x01, 0xF7]), // sysex part 3 + (42, vec![0x90, 0x41, 0x7f]) + ]) } + } + + #[test] + fn compare_sysex_malformed() { + // the sysex must not be merged with the surrounding packets + unsafe { compare_packet_list(vec![ + (42, vec![0x90, 0x40, 0x7f]), + (42, vec![0xF0, 0x01, 0x01, 0x01, 0x01]), // sysex part 1 + (42, vec![0x01, 0x01, 0x01]), // sysex part 2 + //(42, vec![0x01, 0xF7]), // sysex part 3 (missing) + (42, vec![0x90, 0x41, 0x7f]) + ]) } + } + + #[test] + fn compare_sysex_long() { let mut sysex = vec![0xF0]; for _ in 0..300 { - sysex.push(0x00); + sysex.push(0x01); } sysex.push(0xF7); - unsafe { build_packet_list(vec![ - (0, vec![0x90, 0x40, 0x7f]), - (0, vec![0x90, 0x41, 0x7f]), - (0, sysex) + unsafe { compare_packet_list(vec![ + (42, vec![0x90, 0x40, 0x7f]), + (43, vec![0x90, 0x41, 0x7f]), + (43, sysex) ]) } } - unsafe fn build_packet_list(packets: Vec<(MIDITimeStamp, Vec)>) { + /// Compares the results of building a PacketList using our PacketBuffer API + /// and the native API (MIDIPacketListAdd, etc). + unsafe fn compare_packet_list(packets: Vec<(MIDITimeStamp, Vec)>) { use coremidi_sys::{MIDIPacketList, MIDIPacketListInit, MIDIPacketListAdd}; // allocate a buffer on the stack for building the list using native methods @@ -373,17 +547,22 @@ mod tests { let list_native = &*(pkt_list_ptr as *const _ as *const PacketList); // build the PacketBuffer, containing the same packets - /*let mut packet_buf = PacketBuffer::new(); - for pkt in &packets { - packet_buf = packet_buf.with_data(pkt.0, pkt.1.clone()); + let mut packet_buf = PacketBuffer::new(packets[0].0, &packets[0].1); + for pkt in &packets[1..] { + packet_buf.push_packet(pkt.0, &pkt.1); } + + // print buffer contents for debugging purposes + let packet_buf_slice = packet_buf.storage.get_slice(); + println!("\nbuffer: {:?}", packet_buf_slice); + println!("\nnative: {:?}", &buffer[0..packet_buf_slice.len()]); + let list: &PacketList = &packet_buf; // check if the contents match - assert_eq!(list_native.length(), list.length()); - // TODO: check if data matches - */ - - assert_eq!(packets.len(), list_native.length()); + assert_eq!(list_native.length(), list.length(), "PacketList lenghts must match"); + for (n, p) in list_native.iter().zip(list.iter()) { + assert_eq!(n.data(), p.data()); + } } } From f72086adb66675b1373fbc14c19b50aae6bcf3e4 Mon Sep 17 00:00:00 2001 From: Patrick Reisert Date: Fri, 21 Jul 2017 11:17:22 +0200 Subject: [PATCH 8/9] Rename PacketList::length -> len This is consistent with the naming in Rust's standard library. --- src/packets.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/packets.rs b/src/packets.rs index 412579f7b..684ae9878 100644 --- a/src/packets.rs +++ b/src/packets.rs @@ -100,7 +100,7 @@ impl PacketList { /// Get the number of packets in the list. /// - pub fn length(&self) -> usize { + pub fn len(&self) -> usize { self.inner.num_packets as usize } @@ -108,7 +108,7 @@ impl PacketList { /// pub fn iter<'a>(&'a self) -> PacketListIterator<'a> { PacketListIterator { - count: self.length(), + count: self.len(), packet_ptr: self.inner.data.as_ptr(), _phantom: ::std::marker::PhantomData::default(), } @@ -447,7 +447,7 @@ mod tests { packet_buf.push_packet(43, &[0x91u8, 0x40, 0x7f]); packet_buf.push_packet(44, &[0x80u8, 0x40, 0x7f]); packet_buf.push_packet(45, &[0x81u8, 0x40, 0x7f]); - assert_eq!(packet_buf.length(), 4); + assert_eq!(packet_buf.len(), 4); } #[test] @@ -560,7 +560,7 @@ mod tests { let list: &PacketList = &packet_buf; // check if the contents match - assert_eq!(list_native.length(), list.length(), "PacketList lenghts must match"); + assert_eq!(list_native.len(), list.len(), "PacketList lengths must match"); for (n, p) in list_native.iter().zip(list.iter()) { assert_eq!(n.data(), p.data()); } From 2e6b4425478f4ce890b49c06c7606d1f57a65927 Mon Sep 17 00:00:00 2001 From: Patrick Reisert Date: Mon, 4 Sep 2017 17:18:22 +0200 Subject: [PATCH 9/9] Addressed review comments & cleanup --- README.md | 4 +-- src/client.rs | 5 +--- src/lib.rs | 9 +----- src/packets.rs | 75 +++++++++++++++++++++++++++++++------------------- src/ports.rs | 6 +--- 5 files changed, 51 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index ba73b0707..3770348f5 100644 --- a/README.md +++ b/README.md @@ -17,8 +17,8 @@ use std::thread; let client = Client::new("example-client").unwrap(); let output_port = client.output_port("example-port").unwrap(); let destination = Destinations::from_index(0).unwrap(); -let note_on = PacketBuffer::from_data(0, vec![0x90, 0x40, 0x7f]); -let note_off = PacketBuffer::from_data(0, vec![0x80, 0x40, 0x7f]); +let note_on = PacketBuffer::new(0, &[0x90, 0x40, 0x7f]); +let note_off = PacketBuffer::new(0, &[0x80, 0x40, 0x7f]); output_port.send(&destination, ¬e_on).unwrap(); thread::sleep(Duration::from_millis(1000)); output_port.send(&destination, ¬e_off).unwrap(); diff --git a/src/client.rs b/src/client.rs index c84ea9610..6f3cd504d 100644 --- a/src/client.rs +++ b/src/client.rs @@ -3,10 +3,7 @@ use core_foundation::base::{OSStatus, TCFType}; use coremidi_sys::{ MIDIClientRef, MIDIClientCreate, MIDIClientDispose, MIDINotification, - MIDIPortRef, MIDIOutputPortCreate, MIDIEndpointRef, MIDISourceCreate -}; - -use coremidi_sys::{ + MIDIPortRef, MIDIOutputPortCreate, MIDIEndpointRef, MIDISourceCreate, MIDIPacketList, MIDIInputPortCreate, MIDIDestinationCreate }; diff --git a/src/lib.rs b/src/lib.rs index 77f724965..3995c3394 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -212,13 +212,6 @@ pub struct VirtualDestination { #[derive(PartialEq)] pub struct Device { object: Object } -#[cfg(any(target_arch = "arm", target_arch = "aarch64"))] -type AlignmentMarker = [u32; 0]; // ensures 4-byte alignment (on ARM) - - -#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] -type AlignmentMarker = [u8; 0]; // unaligned - /// A [list of MIDI events](https://developer.apple.com/reference/coremidi/midipacketlist) being received from, or being sent to, one endpoint. /// #[repr(C)] @@ -227,7 +220,7 @@ pub struct PacketList { // pointing to valid instances of MIDIPacketList. // This type must NOT implement `Copy`! inner: PacketListInner, - _do_not_construct: AlignmentMarker + _do_not_construct: packets::alignment::Marker } #[repr(packed)] diff --git a/src/packets.rs b/src/packets.rs index 684ae9878..c58163bf7 100644 --- a/src/packets.rs +++ b/src/packets.rs @@ -7,12 +7,23 @@ use std::slice; use std::ops::{Deref, DerefMut}; use PacketList; -use AlignmentMarker; pub type Timestamp = u64; const MAX_PACKET_DATA_LENGTH: usize = 0xffffusize; +#[cfg(any(target_arch = "arm", target_arch = "aarch64"))] +pub mod alignment { + pub type Marker = [u32; 0]; // ensures 4-byte alignment (on ARM) + pub const NEEDS_ALIGNMENT: bool = true; +} + +#[cfg(not(any(target_arch = "arm", target_arch = "aarch64")))] +pub mod alignment { + pub type Marker = [u8; 0]; // unaligned + pub const NEEDS_ALIGNMENT: bool = false; +} + /// A collection of simultaneous MIDI events. /// See [MIDIPacket](https://developer.apple.com/reference/coremidi/midipacket). /// @@ -23,7 +34,7 @@ pub struct Packet { // This type must NOT implement `Copy`! // On ARM, this must be 4-byte aligned. inner: PacketInner, - _do_not_construct: AlignmentMarker + _alignment_marker: alignment::Marker } #[repr(packed)] @@ -171,8 +182,11 @@ const PACKET_HEADER_SIZE: usize = 8 + // MIDIPacket::timeStamp: MIDITimeSta const INLINE_PACKET_BUFFER_SIZE: usize = 28; // must be divisible by 4 enum PacketBufferStorage { - // using u32 ensures correct alignment (required on ARM) + /// Inline stores the data directy on the stack, if it is small enough. + /// NOTE: using u32 ensures correct alignment (required on ARM) Inline([u32; INLINE_PACKET_BUFFER_SIZE / 4]), + /// External is used whenever the size of the data exceeds INLINE_PACKET_BUFFER_SIZE. + /// This means that the size of the contained vector is always greater than INLINE_PACKET_BUFFER_SIZE. External(Vec) } @@ -203,7 +217,7 @@ impl PacketBufferStorage { unsafe fn assign_packet(&mut self, offset: usize, time: MIDITimeStamp, data: &[u8]) { assert!(data.len() <= MAX_PACKET_DATA_LENGTH, "packet data too long"); // cannot store longer size in u16 - if cfg!(any(target_arch = "arm", target_arch = "aarch64")) { + if alignment::NEEDS_ALIGNMENT { debug_assert!(offset & 0b11 == 0); } let slice = self.get_slice_mut(); @@ -324,20 +338,20 @@ impl PacketBuffer { } } - /// Add a new packet containing the provided timestamp and data. + /// Add a new event containing the provided timestamp and data. /// /// According to the official documentation for CoreMIDI, the timestamp represents /// the time at which the events are to be played, where zero means "now". /// The timestamp applies to the first MIDI byte in the packet. /// - /// A packet must not have a timestamp that is smaller than that of a previous packet + /// An event must not have a timestamp that is smaller than that of a previous event /// in the same `PacketList` /// /// Example: /// /// ``` /// let mut chord = coremidi::PacketBuffer::new(0, &[0x90, 0x3c, 0x7f]); - /// chord.push_packet(0, &[0x90, 0x40, 0x7f]); + /// chord.push_data(0, &[0x90, 0x40, 0x7f]); /// println!("{}", &chord as &coremidi::PacketList); /// ``` /// @@ -348,21 +362,8 @@ impl PacketBuffer { /// 0000000000000000: 90 3c 7f /// 0000000000000000: 90 40 7f /// ``` - pub fn push_packet(&mut self, time: MIDITimeStamp, data: &[u8]) -> &mut Self { - let can_merge; - let previous_data_len; - { - // new scope to please the borrow checker - let previous_packet = self.last_written_packet(); - previous_data_len = previous_packet.data().len(); - can_merge = - previous_packet.timestamp() == time && // timestamps match - data[0] != 0xF0 && // not a sysex - data[0] & 0b10000000 != 0 && // but first byte is a status byte - previous_packet.data()[0] != 0xF0 && // previous packet not a sysex - previous_packet.data()[0] & 0b10000000 != 0 && // but first byte is a status byte - previous_data_len + data.len() < MAX_PACKET_DATA_LENGTH; // enough space left in the packet - } + pub fn push_data(&mut self, time: MIDITimeStamp, data: &[u8]) -> &mut Self { + let (can_merge, previous_data_len) = self.can_merge_into_previous(time, data); if can_merge { // write the data into the previous packet @@ -383,18 +384,34 @@ impl PacketBuffer { self } + /// Checks whether the given tiemstamped data can be merged into the previous packet + fn can_merge_into_previous(&self, time: MIDITimeStamp, data: &[u8]) -> (bool, usize) { + let previous_packet = self.last_written_packet(); + let previous_data_len = previous_packet.data().len(); + let can_merge = + previous_packet.timestamp() == time && // timestamps match + data[0] != 0xF0 && // not a sysex + data[0] & 0b10000000 != 0 && // but first byte is a status byte + previous_packet.data()[0] != 0xF0 && // previous packet not a sysex + previous_packet.data()[0] & 0b10000000 != 0 && // but first byte is a status byte + previous_data_len + data.len() < MAX_PACKET_DATA_LENGTH; // enough space left in the packet + (can_merge, previous_data_len) + } + #[inline] fn last_written_packet(&self) -> &Packet { // NOTE: This requires that there always is at least one packet in the buffer // (which is okay because we do not provide an empty constructor) - unsafe { &*(self.storage.get_slice()[self.last_written_pkt_offset..].as_ptr() as *const Packet) } + let packets_slice = self.storage.get_slice(); + let packet_slot = &packets_slice[self.last_written_pkt_offset..]; + unsafe { &*(packet_slot.as_ptr() as *const Packet) } } #[inline] fn get_next_offset(&self) -> usize { let length = self.last_written_packet().inner.length as usize; let next_unadjusted = self.last_written_pkt_offset + PACKET_HEADER_SIZE + length; - if cfg!(any(target_arch = "arm", target_arch = "aarch64")) { + if alignment::NEEDS_ALIGNMENT { (next_unadjusted + 3) & !(3usize) } else { next_unadjusted @@ -413,7 +430,7 @@ mod tests { #[test] pub fn packet_struct_layout() { - let expected_align = if cfg!(any(target_arch = "arm", target_arch = "aarch64")) { 4 } else { 1 }; + let expected_align = if super::alignment::NEEDS_ALIGNMENT { 4 } else { 1 }; assert_eq!(expected_align, mem::align_of::()); assert_eq!(expected_align, mem::align_of::()); @@ -444,9 +461,9 @@ mod tests { #[test] fn packet_list_length() { let mut packet_buf = PacketBuffer::new(42, &[0x90u8, 0x40, 0x7f]); - packet_buf.push_packet(43, &[0x91u8, 0x40, 0x7f]); - packet_buf.push_packet(44, &[0x80u8, 0x40, 0x7f]); - packet_buf.push_packet(45, &[0x81u8, 0x40, 0x7f]); + packet_buf.push_data(43, &[0x91u8, 0x40, 0x7f]); + packet_buf.push_data(44, &[0x80u8, 0x40, 0x7f]); + packet_buf.push_data(45, &[0x81u8, 0x40, 0x7f]); assert_eq!(packet_buf.len(), 4); } @@ -549,7 +566,7 @@ mod tests { // build the PacketBuffer, containing the same packets let mut packet_buf = PacketBuffer::new(packets[0].0, &packets[0].1); for pkt in &packets[1..] { - packet_buf.push_packet(pkt.0, &pkt.1); + packet_buf.push_data(pkt.0, &pkt.1); } // print buffer contents for debugging purposes diff --git a/src/ports.rs b/src/ports.rs index 4466ddda8..1961fb36d 100644 --- a/src/ports.rs +++ b/src/ports.rs @@ -1,11 +1,7 @@ use core_foundation::base::OSStatus; use coremidi_sys::{ - MIDIPortConnectSource, MIDIPortDisconnectSource, MIDIPortDispose -}; - -use coremidi_sys::{ - MIDISend + MIDIPortConnectSource, MIDIPortDisconnectSource, MIDIPortDispose, MIDISend }; use std::ptr;