ndk: Add AMidi interface

[paxbun]

Just noticed that ndk doesn't have an interface for AMidi, so I made one. Please let me know if there're any tests or fixes I have to do before merging!

[MarijnS95]

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)?

[paxbun]

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

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.

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.

[MarijnS95]

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.

[paxbun]

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

[paxbun]

• 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();
      ...

  3. ...and the Java-side MidiDeviceServer.open{Input, Output}Port all have synchronized block on mInputPortOutputPorts and mOutputPortDispatchers[portNumber], which means MidiDevice is thread safe.

[MarijnS95]

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.

[paxbun]

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 {}

[paxbun]

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?

[MarijnS95]

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.

[MarijnS95]

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?

[MarijnS95]

@paxbun fwiw the .unwrap_err() generalization is up at #414, if that looks okay to you we can rebase this on top - address the remaining conversions above - and get this merged in before the next release 🎉

[paxbun]

Sorry for the late reply; I'm working on another project. I'll try to make the work done by this weekend. Thanks for your patience!

[MarijnS95]

@paxbun all good, though you might miss out on the next breaking release window.

That doesn't hurt as we can always do a non-breaking patch release for these purely-additive APIs, and spend that tiny bit more time fleshing out the functions so that we don't have to make breaking changes in the near future.

[paxbun]

@MarijnS95 I think keeping Midi*Port Send is okay; they're just a holder of a file descriptor and some values representing the state of the port.

The Java counterpart of AMidi*Port implements the MIDI handling logic in pure Java, since that logic only requires file IO functions. Likewise, I think assuming that AMidi*Port won't depend on JNI in the future is okay, as the NDK implementation will also contain IO logic only.

Moreover, I think making Midi*Port Sync is also okay; As I said above, AMidiOutputPort is guarded by an atomic integer (it's like Mutex::try_lock), like Java's MidiOutputPort's implementations are also guarded with synchronized blocks. I thought AMidiInputPort_send was not thread-safe, but I just found this comments in the Android source code:

Since the MIDI socket is opened with SOCK_SEQPACKET, messages won't be interleaved even when AMidiInputPort_send is called from multiple threads.

Making MidiOutputPort Send is necessary because in the current NDK API AMidiOutputPort_receive is designed to be called in a separate thread; Though AMidiInputPort_send does not need a dedicated thread, I think some users would need to call it in another thread.

In addition, since Midi*Port<'a> has a lifetime parameter that depends on its associated MidiDevice and MidiDevice is not Send nor Sync, so most users who send Midi*Port<'a> to another thread will attach JNI to that thread.

If you think it's okay to keep Midi*Port Send (and Sync as well), I'll add documentation for this.

[MarijnS95]

@paxbun sounds like you've done enough research to confirm this.

But is it usable that way (e.g. sending ports to threads) when they have a lifetime dependency on the MidiDevice which itself cannot be sent? I don't think that's possible?

[paxbun]

@MarijnS95 You're right, that makes the lifetime parameter meaningless, I overlooked it.
But allowing MidiDevice to be used with multithreading is important for its practical use. How about adding SafeMidiDevice, SafeMidiInputPort, and SafeMidiOutputPort that ensure the safety conditions (e.g., JavaVM thread attachment) and hold their unsafe counterpart?

[MarijnS95]

@MarijnS95 You're right, that makes the lifetime parameter meaningless, I overlooked it.
But allowing MidiDevice to be used with multithreading is important for its practical use. How about adding SafeMidiDevice, SafeMidiInputPort, and SafeMidiOutputPort that ensure the safety conditions (e.g., JavaVM thread attachment) and hold their unsafe counterpart?

I had concerns about the lifetime still making it hard to work around this unless the MidiDevice is either refcounted and clonable, or wrapped in an Arc (same effect) and it seems you've gone for the latter.

Not sure if we really need a "safe" counterpart or can just make this the default implementation?

Anyways, I'll involve @rib since he did a lot on the JNI crate and is probably better at evaluating whether this is the right - and safe - way to go.

Stale since safe additions

[MarijnS95]

Some more () nits, though not sure if they're relevant at this stage.