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.

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

Change the "Active" struct to be sound #116

Merged
merged 13 commits into from
Mar 29, 2023
Merged

Conversation

notgull
Copy link
Member

@notgull notgull commented Mar 24, 2023

The previous implementation is unsound in several cases. This PR ensures that window handles don't outlive the application.

Draft until I can verify that it actually works. Supersedes #114

src/borrowed.rs Outdated Show resolved Hide resolved
@notgull notgull marked this pull request as ready for review March 29, 2023 15:36
@notgull
Copy link
Member Author

notgull commented Mar 29, 2023

android-glutin-triangle

Looks like the glutin example works on Android, at least from a glance. I think we're ready to start merging and releasing this.

@notgull notgull requested a review from Lokathor March 29, 2023 16:05
@Lokathor
Copy link
Contributor

Just having a look on the bus, things seem pretty good.

I'd like for there to be more documentation, perhaps at the module level, about how a person writing a system lib should approach things (eg: winit/sdl2 folks), and how a person writing a graphics lib (eg: wgpu/ash folks) should approach things. I think the ideal is that people other than those two groups shouldn't have to look too carefully in here? Just pass a winit thing to a wgpu thing? So the system lib audience and graphics lib audience seem like our main users.

I wouldn't say that this is a blocking concern, but "good docs" is the second most important part of a crate.

Copy link
Contributor

@Lokathor Lokathor left a comment

Choose a reason for hiding this comment

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

I'm gonna approve this now, and leave it up to you notgull on if you want to add more usage docs now or not. The alternative is we just merge it and we can always add more docs as a patch if people end up having specific questions.

@notgull
Copy link
Member Author

notgull commented Mar 29, 2023

I'd like for there to be more documentation, perhaps at the module level, about how a person writing a system lib should approach things (eg: winit/sdl2 folks), and how a person writing a graphics lib (eg: wgpu/ash folks) should approach things. I think the ideal is that people other than those two groups shouldn't have to look too carefully in here? Just pass a winit thing to a wgpu thing? So the system lib audience and graphics lib audience seem like our main users.

Sounds good! I've added some more documentation, with some specifically targeting winit/wgpu developers.

@Lokathor
Copy link
Contributor

I think for the doc links to work out approximately correctly you'll have to put in links to https://docs.rs/crate_name (or make them not links)

src/borrowed.rs Outdated Show resolved Hide resolved
src/borrowed.rs Outdated Show resolved Hide resolved
@Lokathor Lokathor merged commit 1e5f09d into master Mar 29, 2023
@notgull notgull deleted the notgull/change-actie branch March 30, 2023 00:19
@rib
Copy link

rib commented Apr 2, 2023

heya, just continuing to follow the trail of recent changes here...

Just because I see that Glutin was used to smoke test this change above - does that mean that you were seeing a problem with Glutin on Android before?

As far as I know, this Glutin example had been working for me on Android: https://github.com/rust-mobile/rust-android-examples/tree/main/na-winit-glutin (working across suspend / resume and cross-platform too)

The main consideration that was required for Android apps was that they need to re-create render surfaces when they resume. One portable way of handling this is that apps can drop their surface state when they Suspend (only ever happens on Android / iOS) and they can lazily create the render surfaces when they Resume (in Winit, all platforms get a Resume event).

I have to admit I'm currently a bit daunted by the new Active state tracking here and wondering about the implications of the RwLock that's involved in the Active tracking for Android. (without scrutinizing the details, I'm just initially worried about whether this could dead lock an event loop trying to suspend or resume)

I've had an initial look at the draft changes to use this in Glutin but I don't know that I entirely followed how the active status is handled (I just noticed that when window.window_handle() was being queried from Winit, then the code was expecting that it would be active I think:

let raw_window_handle =
        window.as_ref().map(|window| window.window_handle()).transpose().expect("Inactive");

and I saw that various places (like create_window_surface) would return an Err Result I think if a handle wasn't active but does that mean that this change is purely about avoiding accessing a handle while inactive? (i.e. it doesn't introduce special handling of the inactive case to try and hide that state from the app)

My current understanding is that if you want to write an application for Android based on this change then it's still your responsibility to only try and create a surface while 'active' and this doesn't try to make APIs such as Glutin automatically poll the active status and lazily [re]create surfaces (that's what I originally guessed this change might be aiming for).

If that's the case then I'm not currently sure if this change was required if there wasn't really a safety issue with accessing inactive ANativeWindows on Android?

As far as I know an ANativeWindow should remain safe to access so long as you hold a reference - it's just that the BufferQueue behind it isn't going to hand out new buffers for rendering once it's been "terminated".

Again, sorry for being late with asking questions here, since this has already been merged, but it feels like quite a significant change which I think is likely to affect my usage and I'm hoping to get a clearer understanding and also double check what's required here.

@notgull
Copy link
Member Author

notgull commented Apr 2, 2023

Just because I see that Glutin was used to smoke test this change above - does that mean that you were seeing a problem with Glutin on Android before?

No, it was mostly just to verify that nothing serious had been broken by my changes. I would've used softbuffer but it doesn't support Android yet, see rust-windowing/softbuffer#44

I have to admit I'm currently a bit daunted by the new Active state tracking here and wondering about the implications of the RwLock that's involved in the Active tracking for Android. (without scrutinizing the details, I'm just initially worried about whether this could dead lock an event loop trying to suspend or resume)

I agree, this is definitely the "monster in the closet" for the current implementation; it introduces a potential deadlock that wasn't there before (i.e. someone is holding a window handle while waiting for events, while the event loop is blocked waiting for that user to drop a window handle).

I think that the docs explicitly call out the fact that you shouldn't use a native window handle after onNativeWindowDestroyed:

You MUST ensure that you do not touch the window object after returning from this function

@MarijnS95
Copy link
Member

MarijnS95 commented Apr 2, 2023

@notgull let me know if you need a softbuffer implementation for Android, then I'll prioritize cleaning and merging the promised changes.

Aside that would also be nice to validate the glutin changes against wrapping Java Surface/SurfaceTexture objects in NativeWindow, and using those as "display surfaces" in glutin: https://github.com/MarijnS95/AndroidNativeSurface/blob/master/android_native_surface/src/lib.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants