Initial JNI support #116

wvdschel · 2016-11-02T11:34:31Z

Feedback welcome!

wvdschel · 2016-11-02T12:09:46Z

Example code using these JNI bindings can be seen in rust-windowing/glutin#822

wvdschel · 2016-11-02T13:23:13Z

glue/src/jni.rs

+//}
+
+#[repr(C)]
+pub struct JNINativeInterface {


This was simply copied from injected-glue/ffi.rs

wvdschel · 2016-11-02T13:25:20Z

glue/src/jni.rs

+    static JNI_HANDLE : RefCell<JNIEnvHandle> = RefCell::new(JNIEnvHandle { jni_env: ptr::null_mut(), counter: 0 });
+}
+
+pub fn attach_thread() -> JNIEnv {


Detaching a thread in JNI is immediate, so nested attach/detach pairs are a pain. I've added simple reference counting here, so every call can simply enclose their code in jni::attach_thread()/jni::detach_thread() pairs, and only the final call will actually detach the thread.

Since we're providing a high-level interface, I think attach_thread should return an RAII guard that automatically calls detach_thread in its Drop implementation.

tomaka · 2016-11-02T14:25:40Z

cargo-apk/injected-glue/lib.rs

 /// This static variable  will store the android_app* on creation, and set it back to 0 at
 ///  destruction.
 /// Apart from this, the static is never written, so there is no risk of race condition.
 #[no_mangle]
 pub static mut ANDROID_APP: *mut ffi::android_app = 0 as *mut ffi::android_app;
+pub static mut CLASS_LOADER: ffi::jobject = 0 as ffi::jobject;


CLASS_LOADER and ACTIVITY need some documentation for what they are.

tomaka · 2016-11-02T14:28:29Z

glue/src/jni.rs

+    static JNI_HANDLE : RefCell<JNIEnvHandle> = RefCell::new(JNIEnvHandle { jni_env: ptr::null_mut(), counter: 0 });
+}
+
+pub fn attach_thread() -> JNIEnv {


Since we're providing a high-level interface, I think attach_thread should return an RAII guard that automatically calls detach_thread in its Drop implementation.

tomaka · 2016-11-02T14:35:41Z

Overall I think this needs some more high-level functions as well. But I'm not sure what the right border should be between the low-level bindings and a high-level wrapper, so this could eventually be done in a later PR.

tomaka · 2016-11-02T17:01:02Z

Here's a gist @sfackler made demonstrating a high-level API for JNI stuff: https://gist.github.com/sfackler/5c036006f8d58a1d38887ba014759f74
It is licensed under MIT/Apache2 so we can copy some stuff if we want.

As I said in the previous comment, this might be out of scope of this PR but I thought I'd leave it somewhere for reference.

sfackler · 2016-11-02T17:03:57Z

Might also be worth depending on https://crates.io/crates/jni-sys to avoid needing to have a second copy in here. Happy to make any changes necessary on that end.

wvdschel · 2016-11-03T13:24:53Z

@tomaka, what is your view on using @sfackler's jni-sys crate?
I'd prefer not to re-implement anything that we can readily reuse, but I'm not sure if you're happy to be adding another dependency.

Before I start addressing other feedback, it would be nice to decide if using jni-sys is on the table, and if we should add higher level wrappers to android-rs-glue, jni-sys, or some intermediate crate that wraps raw jni-sys in a nice, safe, Rust-y API.

I agree that adding a higher level API (like JNIEnv's that automatically detach_thread(), or jobject's that automatically DeleteGlobalRef/DeleteLocalRef when dropped). Most of all, I'd like to get rid of the various explicit method calls based on types.

But I'm also not sure this should be part of android-rs-glue.

@sfackler I don't think any explicit changes are required from looking at your code for a brief moment. There are some differences in the JNI implementation of Android, but those mostly (as far as I've read - I'm no expert) relate to how the JNI API is used, not the API itself.

wvdschel · 2016-11-03T13:30:50Z

An alternative approach would be to remove the JNI API from the glue module, and simply using the four methods provided by this patch in injected-glue from jni-sys, guarded by some #![cfg(target_os = "android")].

This would mean that android-glue-rs would not provide JNI itself, but JNI support can optionally be obtained when also including the jni-sys crate.

tomaka · 2016-11-03T14:12:29Z

In my opinion this is the ideal design:

The internal and the external glues both depend on jni-sys and communicate with each other to share the low-level objects (like you have done in your PR).
The external glue also depends on a new crate (named jni for example) that doesn't exist yet and that provides a high-level wrapper around the JNI.
The high-level wrappers are reexported in the external glue's API, but the low-level objects are not exposed.

wvdschel · 2016-11-03T15:11:27Z

I can agree with that. I'll look into making the adaptations over the weekend, probably.

The higher level JNI wrapper will probably take a longer time to complete.

@sfackler Are you interested in creating a repo for the higher level wrapper, or should I?

sfackler · 2016-11-03T17:01:34Z

I probably don't have the time to build out the high level wrapper right now unfortunately, so you might want to get it started.

wvdschel · 2016-11-21T10:07:39Z

Just a note that I have started on reworking this, but haven't found the time to finish it yet (because life).

fschutt · 2018-04-01T08:57:35Z

So ... what is the current state? The jni_sys crate is pretty mature now and it's been two years ...

fschutt · 2018-04-01T09:01:40Z

Why can't the ffi module simply be exposed / exported to the library user? This way the user can hook up the jni crate and from there on it's rather easy to interact with the JNI. I know about "API concerns", but .. something is better than nothing.

tomaka · 2018-04-01T11:23:22Z

I'm ok with exposing the JNI-related functions of the FFI, as long as they are contained within their own scope and don't require an external crate.

fschutt · 2018-04-02T22:59:08Z

@tomaka ... why? What's wrong about the jni crate? I mean ... do you want to re-do all of the work they've done just to not depend on them - how would you then do this function, if not using jni?

tomaka · 2018-04-03T08:33:23Z

Sorry, what I mean is that we should simply reexport the content of the jni-sys crate from our own crate, instead of exposing it in the API.

Initial JNI support

e404e1f

wvdschel force-pushed the master branch from 9986312 to e404e1f Compare November 2, 2016 12:08

wvdschel commented Nov 2, 2016

View reviewed changes

glue/src/jni.rs

//}

#[repr(C)]

pub struct JNINativeInterface {

Copy link

Contributor Author

wvdschel Nov 2, 2016

Choose a reason for hiding this comment

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

This was simply copied from injected-glue/ffi.rs

wvdschel commented Nov 2, 2016

View reviewed changes

tomaka suggested changes Nov 2, 2016

View reviewed changes

Add comments to ACTIVITY and CLASS_LOADER variables.

f8182c5

fschutt mentioned this pull request Apr 1, 2018

Expose get_physical_extents for the MonitorId rust-windowing/winit#442

Closed

This was referenced Jun 2, 2018

Implement display info for Android. rust-windowing/glutin#822

Closed

DPI for everyone rust-windowing/winit#548

Merged

Osspial mentioned this pull request Nov 20, 2018

HiDPI not fully supported on Android rust-windowing/winit#721

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial JNI support #116

Initial JNI support #116

wvdschel commented Nov 2, 2016

wvdschel commented Nov 2, 2016

wvdschel Nov 2, 2016

wvdschel Nov 2, 2016

tomaka Nov 2, 2016

tomaka Nov 2, 2016

tomaka Nov 2, 2016

tomaka commented Nov 2, 2016 •

edited

Loading

tomaka commented Nov 2, 2016 •

edited

Loading

sfackler commented Nov 2, 2016

wvdschel commented Nov 3, 2016 •

edited

Loading

wvdschel commented Nov 3, 2016 •

edited

Loading

tomaka commented Nov 3, 2016

wvdschel commented Nov 3, 2016

sfackler commented Nov 3, 2016

wvdschel commented Nov 21, 2016

fschutt commented Apr 1, 2018

fschutt commented Apr 1, 2018 •

edited

Loading

tomaka commented Apr 1, 2018

fschutt commented Apr 2, 2018

tomaka commented Apr 3, 2018

Initial JNI support #116

Are you sure you want to change the base?

Initial JNI support #116

Conversation

wvdschel commented Nov 2, 2016

wvdschel commented Nov 2, 2016

wvdschel Nov 2, 2016

Choose a reason for hiding this comment

wvdschel Nov 2, 2016

Choose a reason for hiding this comment

tomaka Nov 2, 2016

Choose a reason for hiding this comment

tomaka Nov 2, 2016

Choose a reason for hiding this comment

tomaka Nov 2, 2016

Choose a reason for hiding this comment

tomaka commented Nov 2, 2016 • edited Loading

tomaka commented Nov 2, 2016 • edited Loading

sfackler commented Nov 2, 2016

wvdschel commented Nov 3, 2016 • edited Loading

wvdschel commented Nov 3, 2016 • edited Loading

tomaka commented Nov 3, 2016

wvdschel commented Nov 3, 2016

sfackler commented Nov 3, 2016

wvdschel commented Nov 21, 2016

fschutt commented Apr 1, 2018

fschutt commented Apr 1, 2018 • edited Loading

tomaka commented Apr 1, 2018

fschutt commented Apr 2, 2018

tomaka commented Apr 3, 2018

tomaka commented Nov 2, 2016 •

edited

Loading

tomaka commented Nov 2, 2016 •

edited

Loading

wvdschel commented Nov 3, 2016 •

edited

Loading

wvdschel commented Nov 3, 2016 •

edited

Loading

fschutt commented Apr 1, 2018 •

edited

Loading