android: Hold NativeWindow lock until after notifying the user with Event::Suspended

[MarijnS95]

This applies rust-mobile/ndk#117 on the winit side: Android destroys its window/surface as soon as the user returns from onNativeWindowDestroyed, and we "fixed" this on the ndk-glue side by sending the WindowDestroyed event before locking the window and removing it: this lock has to wait for any user of ndk-glue - ie. winit - to give up its readlock on the window, which is what we utilize here to give users of winit "time" to destroy any resource created on top of a RawWindowHandle.

since we can't pass the user a RawWindowHandle through the HasRawWindowHandle trait we have to document this case explicitly and keep the lock alive on the winit side instead.

• Tested on all platforms changed
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality
• Updated feature matrix, if new features were added or implemented

---

I'm creating this as draft because there are different things to consider: for one I've constantly commented that we should probably pass the RawWindowHandle and/or Android NativeWindow inside the Resumed event somehow across every platform for consistency, and it'd be great if this includes the lock to make the user responsible for holding it instead of implicitly unlocking after Event::Suspended. Then someone recently created a whole bunch of very long issues mentioning similar (concurrency) issues and longevity of the Android backend in general - sorry I've yet to catch up to those but there are too many and they're way too long 😁

[rib]

then someone recently created a whole bunch of very long issues mentioning similar (concurrency) issues and longevity of the Android backend in general - sorry I've yet to catch up to those but there are too many and they're way too long

hehe, yeah that'll be me, sorry :)

TL;DR = I don't think ndk-glue handles native <-> Java synchronization robustly at the moment.

Ideally I think it could be good for ndk-glue to provide a callback based API for polling + dispatching events which can handle synchronization in a way that's similar to the original android_native_app_glue that's ships with the Android NDK where the glue can insert "pre" and "post" code around callbacks that let ndk-glue encapsulate this kind of synchronization so downstream users don't have to follow this kind of convention to get correct synchronization.

I feel like this is an error-prone design at the moment, that pushes the fiddly burden of handling synchronization to downstream users (winit) while also exposing implementation details that could ideally be encapsulated in the glue layer (now winit will depend on parking_lot because that's how ndk_glue internally locks the window state, and ndk_glue now can't change how it handles synchronization without breaking it's API)

I've tried to solve this with how I designed these native-activity and game-activity glue crates, including also handling synchronization for saving app state.

Those concerns aside for a moment though; the changes in this draft look good for being able to address the main synchronization issue I've seen with ndk-glue for window surface destruction.

[MarijnS95]

TL;DR = I don't think ndk-glue handles native <-> Java synchronization robustly at the moment.

TL;DR: I can't but agree.

In principle NDK glue looks like a straightforward, concise wrapper. Using a pipe to tie into looper, and using locks to synchronize objects created and destroyed in its callbacks. Consumers of ndk-glue - in particular winit which lives under the same orga - are probably designed to tightly work with/around the design of ndk-glue, and hide its implementation details. That didn't really seem to have happened, and the Android backend in winit feels like it works "just good enough" and the missing features are only slowly trickling in when users finally need it (ie. it doesn't seem to be used often).

In the end ndk-glue could have also provided a (callback) layer on top of the looper, pipe, and locks, instead of having this implemented inside winit and any other possible consumer of ndk-glue. Combine that with winits "poor" (because desktop differs here) handling of "dynamic" windows that "randomly" (dis)appear and require platform-specific Suspended and Resumed events and the oddities of the Android API trickle down all the way into user code. But that's a discussion better left for #2185/#2293.

so downstream users don't have to follow this kind of convention to get correct synchronization.

For the record I have yet to find a primitive that helps users be aware that they must handle a WindowDestroyed/Suspended event in terms of destroying their Vulkan/OpenGL surfaces, before returning/continuing from the handling of this event.

now winit will depend on parking_lot because that's how ndk_glue internally locks the window state, and ndk_glue now can't change how it handles synchronization without breaking it's API

parking_lot is not necessary for this change. It simply makes the API more convenient by not having an Option inside the ReadLock that you know will never be None after initially checking it (fwiw many other platform backends have a parking_lot dependency as well, this locking crate appears to be widely accepted).

Those concerns aside for a moment though; the changes in this draft look good for being able to address the main synchronization issue I've seen with ndk-glue for window surface destruction.

Yes, I'm surprised we fixed this in ndk-glue but the change wasn't yet applied to winit... Priorities I guess.

What's your thought: get this in now, and keep discussing the future with your game/native-activity wrappers and see where we end up time-wise? There are currently some tricky issues with ndk-glue which'll take time to resolve that I don't have; at the same time your crate may solve a bunch of those implicitly but will also take quite some time/effort to discuss and migrate to? Most importantly updating the user-facing winit part to better suit Android instead of feeling and behaving like an afterthought.

[MarijnS95]

Addendum: It's funny that I just now started reading #2293 and addressed your main concern with this PR, while I was really doing that after noticing that it needed to be done in glutin because winit didn't do it yet. The comment you quoted came from another ndk-glue - but not winit - user who saw the same concerns and I just forgot to update winit after fixing it there.

