Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
ndk: Add AMidi interface #353
Changes from 16 commits
2daf09c
9456589
9cf8581
9f7c9ef
466bf4a
4013f68
ff6697c
02630b0
d3d616e
4f53506
15b917f
5ae1cad
06ebda5
2c92479
b2ec4ea
6f262cc
01877bc
48b4416
aa26247
8235a5c
12df844
34357c7
23b18b0
c79c8c5
895350b
acce571
8900c1c
95220e7
85288eb
cc1c2e3
200ab56
408e460
9a5744b
412b6ad
aa4467e
3cfaad4
2e036d2
aa79a19
0341b4d
c8b2b2e
6044fc2
f22054d
734b81b
7220a9a
241b096
d9ef310
31159dc
129edc2
1ea5b32
83fe4b2
8c64160
5428ca7
802ab4a
300118e
8086a7d
5707555
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MidiIntputPort
is definitely notSync
: it putswrite
in the loop, so there's a possibility of interleaved writing.MidiOutputPort
is definitelySync
:MidiReceiver
, which is called byAMidiOutputPort_receive
implements synchronization logic.MidiOutputPort::receive
will immediately return anErr
if a simultaneous read call was invoked by another thread.MidiDevice
seems to beSync
: asMidiDeviceServer
, which is called byMidiDevice
, usessynchronized
in every method except forgetDeviceInfo()
andsetDeviceInfo()
, but I'm not sure.There was a problem hiding this comment.
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 asSend
withoutClone
/Copy
is also fine.There was a problem hiding this comment.
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
andMidiDeviceServer
- I think we can assume thatMidiDevice
isSync
as well.There was a problem hiding this comment.
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 ofamidi.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
viatransact()
andonTransact()
:IBinder MidiDevice.mDeviceServerBinder
, is set fromandroid_media_midi.cpp
MidiDevice.mDeviceServerBinder
is set in the constructor ofMidiDevice
:MidiDeviceServer.open{Input, Output}Port
all havesynchronized
block onmInputPortOutputPorts
andmOutputPortDispatchers[portNumber]
, which meansMidiDevice
is thread safe.There was a problem hiding this comment.
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 forSync
.There was a problem hiding this comment.
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 whenAMidiDevice_release
is called beforeAttachCurrentThread[AsDaemon]
is called from that thread, which is not a documented behavior. I'll removeSend
fromMidiDevice
.+++
We could add
SafeMidiDevice
type like the following, but I think this must be implemented by the user, not provided fromndk::midi
.There was a problem hiding this comment.
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
orAMidiOutputPort_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 callsAMidiDevice_release
.) Maybe we should removeimpl Send
fromMidiInputPort
andMidiOutputPort
as well?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 thelength
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!). AlternativelyMidiOpcode
could borrow and return the valid sub-slice (instead oflength
?) 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_mutI'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
):