Fix Android crash on resume with Glow backend by Garoven · Pull Request #3867 · emilk/egui

Garoven · 2024-01-22T20:37:23Z

Addition for #3847
In previous one i only fixed crash occurring with Wgpu backend. This fixes crash with Glow backend as well.
I only tested this change with android so most things i changed are behind #[cfg(target_os = "android")].

Both fixes are dirty thought. As #3172 says that "The root viewport is the original viewport, and cannot be closed without closing the application.". So they break rules i guess? But i can't think about better solution for now.

Closes #3861.

MarijnS95

Just like with #3847, please don't put things behind cfgs that should function generically.

Garoven · 2024-01-22T21:55:14Z

I actually think that without these cfg's it should work on all platforms but it's need to be tested

the on_resume fn only updates viewport if it is present
the only way to remove root viewport is to suspend the app, which means on all other platform that app should exit so it isn't a problem

Will change and test it tomorrow. Changing to draft for now

MarijnS95 · 2024-01-22T22:19:24Z

Thanks! Can you also test if they can be simplfied/removed again on wgpu?

I'm by no means an expert on egui, let alone eframe. All I can tell you is that Resumed will always be called once. On mobile platforms, it might be followed by a Suspended, followed by a Resumed, etc. You can assume that they will always alternate like that.

Any state that is implicitly/indirectly owned by a surface should be destroyed in Suspended and recreated in Resumed. If we follow the documentation comments, that should be all state in the *WinitRunning structs:

egui/crates/eframe/src/native/wgpu_integration.rs

Lines 47 to 58 in aa67d31

    
           /// State that is initialized when the application is first starts running via 
        
           /// a Resumed event. On Android this ensures that any graphics state is only 
        
           /// initialized once the application has an associated `SurfaceView`. 
        
           struct WgpuWinitRunning { 
        
               integration: EpiIntegration, 
        
               /// The users application. 
        
               app: Box<dyn App>, 
        
               /// Wrapped in an `Rc<RefCell<…>>` so it can be re-entrantly shared via a weak-pointer. 
        
               shared: Rc<RefCell<SharedState>>, 
        
           }

egui/crates/eframe/src/native/glow_integration.rs

Lines 69 to 95 in aa67d31

    
           /// State that is initialized when the application is first starts running via 
        
           /// a Resumed event. On Android this ensures that any graphics state is only 
        
           /// initialized once the application has an associated `SurfaceView`. 
        
           struct GlowWinitRunning { 
        
               integration: EpiIntegration, 
        
               app: Box<dyn App>, 
        
               // These needs to be shared with the immediate viewport renderer, hence the Rc/Arc/RefCells: 
        
               glutin: Rc<RefCell<GlutinWindowContext>>, 
        
               // NOTE: one painter shared by all viewports. 
        
               painter: Rc<RefCell<egui_glow::Painter>>, 
        
           } 
        
           /// This struct will contain both persistent and temporary glutin state. 
        
           /// 
        
           /// Platform Quirks: 
        
           /// * Microsoft Windows: requires that we create a window before opengl context. 
        
           /// * Android: window and surface should be destroyed when we receive a suspend event. recreate on resume event. 
        
           /// 
        
           /// winit guarantees that we will get a Resumed event on startup on all platforms. 
        
           /// * Before Resumed event: `gl_config`, `gl_context` can be created at any time. on windows, a window must be created to get `gl_context`. 
        
           /// * Resumed: `gl_surface` will be created here. `window` will be re-created here for android. 
        
           /// * Suspended: on android, we drop window + surface.  on other platforms, we don't get Suspended event. 
        
           /// 
        
           /// The setup is divided between the `new` fn and `on_resume` fn. we can just assume that `on_resume` is a continuation of 
        
           /// `new` fn on all platforms. only on android, do we get multiple resumed events because app can be suspended.

While at it, can you remove this comment?:

egui/crates/eframe/src/native/glow_integration.rs

Lines 42 to 52 in aa67d31

    
           // Note: that the current Glutin API design tightly couples the GL context with 
        
           // the Window which means it's not practically possible to just destroy the 
        
           // window and re-create a new window while continuing to use the same GL context. 
        
           // 
        
           // For now this means it's not possible to support Android as well as we can with 
        
           // wgpu because we're basically forced to destroy and recreate _everything_ when 
        
           // the application suspends and resumes. 
        
           // 
        
           // There is work in progress to improve the Glutin API so it has a separate Surface 
        
           // API that would allow us to just destroy a Window/Surface when suspending, see: 
        
           // https://github.com/rust-windowing/glutin/pull/1435

