Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ndk: Add AMidi interface #353

Open
wants to merge 56 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
2daf09c
ndk: Add AMidi interface
paxbun Oct 4, 2022
9456589
ndk: Remove needless explicit lifetimes in midi.rs
paxbun Oct 4, 2022
9cf8581
ndk: Fix mismatching doc comments in midi.rs
paxbun Oct 4, 2022
9f7c9ef
ndk: Remove get_ prefix from midi.rs
paxbun Oct 4, 2022
466bf4a
ndk: Fix doc comments of MidiOutputPort::receive
paxbun Oct 4, 2022
4013f68
ndk-sys: Add feature `midi`
paxbun Oct 5, 2022
ff6697c
ndk: Add feature `midi`
paxbun Oct 5, 2022
02630b0
ndk: Make Midi types implement Send
paxbun Oct 5, 2022
d3d616e
ndk: Update ndk/src/midi.rs
paxbun Oct 5, 2022
4f53506
ndk: Fix mismatching doc comments in midi.rs
paxbun Oct 5, 2022
15b917f
ndk: Fix link to ndk-samples with a permalink
paxbun Oct 5, 2022
5ae1cad
ndk: Replace try_from_primitives in midi.rs with try_from
paxbun Oct 5, 2022
06ebda5
ndk: Make MidiOutputPort::receive return timestamp only with MidiOpco…
paxbun Oct 5, 2022
2c92479
ndk: Add ptr exposing functions to wrappers in midi.rs
paxbun Oct 5, 2022
b2ec4ea
ndk: Raise error when 2 or more msgs received in MidiOutputPort
paxbun Oct 5, 2022
6f262cc
ndk: Remove as_ptr() from structs in midi.rs
paxbun Oct 5, 2022
01877bc
ndk: Add more strict opcode checks to MidiOutputPort
paxbun Oct 5, 2022
48b4416
ndk: Make msg for unexpected opcode in midi.rs more clear
paxbun Oct 5, 2022
aa26247
ndk: Fix vague doc comments in midi.rs
paxbun Oct 5, 2022
8235a5c
ndk: Add mod media_error
paxbun Oct 5, 2022
12df844
ndk: Remove pub use for MediaErrorResult from media/mod.rs
paxbun Oct 5, 2022
34357c7
ndk: Remove pub use from media/mod.rs
paxbun Oct 5, 2022
23b18b0
ndk: Reflect midi.rs changes to CHANGELOG.md
paxbun Oct 5, 2022
c79c8c5
fix: Make MidiDevice::from_java unsafe
paxbun Jun 15, 2023
895350b
Merge branch 'master' into feat/amidi
paxbun Jun 16, 2023
acce571
fix(ndk): Remove all occurrences of ffi::size_t from midi.rs
paxbun Jun 16, 2023
8900c1c
fix(ndk): Remove unnecessary casts from midi.rs
paxbun Jun 16, 2023
95220e7
ndk: Rework media error types
MarijnS95 Jun 15, 2023
85288eb
Merge branch 'ndk-rework-media-error' into feat/amidi
paxbun Jun 16, 2023
cc1c2e3
fix(ndk): Reflect changes in media_error to midi
paxbun Jun 16, 2023
200ab56
Merge branch 'master' into feat/amidi
paxbun Jun 23, 2023
408e460
Update comments in ndk/src/midi.rs
paxbun Aug 10, 2023
9a5744b
Update ndk/src/midi.rs
paxbun Aug 10, 2023
412b6ad
Update ndk/src/midi.rs
paxbun Aug 10, 2023
aa4467e
Update ndk/CHANGELOG.md
paxbun Aug 10, 2023
3cfaad4
fix: Remove impl Send for MidiDevice
paxbun Aug 10, 2023
2e036d2
Fix comments in ndk/src/media_error.rs
paxbun Aug 10, 2023
aa79a19
ndk: Make "midi" feature dependent on "media"
paxbun Aug 10, 2023
0341b4d
fix: Remove redundant Result mapping from midi.rs
paxbun Aug 10, 2023
c8b2b2e
ndk: Add MidiDeviceType::Unknown
paxbun Aug 10, 2023
6044fc2
ndk: Add doc-comments to MidiInputPort
paxbun Aug 16, 2023
f22054d
ndk: Add doc-comments to fields of MidiOpcode::Data
paxbun Aug 16, 2023
734b81b
ndk: Reference the Java Android MIDI docs in the doc-comments
paxbun Aug 16, 2023
7220a9a
ndk: Fix doc-comments of MidiOutputPort::receive
paxbun Aug 16, 2023
241b096
ndk: Drop Send for ndk::midi::Midi*Port
paxbun Aug 17, 2023
d9ef310
ndk: Describe about Java VM thread attachment in the safety section o…
paxbun Aug 17, 2023
31159dc
ndk: Add safe wrapper of midi
paxbun Aug 17, 2023
129edc2
ndk: Fix typos in ndk::midi::safe
paxbun Aug 17, 2023
1ea5b32
Merge branch 'master' into feat/amidi
paxbun Aug 17, 2023
83fe4b2
ndk: Fix clippy warnings in ndk::midi
paxbun Aug 17, 2023
8c64160
ndk: Make examples in ndk::midi compilable
paxbun Aug 17, 2023
5428ca7
ndk: Remove link to SafeMidiDeviceBox from the module-level doc-comme…
paxbun Aug 17, 2023
802ab4a
ndk: Add more detailed description about safety of AMidi functions to…
paxbun Aug 17, 2023
300118e
ndk: Add missing brackets to links to functions in doc-comments in nd…
paxbun Aug 17, 2023
8086a7d
ndk: Enable media_error when "midi" is enabled
paxbun Aug 17, 2023
5707555
Merge remote-tracking branch 'origin/master' into feat/amidi
paxbun Dec 14, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ndk-sys/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ test = []
audio = []
bitmap = []
media = []
midi = []

