Possible solution to keep created surface consistent

[ztlwk]

Description

Motivation

This issue aims for the following topics:

• Users have to self-reference the window handle to the surface, which can be hard to implement.
• The created surfaces must be dropped by user when the the app was suspended, and we may somehow "enforce" it.
• Owning the optional window and surface in every update can have lifetime issues, and we may make some platform-specific changes to make it "consistent" to user.
• The "optional" surface problem seemed to be only present in Android.

I am unsure about whether winit should absolutely do some changes for them, but this issue is opened for the discusssions about the possible solutions and winit api design.

Proposed Change

• Require the user to implement a "bridge" type, which owns the corresponding surface, and gets dropped and recreated by winit when being suspended in Android.
• Make the "bridge" type or ApplicationHandler trait include the window recreation logic, so winit can recreate the window and own it, which assured the users to have a "window" in every update.

Relevant platforms

Android

[daxpedda]

See #3699.
Cc @MarijnS95.

[MarijnS95]

Seen this while reading #3699, and I'm more inclined to pursue that issue. For one, this issue doesn't describe a problem statement, so it's hard to understand what kind of issue or design problem the author intends this proposed design for.

Maybe this is somewhat loosely related to #2337?

[daxpedda]

I think the goal of OP is that we somehow avoid having to re-create RawWindowHandle.
You indicated in the last meeting that this is possible, because that's how it was in the past, could you clarify the details here?

So my understanding is that Window survives just fine.
RawWindowHandle becomes invalid but not unsafe, e.g. rendering just goes into a black hole but doesn't segfault.

How much exactly of the graphics context is invalidated, how about GPU buffers?
Is it possible for Winit to drape over this fact and "patch" the RawWindowHandle so we can pretend everything's continuing to work? So just notifying the user that the app is now in the background and no rendering will be visible and after resumption everything continues to work just fine.

[ztlwk]

Sorry for the vague description, I have edited it to make it more clear.

[MarijnS95]

So my understanding is that Window survives just fine.

On Android, if we consider Window to be analogous to an Activity (of which we can hackily only have just one currently), yes. The NativeWindow/Surface in Android is "just" its temporary backing buffer (or well, buffer queue / producer end).

---

RawWindowHandle becomes invalid but not unsafe, e.g. rendering just goes into a black hole but doesn't segfault.

rust-windowing/raw-window-handle#111 (comment)

---

How much exactly of the graphics context is invalidated, how about GPU buffers?

Just the Surface and Swapchain in Vulkan terms. In GL your context becomes "uncurrent" (because it can only be current when there's a render target, i.e. a Surface, as far as I understood) but the UBOs should continue to exist?

---

Is it possible for Winit to drape over this fact and "patch" the RawWindowHandle so we can pretend everything's continuing to work? So just notifying the user that the app is now in the background and no rendering will be visible and after resumption everything continues to work just fine.

@daxpedda doing some code/discussion archaeology: such a thing was originally implemented in rust-windowing/raw-window-handle#110 (winit side pending in #2744), and later reverted in rust-windowing/raw-window-handle#126.

[rib]

It's hard to know where to pick this up, but while I've been debugging some errors on Android when switching apps, and looking at the latest way in which Winit implements a HasWindowHandle trait for Window, the implementation doesn't look safe for Android...

Looking at the safety comment in this implementation of HasWindowHandle:

#[cfg(feature = "rwh_06")]
impl rwh_06::HasWindowHandle for Window {
    fn window_handle(&self) -> Result<rwh_06::WindowHandle<'_>, rwh_06::HandleError> {
        let raw = self.window.raw_window_handle_rwh_06()?;

        // SAFETY: The window handle will never be deallocated while the window is alive,
        // and the main thread safety requirements are upheld internally by each platform.
        Ok(unsafe { rwh_06::WindowHandle::borrow_raw(raw) })
    }
}

That safety assertion doesn't hold for Android. The raw handle (which doesn't appear to _acquire a ref count on the underlying ANativeWindow) will be a pointer that can become invalid whenever we get a TerminateWindow event (i.e. when the app is about to suspend).

Maybe this is a raw_window_handle issue really though :/

On Android the definition of AndroidNdkWindowHandle seems inherently unsafe:

/// Raw window handle for Android NDK.
#[non_exhaustive]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct AndroidNdkWindowHandle {
    /// A pointer to an `ANativeWindow`.
    pub a_native_window: NonNull<c_void>,
}

since that hands out a Copyable pointer that may become invalid.

maybe it's intentional that raw handles are as unsafe as raw pointers though and it's supposed to be your responsibility to know they are valid before you touch them - but it's scary that you can acquire the handles though safe apis.

WindowHandle::as_raw() provides a safe API for copying a raw handle from a WindowHandle and since the raw handle is Copy there's no constraint on how long you can keep that pointer.

I never really followed the design of the borrowed::HasWindowHandle trait + borrowed::WindowHandle but the docs say "//! These should be 100% safe to pass around and use, no possibility of dangling or invalidity." which unfortunately does not look like it's true on Android.

I think I would have expected the implementation of borrowed::WindowHandle::borrow_raw on Android to call ANativeWindow_acquire in order to get an owning reference for the ANativeWindow to be able guarantee that the raw handle / pointer will remain valid for the lifetime of the WindowHandle (and implement a Drop method that calls ANativeWindow_release(). Unfortunately though this doesn't look like it would be possible without an API break because WindowHandle also implements Copy - which precludes being able to _acquire and _release ref counts in Clone and Drop.

[MarijnS95]

The WindowHandle has a lifetime which inherently ties it to the lifetime of a winit::Window which does (if the code in winit/android-activity didn't get broken) represent an _acquire.

as_raw() isn't unsafe like any other Rust API that takes a pointer out of a safe object; only when turning that pointer back into a safe object (or dereferencing it) do the safety conditions need to be upheld.

---

Returning an acquired handle via RawWindowHandle won't work as you found out. Perhaps there are disjoint use-cases where the raw-window-handle crate could provide refcounted handles (if available on the platform) though.

EDIT: See also the links in my comment above to a horrible implementation change in raw-window-handle that allowed the Window handle to be wiped right out from the RawWindowHandle asynchronously.

---

but while I've been debugging some errors on Android when switching apps

I've rewritten android-activity and the winit backend for Android to finally start dealing with multiple activity handles, by making them analogous to a Window. Beforehand the implementation seemed to rely on it being a globally available singleton though which may have driven "dubious" ANativeWindow liveliness decisions though; I don't fully remember as I only have the new implementation fresh on my mind.

We do need to resolve #3206 (via #4419) and #4420 on the winit API side first though to make this functional.

[kchibisov]

Raw handles are unsafe, WindowHandle<'_> is safer version due to lifetime bound, but this lifetime bound is not enforced(I think it's 'static in some cases where it really shouldn't but it's winit problem).

I think I would have expected the implementation of

Isn't it a problem of particular implementation like we have in winit? If you have 'a lifetime it's safe to work with? Like in winit will have WindowHandle to ref count, at least that what I wanted to do.