This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• Early patch prototype (currently results in an immediate crash) (sdl_android_screen_lock_fix.diff, text/plain, 2013-06-06 08:28:15 +0000, 2725 bytes)
• The patch in its most current form (sdl_android_screen_lock_fix.diff, text/plain, 2013-06-06 14:21:36 +0000, 3352 bytes)
• The patch in its most current form (sdl_android_screen_lock_fix_20130614.diff, text/plain, 2013-06-14 08:08:29 +0000, 4639 bytes)
• (Not a replacement) A minor variant of the last patch (from June 14th). (sdl_android_screen_lock_alt_fix_20130701.diff, text/plain, 2013-07-01 10:34:26 +0000, 4734 bytes)

Reported in version: HG 2.0
Reported for operating system, platform: Android (All), All

Comments on the original bug report:

On 2013-06-06 08:28:15 +0000, wrote:

Created attachment 1177
Early patch prototype (currently results in an immediate crash)

A few more details: From the moment the screen is locked and up to when it gets unlocked, the surface (on the Java side) is not destroyed nor recreated. Furthermore, depending on the device's orientation and application's orientation setup, there are situations where it does not resize, either.

Furthermore, onPause and onResume are called on the right moments, but they do nothing. In practice it means that the C/C++ thread is still running in the background, having a clear impact on battery life.

If someone wants that, I have had an early attempt at a patch, but it fails on application startup for now. There is also the issue of OpenGL ES context recreation mentioned in SDL_androidevents.c, so there may be some more things I don't see at the moment.

On 2013-06-06 08:29:31 +0000, wrote:

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

On 2013-06-06 12:15:06 +0000, wrote:

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.

On 2013-06-06 14:21:36 +0000, wrote:

Created attachment 1178
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.

On 2013-06-13 01:19:41 +0000, Sam Lantinga wrote:

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

On 2013-06-13 09:26:59 +0000, Gabriel Jacobo wrote:

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?

On 2013-06-13 15:39:30 +0000, wrote:

• 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.

On 2013-06-13 15:51:16 +0000, wrote:

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?

On 2013-06-14 08:08:29 +0000, wrote:

Created attachment 1190
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".

On 2013-06-18 12:17:47 +0000, wrote:

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?

On 2013-06-19 04:23:43 +0000, Sam Lantinga wrote:

This latest patch looks good to me. Gabriel?

On 2013-06-21 11:31:22 +0000, Gabriel Jacobo wrote:

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

On 2013-06-23 10:52:41 +0000, Sam Lantinga wrote:

Thanks Gabriel!

On 2013-07-01 08:34:56 +0000, Gabriel Jacobo wrote:

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

On 2013-07-01 10:32:07 +0000, wrote:

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.

On 2013-07-01 10:34:26 +0000, wrote:

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

On 2013-07-06 14:35:04 +0000, Gabriel Jacobo wrote:

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!

On 2013-07-07 10:38:50 +0000, wrote:

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