However.

I may have done that "on purpose".

In certain cases the caller may invoke ndk_glue::native_window(), and themselves be responsible for storing the lock next to their handle on a Vulkan/OpenGL surface, instead of winit.

At least this is what I initially thought when voicing these concerns in rust-windowing/glutin#1411 (comment), where this thought is valid.

But then.

It is more common for applications to use the HasRawWindowHandle trait on a winit::Window, perhaps even the glutin implementation should use that without ever depending on the ndk_glue crate directly.

---

Summing that up, this PR is probably the right thing to do after all given that users of winit don't use ndk_glue hence don't see the lock as that's now hidden behind the Suspended/Resumed events. Otherwise there's no harm in holding the readlock twice.

[rib]

For the record I have yet to find a primitive that helps users be aware that they must handle a WindowDestroyed/Suspended event in terms of destroying their Vulkan/OpenGL surfaces, before returning/continuing from the handling of this event.

Right, I think this is a big enough portability hazard that I think it needs to be addressed at a higher level than just the Android backend. I don't tend to think there's a simple primitive we can find to solve this within the Android backend.

With issue #2182 and #2293 I think I'd currently like to advocate for Winit supporting resume events consistently across all platforms, and then either just updating all examples and potentially introducing some impediments on other platforms that strongly encourage following a pattern where GPU resources like your context and first window + surface are only lazily allocated once you reach your first resume event.

I recently worked on two downstream projects to investigate the feasibility of this and I was able to get both egui and Bevy running based on this model, in such a way that it also worked to run on Windows following the same model.

E.g. this Egui example, based on Winit, runs on Android and Windows: https://github.com/rib/agdk-rust/blob/main/examples/agdk-egui/src/lib.rs
and here's a proof of concept for Bevy too: https://github.com/rib/bevy/tree/android-deferred-render-init (I had a local Winit branch that generates Resumed events on Windows so the same approach ran on Windows)
I'd say that Bevy was by far the more complex architecture to adapt, but in the end it wasn't too big of a change.

[rib]

parking_lot is not necessary for this change. It simply makes the API more convenient by not having an Option inside the ReadLock that you know will never be None after initially checking it (fwiw many other platform backends have a parking_lot dependency as well, this locking crate appears to be widely accepted).

Yep, and to be clear, I'm not making any kind of value judgement on parking lot here which is definitely widely used and yeah it makes sense that other backends are also using it. I'm more just concerned with the leaky ndk-glue abstraction. The choice of locking and synchronization primitives can ideally be encapsulated within a library. In this case ndk-glue now has a public API that imposes a parking_lot dependency on its downstream users and it also means ndk_glue can't simply switch to a different synchronization strategy without breaking its public API, which doesn't seem ideal to me.

[rib]

What's your thought: get this in now, and keep discussing the future with your game/native-activity wrappers and see where we end up time-wise? There are currently some tricky issues with ndk-glue which'll take time to resolve that I don't have; at the same time your crate may solve a bunch of those implicitly but will also take quite some time/effort to discuss and migrate to? Most importantly updating the user-facing winit part to better suit Android instead of feeling and behaving like an afterthought.

My gut instinct atm is that there's no reason to block a fix like this on any other proposal to potentially swap out ndk_glue; this should hopefully fix a legitimate synchronization issue that exists now, and the concerns around leaky API etc don't extend outside of the Android backend so they don't affect users of the Winit API.

Thinking ahead; I'm currently thinking of creating a shim android_activity crate (I was considering renaming my agdk-rust repo to android-activity) that would provide a single place that can allow Rust applications to configure their Activity glue layer. For starters the crate would optionally depend on the native-activity and game-activity crates and let you pick a glue layer based on a feature. Additional glue layers could be implemented to support different Activity base classes (E.g. there were some ideas to create a RustActivity crate suggested here: rust-mobile/ndk#266 (comment)).

This would result in a single crate + API for Winit to use for Android glue while giving applications the ability to choose different Activity base classes. Right now my experimental Winit backend conditionally switches between native-activity and game-activity based on a feature in Winit (since the APIs are compatible it just needs a #[cfg()] test around the use *-activity as ndk_glue currently), but I was thinking it might be beneficial to push that out to a separate crate in case there are opportunities to share code for different activity classes and to allow for non-winit use cases that could also benefit from a common API.

Also just for reference, I'm not expecting that android_activity would have to be a fully encapsulated, lowest-common-denominator API for different activity base classes (since otherwise we wouldn't be able to support any additional Android features in Winit beyond what NativeActivity allows). I was thinking there would be a core API that's common across activity classes (such as poll_events() and the input API and APIs for saving state + querying data_dirs) and then the features for selecting the implementation would also be able to expose APIs that may be specific to that activity class.

For the more general challenge with Android being a kind of second-class platform atm then I mainly thinking about trying to normalize lazy graphics state initialization via resumed events across platforms as a starting point, re: #2307 (comment)

[rib]

But then.

It is more common for applications to use the HasRawWindowHandle trait on a winit::Window, perhaps even the glutin implementation should use that without ever depending on the ndk_glue crate directly.