[package.metadata.docs.rs]
rustdoc-args = ["--cfg", "docsrs"]
Expand Down
4 changes: 4 additions & 0 deletions ndk-sys/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,7 @@ extern "C" {}
#[cfg(all(feature = "audio", target_os = "android"))]
#[link(name = "aaudio")]
extern "C" {}

#[cfg(all(feature = "midi", target_os = "android"))]
#[link(name = "amidi")]
paxbun marked this conversation as resolved.
Show resolved Hide resolved
extern "C" {}
paxbun marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions ndk/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ all = ["audio", "bitmap","media", "api-level-30"]
audio = ["ffi/audio", "api-level-26"]
bitmap = ["ffi/bitmap"]
media = ["ffi/media"]
midi = ["ffi/midi", "api-level-29"]
MarijnS95 marked this conversation as resolved.
Show resolved Hide resolved

api-level-23 = []
api-level-24 = ["api-level-23"]
Expand Down
1 change: 1 addition & 0 deletions ndk/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub mod hardware_buffer_format;
pub mod input_queue;
pub mod looper;
pub mod media;
pub mod midi;
pub mod native_activity;
pub mod native_window;
pub mod surface_texture;
Expand Down
14 changes: 11 additions & 3 deletions ndk/src/media/mod.rs
Original file line number Diff line number Diff line change
@@ -1,23 +1,30 @@
//! Bindings for the NDK media classes.
//!
//! See also [the NDK docs](https://developer.android.com/ndk/reference/group/media)
#![cfg(feature = "media")]
#![cfg(any(feature = "media", feature = "midi"))]

mod error;
MarijnS95 marked this conversation as resolved.
Show resolved Hide resolved

#[cfg(feature = "media")]
pub mod image_reader;
#[cfg(feature = "media")]
pub mod media_codec;

pub use error::NdkMediaError;
use std::{mem::MaybeUninit, ptr::NonNull};
use std::mem::MaybeUninit;

#[cfg(feature = "media")]
use std::ptr::NonNull;

pub type Result<T, E = NdkMediaError> = std::result::Result<T, E>;

fn construct<T>(with_ptr: impl FnOnce(*mut T) -> ffi::media_status_t) -> Result<T> {
pub(crate) fn construct<T>(with_ptr: impl FnOnce(*mut T) -> ffi::media_status_t) -> Result<T> {
let mut result = MaybeUninit::uninit();
let status = with_ptr(result.as_mut_ptr());
NdkMediaError::from_status(status).map(|()| unsafe { result.assume_init() })
}

#[cfg(feature = "media")]
fn construct_never_null<T>(
with_ptr: impl FnOnce(*mut *mut T) -> ffi::media_status_t,
) -> Result<NonNull<T>> {
Expand All @@ -35,6 +42,7 @@ fn construct_never_null<T>(
///
/// As such this function always asserts on `null` values,
/// even when `cfg!(debug_assertions)` is disabled.
#[cfg(feature = "media")]
fn get_unlikely_to_be_null<T>(get_ptr: impl FnOnce() -> *mut T) -> NonNull<T> {
let result = get_ptr();
NonNull::new(result).expect("result should never be null")
Expand Down
305 changes: 305 additions & 0 deletions ndk/src/midi.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,305 @@
//! Bindings for [`AMidiDevice`], [`AMidiInputPort`], and [`AMidiOutputPort`]
//!
//! See [the NDK guide](https://developer.android.com/ndk/guides/audio/midi) for
//! design and usage instructions, and [the NDK reference](https://developer.android.com/ndk/reference/group/midi)
//! for an API overview.
//!
//! [`AMidiDevice`]: https://developer.android.com/ndk/reference/group/midi#amididevice
//! [`AMidiInputPort`]: https://developer.android.com/ndk/reference/group/midi#amidiinputport
//! [`AMidiOutputPort`]: https://developer.android.com/ndk/reference/group/midi#amidioutputport
#![cfg(feature = "midi")]

pub use super::media::Result;
use super::media::{construct, NdkMediaError};

use num_enum::{IntoPrimitive, TryFromPrimitive};
use std::convert::TryFrom;
use std::fmt;
use std::marker::PhantomData;
use std::os::raw::{c_int, c_uint};
use std::ptr::NonNull;

// There is no mention about thread-safety in the NDK reference, but the official Android C++ MIDI
// sample stores `AMidiDevice *` and `AMidi{Input,Output}Port *` in global variables and accesses the
// ports from separate threads.
// See https://github.com/android/ndk-samples/blob/7f6936ea044ee29c36b5c3ebd62bb3a64e1e6014/native-midi/app/src/main/cpp/AppMidiManager.cpp
unsafe impl Send for MidiDevice {}
paxbun marked this conversation as resolved.
Show resolved Hide resolved
unsafe impl<'a> Send for MidiInputPort<'a> {}
unsafe impl<'a> Send for MidiOutputPort<'a> {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can they be accessed concurrently (i.e., Sync), or are these only safe to be used on different threads as long as there aren't two or more threads poking it at the same time (Send)?

Copy link
Contributor Author

@paxbun paxbun Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MidiIntputPort is definitely not Sync: it puts write in the loop, so there's a possibility of interleaved writing.
image

MidiOutputPort is definitely Sync:MidiReceiver, which is called by AMidiOutputPort_receive implements synchronization logic. MidiOutputPort::receive will immediately return an Err if a simultaneous read call was invoked by another thread.
image

MidiDevice seems to be Sync: as MidiDeviceServer, which is called by MidiDevice, uses synchronized in every method except for getDeviceInfo() and setDeviceInfo(), but I'm not sure.
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting design choice and discrepancy between input and output ports. We can also enforce the write with a &mut self borrow but I think keeping them as Send without Clone/Copy is also fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-read the source code of MidiDevice and MidiDeviceServer - I think we can assume that MidiDevice is Sync as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • AMidiDevice_fromJava, AMidiDevice_release:
    Synchronized by openMutex defined in Line 102 of amidi.cpp
  • AMidiDevice_get*:
    Retrieves values from AMidiDevice::deviceInfo, which is not changed after instantiation.
  • AMidi{Input, Output}Port_open -> AMIDI_openPort -> BpMidiDeviceServer::open{Input, Output}Port:
    Calls Java-side MidiDeviceServer via transact() and onTransact():
    1. gFidMidiDeviceServerBinder, which represents IBinder MidiDevice.mDeviceServerBinder, is set from android_media_midi.cpp
    2. MidiDevice.mDeviceServerBinder is set in the constructor of MidiDevice:
    /* package */ MidiDevice(MidiDeviceInfo deviceInfo, IMidiDeviceServer server,
            IMidiManager midiManager, IBinder clientToken, IBinder deviceToken) {
        mDeviceInfo = deviceInfo;
        mDeviceServer = server;
        mDeviceServerBinder = mDeviceServer.asBinder();
        ...
    1. ...and the Java-side MidiDeviceServer.open{Input, Output}Port all have synchronized block on mInputPortOutputPorts and mOutputPortDispatchers[portNumber], which means MidiDevice is thread safe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not preemptively add Send/Sync in that case. If it's not documented there's nothing holding the NDK folks from changing the underling implementation and comments.

We recently requested clarification on other APIs as well, perhaps you can do the same here?

TLDR: Let's keep it Send for now until we have official confirmation for Sync.

Copy link
Contributor Author

@paxbun paxbun Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that AMidiDevice must be released in the thread where the JNI is attached, i.e., the app crashes when AMidiDevice_release is called before AttachCurrentThread[AsDaemon] is called from that thread, which is not a documented behavior. I'll remove Send from MidiDevice.

let midi_device = MidiDevice::from_java(...);
thread::spawn(move || {
  midi_device. ...
  // JNIEnv access error on AMidiDevice_release
});
let midi_device = MidiDevice::from_java(...);
thread::spawn(move || {
  AttachCurrentThread(vm, &env, ...);
  midi_device. ...
  // No error
});

+++

We could add SafeMidiDevice type like the following, but I think this must be implemented by the user, not provided from ndk::midi.

struct SafeMidiDevice {
  device: MidiDevice,
  vm: NonNull<JavaVM>,
}

impl SafeMidiDevice {
  pub fn new(vm: NonNull<JavaVM>) -> Self { ... }
}

impl Into<MidiDevice> for SafeMidiDevice {
  fn into(self) -> MidiDevice {
    AttachCurrentThread(self.vm, ...);
    self.device
  }
}

unsafe impl Send for SafeMidiDevice {}

Copy link
Contributor Author

@paxbun paxbun Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that calling AMidiInputPort_close or AMidiOutputPort_close in non-Java thread makes no issue now, but it's not precisely documented as well, and they even have not appeared in the official example. (The official example only calls AMidiDevice_release.) Maybe we should remove impl Send from MidiInputPort and MidiOutputPort as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should remove it indeed, though having a real-world user/consumer of these API bindings would really help us test and flesh out these things besides just guessing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you resolved this. No clue about removing impl Send from the ports, the NDK makes no guarantees but it seems logical that you might want to process such data on another thread even if the JNI device can only be accessed on a VM-attached thread?


/// Result of [`MidiOutputPort::receive`].
paxbun marked this conversation as resolved.
Show resolved Hide resolved
#[derive(Copy, Clone, Debug)]
pub enum MidiOpcode {
/// No MIDI messages are available.
NoMessage,
/// Received a MIDI message with the given length and the timestamp.
Data { length: usize, timestamp: i64 },
/// Instructed to discard all pending MIDI data.
Flush,
}

#[repr(u32)]
#[derive(Copy, Clone, Debug, PartialEq, Eq, TryFromPrimitive, IntoPrimitive)]
pub enum MidiDeviceType {
Bluetooth = ffi::AMIDI_DEVICE_TYPE_BLUETOOTH,
USB = ffi::AMIDI_DEVICE_TYPE_USB,
Virtual = ffi::AMIDI_DEVICE_TYPE_VIRTUAL,
}

#[derive(Debug)]
pub struct MidiDevice {
ptr: NonNull<ffi::AMidiDevice>,
}

impl MidiDevice {
/// Assumes ownership of `ptr`
///
/// # Safety
/// `ptr` must be a valid pointer to an Android [`ffi::AMidiDevice`].
pub unsafe fn from_ptr(ptr: NonNull<ffi::AMidiDevice>) -> Self {
Self { ptr }
}

pub fn ptr(&self) -> NonNull<ffi::AMidiDevice> {
self.ptr
}

fn as_ptr(&self) -> *mut ffi::AMidiDevice {
self.ptr.as_ptr()
}

/// Connects a native Midi Device object to the associated Java MidiDevice object.
///
/// Use the returned [`MidiDevice`] to access the rest of the native MIDI API.
pub fn from_java(
env: *mut jni_sys::JNIEnv,
midi_device_obj: jni_sys::jobject,
) -> Result<MidiDevice> {
unsafe {
let ptr = construct(|res| ffi::AMidiDevice_fromJava(env, midi_device_obj, res))?;
Ok(Self::from_ptr(NonNull::new_unchecked(ptr)))
}
}

/// Gets the number of input (sending) ports available on the specified MIDI device.
pub fn num_input_ports(&self) -> Result<usize> {
let num_input_ports = unsafe { ffi::AMidiDevice_getNumInputPorts(self.as_ptr()) };
if num_input_ports >= 0 {
Ok(num_input_ports as usize)
} else {
NdkMediaError::from_status(ffi::media_status_t(num_input_ports as c_int)).map(|_| 0)
}
}

/// Gets the number of output (receiving) ports available on the specified MIDI device.
pub fn num_output_ports(&self) -> Result<usize> {
let num_output_ports = unsafe { ffi::AMidiDevice_getNumOutputPorts(self.as_ptr()) };
if num_output_ports >= 0 {
Ok(num_output_ports as usize)
} else {
Err(
NdkMediaError::from_status(ffi::media_status_t(num_output_ports as c_int))
.unwrap_err(),
)
}
}

/// Gets the MIDI device type.
pub fn device_type(&self) -> Result<MidiDeviceType> {
let device_type = unsafe { ffi::AMidiDevice_getType(self.as_ptr()) };
if device_type >= 0 {
let device_type = MidiDeviceType::try_from(device_type as u32).map_err(|e| {
NdkMediaError::UnknownResult(ffi::media_status_t(e.number as c_int))
})?;
Ok(device_type)
} else {
Err(NdkMediaError::from_status(ffi::media_status_t(device_type)).unwrap_err())
}
}

/// Opens the input port so that the client can send data to it.
pub fn open_input_port(&self, port_number: i32) -> Result<MidiInputPort> {
unsafe {
let input_port =
construct(|res| ffi::AMidiInputPort_open(self.as_ptr(), port_number, res))?;
Ok(MidiInputPort::from_ptr(NonNull::new_unchecked(input_port)))
}
}

/// Opens the output port so that the client can receive data from it.
pub fn open_output_port(&self, port_number: i32) -> Result<MidiOutputPort> {
unsafe {
let output_port =
construct(|res| ffi::AMidiOutputPort_open(self.as_ptr(), port_number, res))?;
Ok(MidiOutputPort::from_ptr(NonNull::new_unchecked(
output_port,
)))
}
}
}

impl Drop for MidiDevice {
fn drop(&mut self) {
let status = unsafe { ffi::AMidiDevice_release(self.as_ptr()) };
NdkMediaError::from_status(status).unwrap();
}
}

pub struct MidiInputPort<'a> {
ptr: NonNull<ffi::AMidiInputPort>,
_marker: PhantomData<&'a MidiDevice>,
}

impl<'a> fmt::Debug for MidiInputPort<'a> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("MidiInputPort")
.field("inner", &self.ptr)
.finish()
}
}

impl<'a> MidiInputPort<'a> {
/// Assumes ownership of `ptr`
///
/// # Safety
/// `ptr` must be a valid pointer to an Android [`ffi::AMidiInputPort`].
pub unsafe fn from_ptr(ptr: NonNull<ffi::AMidiInputPort>) -> Self {
Self {
ptr,
_marker: PhantomData,
}
}

pub fn ptr(&self) -> NonNull<ffi::AMidiInputPort> {
self.ptr
}

fn as_ptr(&self) -> *mut ffi::AMidiInputPort {
self.ptr.as_ptr()
}

/// Sends data to the specified input port.
MarijnS95 marked this conversation as resolved.
Show resolved Hide resolved
pub fn send(&self, buffer: &[u8]) -> Result<usize> {
let num_bytes_sent = unsafe {
ffi::AMidiInputPort_send(self.as_ptr(), buffer.as_ptr(), buffer.len() as ffi::size_t)
};
if num_bytes_sent >= 0 {
Ok(num_bytes_sent as usize)
} else {
Err(
NdkMediaError::from_status(ffi::media_status_t(num_bytes_sent as c_int))
.unwrap_err(),
)
}
}

/// Sends a message with a 'MIDI flush command code' to the specified port.
///
/// This should cause a receiver to discard any pending MIDI data it may have accumulated and
/// not processed.
pub fn send_flush(&self) -> Result<()> {
let result = unsafe { ffi::AMidiInputPort_sendFlush(self.as_ptr()) };
NdkMediaError::from_status(result)
}

pub fn send_with_timestamp(&self, buffer: &[u8], timestamp: i64) -> Result<usize> {
MarijnS95 marked this conversation as resolved.
Show resolved Hide resolved
let num_bytes_sent = unsafe {
ffi::AMidiInputPort_sendWithTimestamp(
self.as_ptr(),
buffer.as_ptr(),
buffer.len() as ffi::size_t,
timestamp,
)
};
if num_bytes_sent >= 0 {
Ok(num_bytes_sent as usize)
} else {
Err(
NdkMediaError::from_status(ffi::media_status_t(num_bytes_sent as c_int))
.unwrap_err(),
)
}
}
}

impl<'a> Drop for MidiInputPort<'a> {
fn drop(&mut self) {
unsafe { ffi::AMidiInputPort_close(self.as_ptr()) };
}
}

pub struct MidiOutputPort<'a> {
ptr: NonNull<ffi::AMidiOutputPort>,
_marker: PhantomData<&'a MidiDevice>,
}

impl<'a> fmt::Debug for MidiOutputPort<'a> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("MidiOutputPort")
.field("inner", &self.ptr)
.finish()
}
}