It is no longer valid.

Finally, what about #3676?

Garoven · 2024-01-23T13:29:47Z

After digging deeper in codebase i have found that there is a way to fix this bug without dropping ROOT viewport. The same can be done with wgpu but don't know if i should do it in this pr.

m-hugo · 2024-01-23T14:37:09Z

works fine

MarijnS95 · 2024-01-24T12:39:46Z

i have found that there is a way to fix this bug without dropping ROOT viewport

I am not familiar enough with eframe to say, but viewports sound like a thing that should sustain a drawing surface disappearing (because the screen is temporarily blanked, or another app is in focus). In fact even the winit::Window should persist as it is currently not at all tied to the lifetime of an Android surface (and it shouldn't, it should be tied to an Activity, but everyone seems to wrongly assume that those are singletons). In winit it's just an empty shell that communicates with the fake Activity singleton.

Glad to have the code platform-agnostic. Can you retroactively apply the same to the wgpu backend?

Garoven · 2024-01-24T13:50:55Z

I have a branch with wgpu changed already. But it requires to make bigger changes. I had to move some code around to make borrow checker work. Now it looks more similar to glow structure so it should be better. But i think it better to make another pr that refreshes wgpu integration(move\clean code + add comments) to catch up with glow then change Suspend/Resume cycle.

MarijnS95 · 2024-01-24T13:55:24Z

@Garoven sounds good, looking forward to see that PR and I agree that the glow structure looks more like what platform-agnostic winit integrations should be.

Indeed such changes are better served in a separate PR to lighten the review/testing burden 👍

Garoven · 2024-01-24T14:02:15Z

Now I think that everything is good so it's time to re-open this pr.

emilk

That was a very pleasant diff!

Garoven added 2 commits January 22, 2024 19:14

fixed android(Glow) crash on resume

c41899b

cargo cranky

ae0ed9c

MarijnS95 suggested changes Jan 22, 2024

View reviewed changes

Garoven marked this pull request as draft January 22, 2024 21:55

changed overcomplicated fix to correct one

1a29a46

Garoven requested a review from MarijnS95 January 23, 2024 13:30

Garoven mentioned this pull request Jan 23, 2024

Glow/Android Only: Going directly from eframe to Home breaks the eframe on resume #3861

Closed

removed out of date comment

8cb6024

Garoven marked this pull request as ready for review January 24, 2024 14:02

emilk approved these changes Jan 24, 2024

View reviewed changes

emilk added eframe Relates to epi and eframe egui_glow Relates to running egui_glow on native android and removed egui_glow Relates to running egui_glow on native labels Jan 24, 2024

emilk merged commit 200051d into emilk:master Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Android crash on resume with Glow backend#3867

Fix Android crash on resume with Glow backend#3867
emilk merged 4 commits intoemilk:masterfrom
Garoven:android-glow-fix

Garoven commented Jan 22, 2024

Uh oh!

MarijnS95 left a comment

Uh oh!

Garoven commented Jan 22, 2024

Uh oh!

MarijnS95 commented Jan 22, 2024

Uh oh!

Garoven commented Jan 23, 2024

Uh oh!

m-hugo commented Jan 23, 2024

Uh oh!

MarijnS95 commented Jan 24, 2024

Uh oh!

Garoven commented Jan 24, 2024

Uh oh!

MarijnS95 commented Jan 24, 2024

Uh oh!

Garoven commented Jan 24, 2024

Uh oh!

emilk left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Garoven commented Jan 22, 2024

Uh oh!

MarijnS95 left a comment

Choose a reason for hiding this comment

Uh oh!

Garoven commented Jan 22, 2024

Uh oh!

MarijnS95 commented Jan 22, 2024

Uh oh!

Garoven commented Jan 23, 2024

Uh oh!

m-hugo commented Jan 23, 2024

Uh oh!

MarijnS95 commented Jan 24, 2024

Uh oh!

Garoven commented Jan 24, 2024

Uh oh!

MarijnS95 commented Jan 24, 2024

Uh oh!

Garoven commented Jan 24, 2024

Uh oh!

emilk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants