Switch from android_glue to ndk-glue and remove outdated lifecycle handing
#1411
Conversation
MarijnS95
changed the title
android_glue to ndk-glue
For the record: I'll PR my example as soon as I get home that showcases how this is done. And also for the record, we're working on changes to make this more ergonomic too. |
| // '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. |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
It looks like #1320 reported this too, which we can probably close as "outdated" when this is merged.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
(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).
MarijnS95
changed the title
android_glue to ndk-glueandroid_glue to ndk-glue and remove outdated lifecycle handing
|
@jamienicol As part of this PR, can you also update the |
…guards It has come up previously that the `native_window()` function is easier to use if we could transpose a `LockGuard<Option<>>` into an `Option<LockGuard<>>`. After all, as soon as you have the lock you only need to check the `Option<>` value once and are thereafter guaranteed that its value won't change, until the lock is given up. At the same time `parking_lot` has a `send_guard` feature which allows moving a lock guard to another thread (as long as `deadlock_detection` is disabled); a use case for this recently came up [in glutin] where the `NativeWindow` handle lock should be stored with a GL context so that our `fn on_window_destroyed()` callback doesn't return until after the `Surface` created on it is removed from the context and destroyed. Glutin forces this context to be `Send`, and a lock guard already allows `Sync` if `NativeWindow` is `Sync` (which it is). [in glutin]: rust-windowing/glutin#1411 (comment)
|
If you're looking at switching the glue layer, I wonder if it could be worth evaluating these glue crates that I've been working on recently, while I've been investigating how to support native Rust applications that aren't always based on NativeActivity. https://github.com/rib/agdk-rust The glue API was initially based on The API also supports input events, and exposes things like the |
|
@rib Let's tackle one thing at a time. This patch and some subsequent changes are working well for its purpose, and are simply necessary to accompany
Leave the sales-pich for |
Yep, absolutely, makes sense. Definitely not meaning to disrupt fixing issues that exist now.
haha, :) sorry for sounding like a sales pitch. I mainly just wanted to float the idea, with the specific technical context that the API in There's also room for incorporating a similar approach in |
jamienicol
force-pushed
the
ndk-glue
branch
2 times, most recently
from
c0222a9 to
a1557f3
Compare
|
@MarijnS95 I've updated the link in the readme and added the comment to the Cargo.toml file. And rebased and fixed the changelog conflict. Do we need to do anything with the native window lock in this PR? or do we need to wait for your winit PR to land, and update the winit dep in glutin first? |
|
Perhaps I should combine the two Android changelog entries to emphasise what the users will actually care about, eg:
How does that sound? |
|
@jamienicol Sounds good on the suggested changelog regrouping, I love some sub- |
Let's leave it like this for now. I'll probably plan a larger change that removes |
jamienicol
force-pushed
the
ndk-glue
branch
2 times, most recently
from
afbb6d4 to
008e5c2
Compare
Additionally remove the Android lifecycle handling code, as winit no longer provides the required callback meaning it does not compile. This fixes compilation on Android. However, there is a caveat: Glutin is no longer able to automatically destroy and create the Surface in response to Android lifecycle events. The application must therefore ensure they only create a Context following a winit Event::Resumed, and they destroy the context when receiving an Event::Suspended.
|
@rib:
Hehe you may have noticed I have a habit for spamming ideas and walls of text in PRs too (side-eye rust-windowing/winit#2307) 😁
It's a good idea to conjure up something that solves this broadly for the generic
It's all good, just that |
How embarrassing; I just read the title of this project in the README and I thought (And that in any case has little effect on |
…guards It has come up previously that the `native_window()` function is easier to use if we could transpose a `LockGuard<Option<>>` into an `Option<LockGuard<>>`. After all, as soon as you have the lock you only need to check the `Option<>` value once and are thereafter guaranteed that its value won't change, until the lock is given up. At the same time `parking_lot` has a `send_guard` feature which allows moving a lock guard to another thread (as long as `deadlock_detection` is disabled); a use case for this recently came up [in glutin] where the `NativeWindow` handle lock should be stored with a GL context so that our `fn on_window_destroyed()` callback doesn't return until after the `Surface` created on it is removed from the context and destroyed. Glutin forces this context to be `Send`, and a lock guard already allows `Sync` if `NativeWindow` is `Sync` (which it is). [in glutin]: rust-windowing/glutin#1411 (comment)
…guards It has come up previously that the `native_window()` function is easier to use if we could transpose a `LockGuard<Option<>>` into an `Option<LockGuard<>>`. After all, as soon as you have the lock you only need to check the `Option<>` value once and are thereafter guaranteed that its value won't change, until the lock is given up. At the same time `parking_lot` has a `send_guard` feature which allows moving a lock guard to another thread (as long as `deadlock_detection` is disabled); a use case for this recently came up [in glutin] where the `NativeWindow` handle lock should be stored with a GL context so that our `fn on_window_destroyed()` callback doesn't return until after the `Surface` created on it is removed from the context and destroyed. Glutin forces this context to be `Send`, and a lock guard already allows `Sync` if `NativeWindow` is `Sync` (which it is). [in glutin]: rust-windowing/glutin#1411 (comment)
…guards It has come up previously that the `native_window()` function is easier to use if we could transpose a `LockGuard<Option<>>` into an `Option<LockGuard<>>`. After all, as soon as you have the lock you only need to check the `Option<>` value once and are thereafter guaranteed that its value won't change, until the lock is given up. At the same time `parking_lot` has a `send_guard` feature which allows moving a lock guard to another thread (as long as `deadlock_detection` is disabled); a use case for this recently came up [in glutin] where the `NativeWindow` handle lock should be stored with a GL context so that our `fn on_window_destroyed()` callback doesn't return until after the `Surface` created on it is removed from the context and destroyed. Glutin forces this context to be `Send`, and a lock guard already allows `Sync` if `NativeWindow` is `Sync` (which it is). [in glutin]: rust-windowing/glutin#1411 (comment)
…guards It has come up previously that the `native_window()` function is easier to use if we could transpose a `LockGuard<Option<>>` into an `Option<LockGuard<>>`. After all, as soon as you have the lock you only need to check the `Option<>` value once and are thereafter guaranteed that its value won't change, until the lock is given up. At the same time `parking_lot` has a `send_guard` feature which allows moving a lock guard to another thread (as long as `deadlock_detection` is disabled); a use case for this recently came up [in glutin] where the `NativeWindow` handle lock should be stored with a GL context so that our `fn on_window_destroyed()` callback doesn't return until after the `Surface` created on it is removed from the context and destroyed. Glutin forces this context to be `Send`, and a lock guard already allows `Sync` if `NativeWindow` is `Sync` (which it is). [in glutin]: rust-windowing/glutin#1411 (comment)
…guards It has come up previously that the `native_window()` function is easier to use if we could transpose a `LockGuard<Option<>>` into an `Option<LockGuard<>>`. After all, as soon as you have the lock you only need to check the `Option<>` value once and are thereafter guaranteed that its value won't change, until the lock is given up. At the same time `parking_lot` has a `send_guard` feature which allows moving a lock guard to another thread (as long as `deadlock_detection` is disabled); a use case for this recently came up [in glutin] where the `NativeWindow` handle lock should be stored with a GL context so that our `fn on_window_destroyed()` callback doesn't return until after the `Surface` created on it is removed from the context and destroyed. Glutin forces this context to be `Send`, and a lock guard already allows `Sync` if `NativeWindow` is `Sync` (which it is). [in glutin]: rust-windowing/glutin#1411 (comment)
…guards (#282) It has come up previously that the `native_window()` function is easier to use if we could transpose a `LockGuard<Option<>>` into an `Option<LockGuard<>>`. After all, as soon as you have the lock you only need to check the `Option<>` value once and are thereafter guaranteed that its value won't change, until the lock is given up. At the same time `parking_lot` has a `send_guard` feature which allows moving a lock guard to another thread (as long as `deadlock_detection` is disabled); a use case for this recently came up [in glutin] where the `NativeWindow` handle lock should be stored with a GL context so that our `fn on_window_destroyed()` callback doesn't return until after the `Surface` created on it is removed from the context and destroyed. Glutin forces this context to be `Send`, and a lock guard already allows `Sync` if `NativeWindow` is `Sync` (which it is). [in glutin]: rust-windowing/glutin#1411 (comment)
…guards (#282) It has come up previously that the `native_window()` function is easier to use if we could transpose a `LockGuard<Option<>>` into an `Option<LockGuard<>>`. After all, as soon as you have the lock you only need to check the `Option<>` value once and are thereafter guaranteed that its value won't change, until the lock is given up. At the same time `parking_lot` has a `send_guard` feature which allows moving a lock guard to another thread (as long as `deadlock_detection` is disabled); a use case for this recently came up [in glutin] where the `NativeWindow` handle lock should be stored with a GL context so that our `fn on_window_destroyed()` callback doesn't return until after the `Surface` created on it is removed from the context and destroyed. Glutin forces this context to be `Send`, and a lock guard already allows `Sync` if `NativeWindow` is `Sync` (which it is). [in glutin]: rust-windowing/glutin#1411 (comment)
Closes #1307, closes #1313, closes #1334
Additionally remove the Android lifecycle handling code, as winit no longer provides the required callback meaning it does not compile.
This fixes compilation on Android. However, there is a caveat: Glutin is no longer able to automatically destroy and create the Surface in response to Android lifecycle events. The application must therefore ensure they only create a Context following a
winitEvent::Resumed, and they destroy the context when receiving anEvent::Suspended.cargo fmthas been run on this branchcargo docbuilds successfullyCHANGELOG.mdif knowledge of this change could be valuable to users