We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

I should add that I haven't touched the definition of SDL_ANDROID_BLOCK_ON_PAUSE.

Minor update: Actually the crash was app-specific and not related to the patch...

Nevertheless, it's still not working as expected. So for now I'd just try the following simpler (but possibly more dangerous) approach: Pause via onPause and resume via onResume. I have a feeling some race condition (related to the OpenGL ES context) is possible, though.

Created attachment 1178 [details]
The patch in its most current form

Alright, eventually I've come up with something not that different from the first revision of the patch, only it's working well for me this time (when minimizing the app, as well as locking the screen). More tests are surely helpful, though.

Gabriel, can you look at this patch for the 2.0 release?

I think the solution will have to be a bit more involved. Here are the constraints as I see it:

* SDL is tied to the SurfaceView lifecycle (references : http://stackoverflow.com/questions/683136/android-illegalthreadstateexception-in-lunarlander http://stackoverflow.com/questions/4708198/flaw-in-the-lunar-lander-example-illegalthreadstateexception http://blorb.tumblr.com/post/236799414/simple-java-android-game-loop). In my opinion, we need to preserve that (doing otherwise may work in your system or mine, but there's enough reports that this is a problematic area, probably due to bugs in Android, that we should proceed with caution).

* SDL main thread runs separate from the Java thread that receives these events, and communication between them is asynchronous (done via events, and a couple of semaphores). See Java_org_libsdl_app_SDLActivity_nativePause and Java_org_libsdl_app_SDLActivity_nativeResume

* The event loop for Android doesn't block immediately, but can possibly keep going for a little while until SDL_WINDOWEVENT_FOCUS_LOST and SDL_WINDOWEVENT_MINIMIZED reach the app. See Android_PumpEvents.


I think it would be wise, in the interest of preserving the stability we seem to have achieved across different Android systems, to leave the code in surfaceDestroyed (and surfaceCreated, etc) intact. Also, we need to consider that after calling nativePause the app will keep going for a bit (also, nativePause and nativeResume are not "re entrant", I mean you can not call nativePause twice without calling nativeResume in the middle, else you'll probably get a deadlock).

Still, we need to add something to onPause, onResume to solve the screen locking problem, but this code needs to handle (probably via some flags such as mIsPausing, mIsResuming, etc) the various scenarios we may get:

* onPause and onResume can be called without surfaceCreated/surfaceDestroyed being called.
* onPause and onResume can be called before or after surfaceCreated/surfaceDestroyed are called.
* surfaceCreated/surfaceDestroyed can be called without onPause/onResume being called (maybe?)

How does this sound?

> * SDL is tied to the SurfaceView lifecycle (references :
> http://stackoverflow.com/questions/683136/android-
> illegalthreadstateexception-in-lunarlander
> http://stackoverflow.com/questions/4708198/flaw-in-the-lunar-lander-example-
> illegalthreadstateexception
> http://blorb.tumblr.com/post/236799414/simple-java-android-game-loop). In my
> opinion, we need to preserve that (doing otherwise may work in your system
> or mine, but there's enough reports that this is a problematic area,
> probably due to bugs in Android, that we should proceed with caution).

If I understand this correctly, it isn't a problem in our case since the secondary thread is created, and then started, only once (in startApp).

> (also,
> nativePause and nativeResume are not "re entrant", I mean you can not call
> nativePause twice without calling nativeResume in the middle, else you'll
> probably get a deadlock).

At the moment (in the current revision of the patch) mIsPaused should take care of that. There is also an occurence of mIsResumingFromPause, used in case onResume is called between calls to surfaceDestroyed and surfaceCreated (actually surfaceChanged).

> 
> Still, we need to add something to onPause, onResume to solve the screen
> locking problem, but this code needs to handle (probably via some flags such
> as mIsPausing, mIsResuming, etc) the various scenarios we may get:
> 
> * onPause and onResume can be called without surfaceCreated/surfaceDestroyed
> being called.
> * onPause and onResume can be called before or after
> surfaceCreated/surfaceDestroyed are called.
> * surfaceCreated/surfaceDestroyed can be called without onPause/onResume
> being called (maybe?)
> 
> How does this sound?

Yeah, anything you might not expect may occur. I guess we may assume, though, that any of the following occurs for now (although there is always some risk in doing so):
- Calls to onPause and onResume are interleaved, i.e. between two successive calls to onPause there will always be a call to onResume and vice-versa.
- Similarly, calls to surfaceCreated and surfaceDestroyed are interleaved.

What about the following approach:

- Never let surfaceChanged be involved in this (not sure this is good).
- surfaceCreated should create and start the secondary thread on the very first time it is called (currently done by startApp via surfaceChanged).
- Both surfaceDestroyed and onPause should try to initiate a pause: If mIsPaused is set to "false", change to "true" and signal the secondary thread. Otherwise, don't do so.
- Furthermore, both surfaceDestroyed and onPause should turn off the sensors if a valid surface is available.
- Similarly, both surfaceCreated and onResume should turn on the sensors if possible.
- onResume should check that mIsPaused is set to "true" *and* a valid surface is ready. If yes, change to "false" and signal the secondary thread. Otherwise, do nothing.
- In a similar manner, for any call to surfaceCreated other than the very first one, signal the secondary thread if mIsPaused is set to "true" and change the boolean's value to "false".
- Finally, should it be safer to keep another boolean for tracking the surface's state, rather than calling SurfaceView.getHolder().getSurface().isValid() as currently done in the patch?

Created attachment 1190 [details]
The patch in its most current form

Alright, an updated patch has been added.

A brief list of the changes, in comparison to *vanilla* SDL2:
- The startApp function (originally called by surfaceChanged) is gone. Its contents, with a few modifications, have been relocated to the body of surfaceCreated. They are applied only on the very first time surfaceCreated is called.
- Furthermore, the very first function call to surfaceCreated is responsible for enabling sensor input on its own.
- A new mIsSurfaceReady boolean tracking the surface's state has been added, initialized to "false" (surface is not ready).
- It is set to "true" during a call to surfaceCreated and to "false" during a call to surfaceDestroyed.
- A function named "handlePause" responsible for checking if a pause is desired. It can be called by onPause and surfaceDestroyed.
- Similarly, there is a function "handleResume" called by onResume. It is also called by surfaceCreated if it is not the very first time surfaceCreated is called.
- handlePause/handleResume does whatever is needed only if the mIsPaused boolean's value needs to be flipped *and* mIsSurfaceReady is set to "true".
- In such a case, handlePause/handleResume calls the respective native method (notifying the SDL main thread) and disables/enables sensor input. Furthermore, the value of mIsPaused is updated to the expected value.
- Finally, during a call to surfaceCreate, mIsSurfaceReady is set to "true" *before* handleResume is called (in case it does). Conversely, during a call to surfaceDestroy the value of mIsSurfaceReady is set to "false" *after* handlePause is called. Both of these are done on purpose, as the SDL main thread is notified only if mIsSurfaceReady is set to "true".

Maybe I should add the following: If one fears of applying great modifications to the surface* functions, then I guess that some of the code in the current patch can be relocated back to the original locations.

Basically, if I have not missed anything then it means that some (if not all) of the code the patch currently adds to surfaceCreated can be moved back to surfaceChanged, along with the optional restoration of the startApp function.

Any opinion about this?

This latest patch looks good to me.  Gabriel?

I'll test this and commit if everything looks ok. Thanks!

Thanks Gabriel!

Sorry about the delay, I haven't tested this yet, but I haven't forgotten about it!

Thanks for taking care of the issue anyway!

In case it helps, I am going to attach a minor variant of the patch, which - in comparison to the current patch - simply moves a few things from surfaceCreated to surfaceChanged (hence being a bit more similar to vanilla SDL HG). I have just come up with that and haven't tested this, but it's no more than an alternative to the current patch, in case it helps in any arbitrary way.

Created attachment 1201 [details]
(Not a replacement) A minor variant of the last patch (from June 14th).

Fixed in http://hg.libsdl.org/SDL/rev/1aadd7170248

I had to add a bit of logic to prevent proper shutdown/recreation of EGL surfaces as it was causing problems in my Nexus 4 when going to sleep.

Thanks!

Thanks for applying the patch! So far it seems to work as expected.

Interestingly, the little logic you have added has partially resolved the following bug I have also reported: https://bugzilla.libsdl.org/show_bug.cgi?id=1889