rust-mobile
ndk: Add AMidi interface
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!
Are there any additional fixes needed? Please let me know if there are any :)
Are there any additional fixes needed? Please let me know if there are any :)
There are a few review comments still unresolved, but nothing major. I just ended up having to deal with some high(er) priority items the past few days, thanks for your patience.
I accidently merged another branch to feat/amidi, I will make a new branch (feat/amidi-2) and a PR from the commit where we've left.
@paxbun please don't, just force-push the old commit back.
Otherwise all review context gets lost here.
(I can help you with that if needed)
I didn't know that force-pushing also works in this case... Actually, this is my first time accidentally merging branches already published for a PR. Thanks for your reply!
~~@MarijnS95 I found some occurrences of as ffi::size_t in the current code. Would it be better to change that into as _? (#370)~~ Merged the newly updated master into feat/amidi and removed all ffi::size_ts!
Hehe, those should all be gone now :)
Would you mind providing your feedback on #399 and rebase on that so we have consistent error types?
I changed the base branch from master to ndk-rework-media-error; should I revert it back to master?
I changed the base branch from
mastertondk-rework-media-error; should I revert it back tomaster?
That gives a nice overview of what this PR entails after #399 has been merged, but will have to be changed back when I inevitably merge that PR and auto-delete the branch (hope we can still do that, but I think we cannot...), so better to leave it at master and will re-review this once that PR is in :crossed_fingers:
@paxbun can you rebase this to make the changes from #399 trickle through and get rid of the conflicts?
Yes, we're using this in our app, though our app only uses MIDI input, not output. All my contributions to this repo were derived from that app.
As you can see our app features some audio-based ones, and those are based on ndk::audio. I then needed to implement MIDI input, and found that there's no ndk::midi, and that's why I said "Just noticed that ndk doesn't have an interface for AMidi."
Maybe making an example app for testing this (using android-activity) would be great, but I need some time for that.
No need to make an example, just pointing out that it sounded like this was added for the sake of adding it rather than needing it, all good if you're using it!
On the note of
MediaError::from_status(...).unwrap_err(), I feel like this is an API limitation ofMediaErrorand might think of something better at some point, since the case of handling any non-negative code as a useful value, any known negative number as an error code, and any unknown error code asUnknownStatusis prevalent here and elsewhere.
I was thinking of something along the lines of:
https://github.com/MarijnS95/ndk/compare/negative-error-code-helper
WDYT?
I was thinking of something along the lines of:
https://github.com/MarijnS95/ndk/compare/negative-error-code-helper
WDYT?
LGTM! There are 6 branches checking whether the return value is negative; midi.rs will benefit from from_status_if_negative.
@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 :tada:
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!
@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.
@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.
@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?
@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 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, andSafeMidiOutputPortthat 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.