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

Conversation

paxbun
Copy link
Contributor

@paxbun paxbun commented Oct 4, 2022

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!

ndk/src/midi.rs Outdated Show resolved Hide resolved
ndk/src/midi.rs Outdated Show resolved Hide resolved
ndk/src/midi.rs Outdated Show resolved Hide resolved
ndk/src/midi.rs Outdated Show resolved Hide resolved
Added Cargo feature `midi` and optional linkage to `libamidi.so`
Added Cargo feature `midi`, which enables `ffi/midi`, `api-level-29`,
and NdkMediaError in `media`
ndk/Cargo.toml Show resolved Hide resolved
ndk-sys/src/lib.rs Show resolved Hide resolved
ndk/src/midi.rs Outdated Show resolved Hide resolved
ndk/src/midi.rs Outdated Show resolved Hide resolved
ndk/src/midi.rs Outdated Show resolved Hide resolved
ndk/src/midi.rs Outdated Show resolved Hide resolved
ndk/src/midi.rs Outdated Show resolved Hide resolved
ndk/src/midi.rs Outdated Show resolved Hide resolved
ndk/src/midi.rs Outdated Show resolved Hide resolved
ndk/src/midi.rs Outdated
Comment on lines 22 to 28
// 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 {}
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?

ndk/src/midi.rs Outdated Show resolved Hide resolved
ndk/src/midi.rs Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Member

@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
Copy link
Contributor Author

paxbun commented Aug 15, 2023

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
Copy link
Member

@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
Copy link
Contributor Author

paxbun commented Aug 16, 2023

@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.
image
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:
image
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
Copy link
Member

@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
Copy link
Contributor Author

paxbun commented Aug 17, 2023

@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
Copy link
Member

MarijnS95 commented Aug 17, 2023

@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.

@MarijnS95 MarijnS95 requested a review from rib August 17, 2023 07:33
@MarijnS95 MarijnS95 dismissed their stale review August 17, 2023 07:33

Stale since safe additions

ndk/src/midi/safe.rs Outdated Show resolved Hide resolved
Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

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

ndk/src/midi/safe.rs Outdated Show resolved Hide resolved
ndk/src/midi/safe.rs Outdated Show resolved Hide resolved
ndk/src/midi/safe.rs Outdated Show resolved Hide resolved
ndk/src/midi/safe.rs Outdated Show resolved Hide resolved
ndk/src/midi/safe.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting Waiting for a response or another PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants