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.

Sign up for GitHub

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

Jump to bottom

Switch from android_glue to ndk-glue and remove outdated lifecycle handing #1411

Merged
merged 1 commit into from
May 30, 2022
Merged

Switch from android_glue to ndk-glue and remove outdated lifecycle handing #1411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@

- Fix crash when creating OpenGLES context without explicit version.
- Add `buffer_age` method on `WindowedContext`.
- On Android, switched from `StaticStructGenerator` to `StructGenerator`, fixing some compilation errors.
- Return an `Err` instead of panicking when surfaceless GLX context creation fails on Linux.
- Fix compilation on Android:
- Switch from `StaticStructGenerator` to `StructGenerator` to dynamically load symbols.
- Replace `android_glue` dependency with `ndk-glue`, and remove broken lifecycle event handling.
- Glutin can now be used on Android, however, the application must ensure it only creates the `Context` following a winit `Event::Resumed` event, and destroys the `Context` in response to a `Event::Suspended` event.

# Version 0.28.0 (2021-12-02)

Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ Glutin is only officially supported on the latest stable version of the Rust com

To compile the examples for android, you have to use the `cargo apk` utility.

See [the `android-rs-glue` repository](https://github.com/rust-windowing/android-rs-glue) for instructions.
See [`cargo-apk` in the `android-ndk-rs` repository](https://github.com/rust-windowing/android-ndk-rs/cargo-apk) for instructions.

### Emscripten with asmjs

Expand Down
2 changes: 1 addition & 1 deletion glutin/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ lazy_static = "1.3"
winit = { version = "0.26", default-features = false }

[target.'cfg(target_os = "android")'.dependencies]
android_glue = "0.2"
glutin_egl_sys = { version = "0.1.5", path = "../glutin_egl_sys" }
libloading = "0.7"
ndk-glue = "0.5" # Keep in sync with winit
parking_lot = "0.11"

[target.'cfg(target_os = "emscripten")'.dependencies]
Expand Down
55 changes: 4 additions & 51 deletions glutin/src/api/android/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use crate::api::egl::{Context as EglContext, NativeDisplay, SurfaceType as EglSu
use crate::CreationError::{self, OsError};
use crate::{Api, ContextError, GlAttributes, PixelFormat, PixelFormatRequirements, Rect};

use crate::platform::android::EventLoopExtAndroid;
use glutin_egl_sys as ffi;
use parking_lot::Mutex;
use winit;
Expand All @@ -23,30 +22,6 @@ struct AndroidContext {
#[derive(Debug)]
pub struct Context(Arc<AndroidContext>);

#[derive(Debug)]
struct AndroidSyncEventHandler(Arc<AndroidContext>);

impl android_glue::SyncEventHandler for AndroidSyncEventHandler {
fn handle(&mut self, event: &android_glue::Event) {
match *event {
// 'on_surface_destroyed' Android event can arrive with some delay
// because multithreading communication. Because of
// that, swap_buffers can be called before processing
// 'on_surface_destroyed' event, with the native window
// surface already destroyed. EGL generates a BAD_SURFACE error in
// this situation. Set stop to true to prevent
// swap_buffer call race conditions.
Comment on lines -32 to -38
Copy link
Member

@MarijnS95 MarijnS95 May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah we "fixed" this in ndk-glue but it relies on the caller/user (winit) to hold a lock on the window and release it after its done all its cleanup upon receiving WindowDestroyed. Looks like at least winit's fn raw_window_handle() -> RawWindowHandle doesn't do that yet, but new_windowed() here calls directly into ndk_glue::native_window() where we can implement that 🎉

It should perhaps be part of this PR though (to store nwin inside the context).

Copy link
Member

Choose a reason for hiding this comment

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

It looks like #1320 reported this too, which we can probably close as "outdated" when this is merged.

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw the API is currently annoying with an Option inside the lock, I'm thinking about changing ndk-glue to parking_lot which allows mapping - in this case specifically for transmuting - the Option out of the RWLockReadGuard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Putting the RwLockGuard in AndroidContext fails to compile because it is not Sync:

   Compiling glutin v0.28.0 (/home/jamie/src/glutin/glutin)
error[E0277]: `std::sync::RwLockReadGuard<'static, Option<NativeWindow>>` cannot be sent between threads safely
   --> glutin/src/context.rs:204:6
    |
204 | impl FailToCompileIfNotSendSync for Context<NotCurrent> {}
    |      ^^^^^^^^^^^^^^^^^^^^^^^^^^ `std::sync::RwLockReadGuard<'static, Option<NativeWindow>>` cannot be sent between threads safely
    |
    = help: within `AndroidContext`, the trait `Send` is not implemented for `std::sync::RwLockReadGuard<'static, Option<NativeWindow>>`
    = note: required because it appears within the type `Option<std::sync::RwLockReadGuard<'static, Option<NativeWindow>>>`
note: required because it appears within the type `AndroidContext`
   --> glutin/src/api/android/mod.rs:18:8
    |
18  | struct AndroidContext {
    |        ^^^^^^^^^^^^^^
    = note: required because of the requirements on the impl of `Sync` for `Arc<AndroidContext>`
note: required because it appears within the type `api::android::Context`
   --> glutin/src/api/android/mod.rs:25:12
    |
25  | pub struct Context(Arc<AndroidContext>);
    |            ^^^^^^^
note: required because it appears within the type `context::Context<context::NotCurrent>`
   --> glutin/src/context.rs:34:12
    |
34  | pub struct Context<T: ContextCurrentState> {
    |            ^^^^^^^
note: required by a bound in `FailToCompileIfNotSendSync`
   --> glutin/src/context.rs:201:18
    |
199 | trait FailToCompileIfNotSendSync
    |       -------------------------- required by a bound in this
200 | where
201 |     Self: Send + Sync,
    |                  ^^^^ required by this bound in `FailToCompileIfNotSendSync`

Unless I'm misunderstanding your suggestion? Any ideas how to fix?

Copy link
Member

@MarijnS95 MarijnS95 May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You're not misunderstanding my suggestion, it's just a little oversight that I haven't checked further than "this lock guard must be held in some way" and now that you actually try it, see that glutin requires its context to be Send/Sync.

My gut feeling says that glutin relies on the (EGL) context is internally synchronized (hence safe to not only Send to a different thread, but also access from multiple threads concurrently (Sync) without locking on the Rust side). A lock guard is obviously not, so it seems like we'll have to wrap the lock guard inside a Mutex 🤭. Logically that doesn't solve the requirement for Send though, maybe those locks from parking_lot can be moved into a different thread?

Copy link
Member

@MarijnS95 MarijnS95 May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For reference, the nomicon nicely explains why lock guards are not Send:

https://doc.rust-lang.org/nomicon/send-and-sync.html

A nice example where this does not happen is with a MutexGuard: notice how it is not Send. The implementation of MutexGuard uses libraries that require you to ensure you don't try to free a lock that you acquired in a different thread.

Looks like parking_lot supports sending a lock guard though, as long as we enable send_guard: https://github.com/Amanieu/parking_lot#usage

At this point it feels like we might put this issue on ice, and solve it when we get to it - as I'd like to address it in winit as well and anyway need parking_lot to be able to map a lock guard and get rid of the inner Option<>.

EDIT: If we do skip this (I'm out of ideas bar migrating ndk-glue to parking_lot), please put a TODO comment in the code here to explain that we should really hold on to the lock even if the context is moved to a different thread.

Copy link
Member

Choose a reason for hiding this comment

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

@jamienicol Here you go:

rust-mobile/ndk#282
jamienicol/glutin@ndk-glue...MarijnS95:ndk-glue-native-window-lock

Copy link
Member

Choose a reason for hiding this comment

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

We'll need to figure out something with the versioning - I had to do some patch hacks to get everything in the dependency chain (winit) to use the same ndk and ndk-glue. But that's after everyone is content with this idea :)

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw I've added this to winit in rust-windowing/winit#2307 to make the lock held at least as long as until after winit users processed and returned from Event::Suspended. With that in this becomes less of an issue but it still feels right to hold the lock at exactly the same place where the "consumer" of the window - an EGL window surface - is lifecycle-managed. However, given that we don't really help the user yet with lifetime management given the removal of set_suspend_callback, we should just get your PR merged without solving this one and I'll take it on later.

Copy link
Member

Choose a reason for hiding this comment

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

I've been having even more thoughts about this (rust-windowing/winit#2307 (comment)), and I don't think glutin should hold the lock for this bit of the API but depend on winit to do it (rust-windowing/winit#2307).

Then, with that squared away (though we can just "trust" the surface is safe for now since that's what everyone is doing and works "okay enough", until the next winit release with my PR in) glutin doesn't - shouldn't - use ndk_glue directly at all. winit uses raw-window-handle to expose these details, and glutin can simply use it directly:

MarijnS95@baaf31a

(I've added the locks to winit with that in mind: people will use that raw-window-handle interface and never see nor know about the Android lock directly).

In fact we can use this to our advantage to unify glutin to use this "generic" way to communicate window-handle pointers around and create GL contexts on it, that's how it's done for Vulkan at least: https://github.com/ash-rs/ash/tree/master/ash-window (out of scope for this PR).

android_glue::Event::TermWindow => {
let mut stopped = self.0.stopped.as_ref().unwrap().lock();
*stopped = true;
}
_ => {
return;
}
};
}
}

impl Context {
#[inline]
pub fn new_windowed<T>(
Expand All @@ -57,41 +32,19 @@ impl Context {
) -> Result<(winit::window::Window, Self), CreationError> {
let win = wb.build(el)?;
let gl_attr = gl_attr.clone().map_sharing(|c| &c.0.egl_context);
let nwin = unsafe { android_glue::get_native_window() };
if nwin.is_null() {
return Err(OsError("Android's native window is null".to_string()));
}
let nwin = ndk_glue::native_window();
let nwin =
nwin.as_ref().ok_or_else(|| OsError("Android's native window is null".to_string()))?;
let native_display = NativeDisplay::Android;
let egl_context =
EglContext::new(pf_reqs, &gl_attr, native_display, EglSurfaceType::Window, |c, _| {
Ok(c[0])
})
.and_then(|p| p.finish(nwin as *const _))?;
.and_then(|p| p.finish(nwin.ptr().as_ptr() as *const _))?;
let ctx = Arc::new(AndroidContext { egl_context, stopped: Some(Mutex::new(false)) });

let handler = Box::new(AndroidSyncEventHandler(ctx.clone()));
android_glue::add_sync_event_handler(handler);
let context = Context(ctx.clone());

el.set_suspend_callback(Some(Box::new(move |suspended| {
let mut stopped = ctx.stopped.as_ref().unwrap().lock();
*stopped = suspended;
if suspended {
// Android has stopped the activity or sent it to background.
// Release the EGL surface and stop the animation loop.
unsafe {
ctx.egl_context.on_surface_destroyed();
}
} else {
// Android has started the activity or sent it to foreground.
// Restore the EGL surface and animation loop.
unsafe {
let nwin = android_glue::get_native_window();
ctx.egl_context.on_surface_created(nwin as *const _);
}
}
})));

Ok((win, context))
}

Expand Down
50 changes: 0 additions & 50 deletions glutin/src/api/egl/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,6 @@ pub struct Context {
surface: Option<Mutex<ffi::egl::types::EGLSurface>>,
api: Api,
pixel_format: PixelFormat,
#[cfg(target_os = "android")]
config_id: ffi::egl::types::EGLConfig,
}

fn get_egl_version(
Expand Down Expand Up @@ -511,52 +509,6 @@ impl Context {
self.display
}

// Handle Android Life Cycle.
// Android has started the activity or sent it to foreground.
// Create a new surface and attach it to the recreated ANativeWindow.
// Restore the EGLContext.
#[cfg(target_os = "android")]
pub unsafe fn on_surface_created(&self, nwin: ffi::EGLNativeWindowType) {
let egl = EGL.as_ref().unwrap();
let mut surface = self.surface.as_ref().unwrap().lock();
if *surface != ffi::egl::NO_SURFACE {
return;
}
*surface = egl.CreateWindowSurface(self.display, self.config_id, nwin, std::ptr::null());
if surface.is_null() {
panic!("on_surface_created: eglCreateWindowSurface failed with 0x{:x}", egl.GetError())
}
let ret = egl.MakeCurrent(self.display, *surface, *surface, self.context);
if ret == 0 {
panic!("on_surface_created: eglMakeCurrent failed with 0x{:x}", egl.GetError())
}
}

// Handle Android Life Cycle.
// Android has stopped the activity or sent it to background.
// Release the surface attached to the destroyed ANativeWindow.
// The EGLContext is not destroyed so it can be restored later.
#[cfg(target_os = "android")]
pub unsafe fn on_surface_destroyed(&self) {
let egl = EGL.as_ref().unwrap();
let mut surface = self.surface.as_ref().unwrap().lock();
if *surface == ffi::egl::NO_SURFACE {
return;
}
let ret = egl.MakeCurrent(
self.display,
ffi::egl::NO_SURFACE,
ffi::egl::NO_SURFACE,
ffi::egl::NO_CONTEXT,
);
if ret == 0 {
panic!("on_surface_destroyed: eglMakeCurrent failed with 0x{:x}", egl.GetError())
}

egl.DestroySurface(self.display, *surface);
*surface = ffi::egl::NO_SURFACE;
}

#[inline]
pub fn get_proc_address(&self, addr: &str) -> *const core::ffi::c_void {
let egl = EGL.as_ref().unwrap();
Expand Down Expand Up @@ -991,8 +943,6 @@ impl<'a> ContextPrototype<'a> {
surface: surface.map(|s| Mutex::new(s)),
api: self.api,
pixel_format: self.pixel_format,
#[cfg(target_os = "android")]
config_id: self.config_id,
})
}
}
Expand Down