Summing that up, this PR is probably the right thing to do after all given that users of winit don't use ndk_glue hence don't see the lock as that's now hidden behind the Suspended/Resumed events. Otherwise there's no harm in holding the readlock twice.

hmm, yeah, I didn't realize glutin was currently dependent on ndk_glue directly. Especially considering this kind of locking design in ndk_glue I would think it's probably for the best to at least aim to keep the synchronization responsibilities encapsulated inside Winit, and would guess it'd be best for glutin to be getting it's raw window handles via Winit.

[MarijnS95]

For the record I have yet to find a primitive that helps users be aware that they must handle a WindowDestroyed/Suspended event in terms of destroying their Vulkan/OpenGL surfaces, before returning/continuing from the handling of this event.

Right, I think this is a big enough portability hazard that I think it needs to be addressed at a higher level than just the Android backend. I don't tend to think there's a simple primitive we can find to solve this within the Android backend.

In case it wasn't entirely clear I'm specifically looking for a solution that forces the API to be consistent across all platforms, to replace the Android-specific after-thought that we have now. In addition something that more clearly pushes the burden of managing the implicit dependency between Vulkan/GL surfaces and a window.

However, I initially limited my thoughts to RawWindowHandle in Resumed (and removing the getter/trait from Window), because Window is pretty much an empty shell on Android that I don't look at much. But for true platform agnostic behaviour things presumably need to change across the board.

For one I remember some getter functions on Window not behaving correctly prior to Resumed (IIRC it was the window size, which isn't surprising) but will need to recheck that before commenting further.

With issue #2182 and #2293 I think I'd currently like to advocate for Winit supporting resume events consistently across all platforms, and then either just updating all examples and potentially introducing some impediments on other platforms that strongly encourage following a pattern where GPU resources like your context and first window + surface are only lazily allocated once you reach your first resume event.

Exactly this. Slightly more burden for "desktop app" writers, but they'll "thank us" when their app runs flawless on mobile, instead of finding out through obscure errors that they need to handle this differently.

---

Yep, and to be clear, I'm not making any kind of value judgement on parking lot here which is definitely widely used and yeah it makes sense that other backends are also using it. I'm more just concerned with the leaky ndk-glue abstraction. The choice of locking and synchronization primitives can ideally be encapsulated within a library. In this case ndk-glue now has a public API that imposes a parking_lot dependency on its downstream users and it also means ndk_glue can't simply switch to a different synchronization strategy without breaking its public API, which doesn't seem ideal to me.

I guess we could newtype-wrap it just to get this issue done and over with, at which point we're free to implement whatever locking abstraction we want.

---

My gut instinct atm is that there's no reason to block a fix like this on any other proposal to potentially swap out ndk_glue; this should hopefully fix a legitimate synchronization issue that exists now, and the concerns around leaky API etc don't extend outside of the Android backend so they don't affect users of the Winit API.

...

For the more general challenge with Android being a kind of second-class platform atm then I mainly thinking about trying to normalize lazy graphics state initialization via resumed events across platforms as a starting point, re: #2307 (comment)

I guess we should talk to the winit maintainers for a timeline on the next release. Specifically I'd like to get these changes merged but without a git dependency - though at the same time that forces the next winit release to coordinate with an NDK/ndk_glue breaking release as requested in #1995.

On the side, in separate issues that don't fill up this PR, we should discuss how to proceed on making Android a first-class citizen, how to lay out the underlying crates and architecture, etc.

---

hmm, yeah, I didn't realize glutin was currently dependent on ndk_glue directly. Especially considering this kind of locking design in ndk_glue I would think it's probably for the best to at least aim to keep the synchronization responsibilities encapsulated inside Winit, and would guess it'd be best for glutin to be getting it's raw window handles via Winit.

We've already shoved ndk_glue out as soon as it came in, since this should be handled through raw-window-handle instead: rust-windowing/glutin#1418

Over time (I don't use glutin, so progress only happens in down-time) I'm pushing towards working winit out the door too: raw-window-handle was specifically designed for this, allows for a more generic implementation (just a match block) across all platforms, and will allow arbitrary Android Surfaces (NativeWindows...) to be used as render target (rust-mobile/ndk#274).

[MarijnS95]

@rib I like the newtype suggestion, see rust-mobile/ndk#288. This is now all ready from my side, and probably the last change we can do before pushing out 0.27. I wish we could do a larger Suspended/Resumed refactor, but that'll probably be a larger endeavour requiring precise API design.

[MarijnS95]

This applies rust-windowing/android-ndk-rs#117 on the winit side

It is thanks to @rib's commit above that links to these PRs, that I found #1892 again which did end up applying rust-mobile/ndk#117 to winit. It wasn't forgotten about, I just forgot about that PR being open and ending up stale...

[maciejgodek]

#1892 author here, glad this got fixed eventually! Apologies for abandoning the PR, my priorities changed for a while. :)

[MarijnS95]

@maciejgodek no worries, I think this code even ended up being replaced with a completely different android-activity backend now. Feel free to close the PR to that effect though, as there's nothing actionable anymore.