impl<'a> MidiOutputPort<'a> {
/// Assumes ownership of `ptr`
///
/// # Safety
/// `ptr` must be a valid pointer to an Android [`ffi::AMidiOutputPort`].
pub unsafe fn from_ptr(ptr: NonNull<ffi::AMidiOutputPort>) -> Self {
Self {
ptr,
_marker: PhantomData,
}
}

pub fn ptr(&self) -> NonNull<ffi::AMidiOutputPort> {
self.ptr
}

fn as_ptr(&self) -> *mut ffi::AMidiOutputPort {
self.ptr.as_ptr()
}

/// Receives the next pending MIDI message.
///
/// To retrieve all pending messages, the client should repeatedly call this method until it
/// returns [`Ok(MidiOpcode::NoMessage)`].
///
/// Note that this is a non-blocking call. If there are no Midi messages are available, the
/// function returns [`Ok(MidiOpcode::NoMessage)`] immediately (for 0 messages received).
MarijnS95 marked this conversation as resolved.
Show resolved Hide resolved
pub fn receive(&self, buffer: &mut [u8]) -> Result<MidiOpcode> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add an extra paragraph explaining that the MidiOpcode contains the length which the user must use to bound their slice?

Alternatively I've been toying with buffer: &mut &mut [u8]-like APIs, where I update the caller slice to *buffer = &mut buffer[..num_bytes_received] so that the user only gets the valid number of bytes back. This becomes more important when you start supporting [MaybeUninit<u8>] inputs (which may be useful here!). Alternatively MidiOpcode could borrow and return the valid sub-slice (instead of length?) so that the user has direct access to these.

Unfortunately all slice helpers around MaybeUninit are eternally stuck in "nightly-only experimental" land: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#method.slice_assume_init_mut

I'll leave this choice up to you, depending on convenience. The user quite likely already has something along the form of (i.e. it is quite hard to ignore/overlook MidiOpcode::Data::length):

match receive(&mut buffer)? {
    NoMessage => break,
    Data { length, timestamp: _ } => {
        let useful = &buffer[..length as usize];
        // ...
    }
    Flush => ...,
}

let mut opcode = 0i32;
let mut timestamp = 0i64;
let mut num_bytes_received: ffi::size_t = 0;
let result = unsafe {
ffi::AMidiOutputPort_receive(
self.as_ptr(),
&mut opcode,
buffer.as_mut_ptr(),
buffer.len() as ffi::size_t,
&mut num_bytes_received,
MarijnS95 marked this conversation as resolved.
Show resolved Hide resolved
&mut timestamp,
)
};

if result < 0 {
Err(NdkMediaError::from_status(ffi::media_status_t(result as c_int)).unwrap_err())
} else if result == 0 {
MarijnS95 marked this conversation as resolved.
Show resolved Hide resolved
Ok(MidiOpcode::NoMessage)
} else if opcode as c_uint == ffi::AMIDI_OPCODE_DATA {
Ok(MidiOpcode::Data {
length: num_bytes_received as usize,
timestamp,
})
} else {
Ok(MidiOpcode::Flush)
}
}
}

impl<'a> Drop for MidiOutputPort<'a> {
fn drop(&mut self) {
unsafe { ffi::AMidiOutputPort_close(self.as_ptr()) };
}
}