Android: add command mecanism so that SDLActivity#14962
Conversation
|
still need comment: "We still need to pump events for lifecycle activity even if there's no window or video hasn't been initialized."
|
|
I'm marking this as draft until you have a chance to test it more thoroughly in production. @AntTheAlchemist, any chance you can try this patch in a production release and provide feedback? |
|
FWIW, right now there is only one window, but conceptually the pump call would get all life cycle events and events for all windows. |
1bsyl
force-pushed
the
br_android_RPC_v2
branch
from
769a432 to
de582b3
Compare
|
I remember I did a while ago a patch to have multiple SDL_Window on Android, there were several SurfaceView and several Activity involved. I don't remember how it worked though.. just that it didn't provide all the functionality of SDL. maybe I could dust it out and give a try. Btw, I am trying the patch in production. I need a one or two week to have more information. |
|
ok, so far that's ok. it's been 1 week, and 138k users. Production console says: Still some low crashes in EGL libs and ANR in nativePollOnce. but maybe not SDL related. 
|
|
MultiCraft Open Source is on the line 😀 |
|
@MoNTE48 nice, thanks for sharing. Let us know ! BTW: "the compare vs ALL Release" metric is not really relevant. because my SDL lib was a modified version. |
|
After 30K sessions, I can see that the ANR issue has been resolved. I can clearly see new ANRs with AppMetrica or the advertising library, but libSDL is no longer the cause. I didn't enable the GAMEPAD_AS_RPC build flag, but I'm not sure how many of my players use gamepads. Just one new, rare crash: |
|
Ok thanks for the info ! do you have information like how your crash rate or anr rate is now ?. I also have the ANativeWindow_setBuffersGeometry crash in SDL_CreateWindow, I have also some rare crash in: I believe it's possible that some ANR becomes Crash. or inverse. or change name. (Of course, if this is something reproducible, even with adding SDL_Delay() in code, this is has to be debugged...) |
|
looking at aosp code, setBuffersGeometry calls set_buffer_format, which doesn't check the handle validity. |
|
I'm late to the party - sorry guys. I'm now taking a proper look at this and report back with results from a production release. Currently, my gamepad heavy app stands at 0.02% crash rate and 0.18% ANR rate. Current common crashes (without this pull): Current common ANRs (without this pull): Watch this space. |
|
nice! |
|
@1bsyl what about |
1bsyl
force-pushed
the
br_android_RPC_v2
branch
from
36daee4 to
4359ae4
Compare
|
@MoNTE48 I adding this anyway |
So this is 3 weeks. This doesn't take into account : Android: check native_window is valid before calling ANativeWindow_setBuffersGeometry Android: add semaphores so that nativeSurface methods can wait up to going to try another test with the two previous commits |
1bsyl
force-pushed
the
br_android_RPC_v2
branch
from
4359ae4 to
224c331
Compare
|
So it's 1 week after starting 2nd test. that includes in addition:
Install base is the same as 1st test: ~130k users. So far, number of crash are reduced: 15 
edit: as difference with 1st test, I rolled out the 2nd test directly to 100%. (whereas I waited 1 day before doing that for the 1st one). |
|
This patch doesn't work for my apps. It seems to block all input events, which only come through when the app enters background. The 2nd time I leave the app and return, I get a black screen. Am I missing something? |
|
SDL_AppEvent isn't being called for input events. Is anyone else using the SDL_App* stuff? |
|
@AntTheAlchemist I didn't try SDL_AppEvent, but now it's ok with last commit. It also takes into account block on pause (very last commit separated). but there are more changes inside: all cycle event are moved with the RPC command flow, instead of being a separate channel. this is going to be a new test in a few weeks, once the v2 finished.
NB: |
|
Thanks for that. Seems to work, but I've found a problem. After a SDL_WINDOW_RESTORED, the first frame drawn produces a black screen. This used to be fixed with a pre-emptive duplicated call to SDL_RenderPresent(), but that no longer works. My app doesn't redraw every SDL_AppIterate. It sets a redraw and refresh flag after a SDL_WINDOW_RESTORED event is recieved, then in the following SDL_AppIterate call, no draw methods are valid. No other events are sent, so the app is juts a black screen. There's no way to fix this unless my app redraws every SDL_AppIterate. |
|
@AntTheAlchemist to be clear:
diff --git a/test/testsprite.c b/test/testsprite.c
index 575be1163..010a8b49c 100644
--- a/test/testsprite.c
+++ b/test/testsprite.c
@@ -79,6 +79,7 @@ static bool LoadSprite(const char *file)
return true;
}
+static int g_draw = 0; // or can start at 1 also.
static void MoveSprites(SDL_Renderer *renderer, SDL_Texture *sprite)
{
int i;
@@ -86,6 +87,10 @@ static void MoveSprites(SDL_Renderer *renderer, SDL_Texture *sprite)
SDL_FRect temp;
SDL_FRect *position, *velocity;
+ if (g_draw) {
+ SDL_Log("will draw...");
+ g_draw = 0;
+ } else { return; }
/* Query the sizes */
SDL_SetRenderViewport(renderer, NULL);
SDL_GetRenderSafeArea(renderer, &viewport);
@@ -565,6 +570,10 @@ SDL_AppResult SDL_AppEvent(void *appstate, SDL_Event *event)
if (event->type == SDL_EVENT_RENDER_DEVICE_RESET) {
LoadSprite(icon);
}
+ if (event->type == SDL_EVENT_WINDOW_RESTORED) {
+ SDL_Log("SDL_EVENT_WINDOW_RESTORED !!!");
+ g_draw = 1;
+ }
return SDLTest_CommonEventMainCallbacks(state, event);
}
here's simplified log, when going to FG |
|
I've just updated with latest main, because it had some non automatic merge. |
|
Updating again with latest main, because it had some non automatic merge. |
|
This is 3 week after 5th test. 
I see no new issue. For me this sounds ok. I'm moving to review ! |
|
Can you write a note for the merged commit that explains what is being done and why it's a good idea? |
- now SDLActivity defers most of SDL code internal execution to the Main thread. Since all code is now in Main thread, this prevents any race conditions within SDL code. - WatchEvents, including user ones, are now exected in SDL Main C thread This prevents race conditions that a user could add. - Simplify the locking/mutex internals between SDLActivity and Main. SDLActivity native code / synchronization is reduced to the minimum and fastest. - ordering between pause/resume and java surface create/changed/destroyed is kept. (prevent to do a "resume" before java surface isn't set up) - Fixed bug libsdl-org#11324 - Android opengles2: First SDL_RenderPresent() broken after resize Call ANativeWindow_setBuffersGeometry when size changes ( Android_NativeSurfaceResized ) NB: you can define SDL_ANDROID_GAMEPAD_AS_RPC in src/core/android/SDL_android.c so that the code that sends gamepad events is moved to main SDLThread (see libsdl-org#14925, libsdl-org#14962)
|
done ! |
|
@1bsyl, sorry for the dumb question, but is there any reason why |
|
@Packetdancer, can you please review this? |
that's a good point. ... maybe the readme/android to document this ? :/ |
- now SDLActivity defers most of SDL code internal execution to the Main thread. Since all code is now in Main thread, this prevents any race conditions within SDL code. - WatchEvents, including user ones, are now exected in SDL Main C thread This prevents race conditions that a user could add. - Simplify the locking/mutex internals between SDLActivity and Main. SDLActivity native code / synchronization is reduced to the minimum and fastest. - ordering between pause/resume and java surface create/changed/destroyed is kept. (prevent to do a "resume" before java surface isn't set up) - Fixed bug libsdl-org#11324 - Android opengles2: First SDL_RenderPresent() broken after resize Call ANativeWindow_setBuffersGeometry when size changes ( Android_NativeSurfaceResized ) NB: you can define SDL_ANDROID_GAMEPAD_AS_RPC in src/core/android/SDL_android.c so that the code that sends gamepad events is moved to main SDLThread (see libsdl-org#14925, libsdl-org#14962)
|
- now SDLActivity defers most of SDL code internal execution to the Main thread. Since all code is now in Main thread, this prevents any race conditions within SDL code. - WatchEvents, including user ones, are now exected in SDL Main C thread This prevents race conditions that a user could add. - Simplify the locking/mutex internals between SDLActivity and Main. SDLActivity native code / synchronization is reduced to the minimum and fastest. - ordering between pause/resume and java surface create/changed/destroyed is kept. (prevent to do a "resume" before java surface isn't set up) - Fixed bug libsdl-org#11324 - Android opengles2: First SDL_RenderPresent() broken after resize Call ANativeWindow_setBuffersGeometry when size changes ( Android_NativeSurfaceResized ) NB: you can define SDL_ANDROID_GAMEPAD_AS_RPC in src/core/android/SDL_android.c so that the code that sends gamepad events is moved to main SDLThread (see libsdl-org#14925, libsdl-org#14962)
|
- now SDLActivity defers most of SDL code internal execution to the Main thread. Since all code is now in Main thread, this prevents any race conditions within SDL code. - WatchEvents, including user ones, are now exected in SDL Main C thread This prevents race conditions that a user could add. - Simplify the locking/mutex internals between SDLActivity and Main. SDLActivity native code / synchronization is reduced to the minimum and fastest. - ordering between pause/resume and java surface create/changed/destroyed is kept. (prevent to do a "resume" before java surface isn't set up) - Fixed bug libsdl-org#11324 - Android opengles2: First SDL_RenderPresent() broken after resize Call ANativeWindow_setBuffersGeometry when size changes ( Android_NativeSurfaceResized ) NB: you can define SDL_ANDROID_GAMEPAD_AS_RPC in src/core/android/SDL_android.c so that the code that sends gamepad events is moved to main SDLThread (see libsdl-org#14925, libsdl-org#14962)
|
|
||
| But if you want the internal gamepad events be executed mostly in the Main SDL Thread, | ||
| (for instance, if you want your Gamepad WatchEvent attached be threadsafe by being executed | ||
| in main C thread), you can enable the define SDL_ANDROID_GAMEPAD_AS_RPC |
There was a problem hiding this comment.
I don't know if we actually want this. Other platforms can have gamepad events coming in off the main thread. We should probably document that and remove this.
|
Could we make this work like SDL_HINT_JOYSTICK_THREAD does? |
|
or this can be the other way: |
No, we can't introduce performance issues by default. Gamepad latency is critical for games. |
|
then, I can just remove the documentation part, so that this keeps things simple ?? |
- now SDLActivity defers most of SDL code internal execution to the Main thread. Since all code is now in Main thread, this prevents any race conditions within SDL code. - WatchEvents, including user ones, are now exected in SDL Main C thread This prevents race conditions that a user could add. - Simplify the locking/mutex internals between SDLActivity and Main. SDLActivity native code / synchronization is reduced to the minimum and fastest. - ordering between pause/resume and java surface create/changed/destroyed is kept. (prevent to do a "resume" before java surface isn't set up) - Fixed bug libsdl-org#11324 - Android opengles2: First SDL_RenderPresent() broken after resize Call ANativeWindow_setBuffersGeometry when size changes ( Android_NativeSurfaceResized ) NB: you can define SDL_ANDROID_GAMEPAD_AS_RPC in src/core/android/SDL_android.c so that the code that sends gamepad events is moved to main SDLThread (see libsdl-org#14925, libsdl-org#14962)
- now SDLActivity defers most of SDL code internal execution to the Main thread. Since all code is now in Main thread, this prevents any race conditions within SDL code. - WatchEvents, including user ones, are now exected in SDL Main C thread This prevents race conditions that a user could add. - Simplify the locking/mutex internals between SDLActivity and Main. SDLActivity native code / synchronization is reduced to the minimum and fastest. - ordering between pause/resume and java surface create/changed/destroyed is kept. (prevent to do a "resume" before java surface isn't set up) - Fixed bug libsdl-org#11324 - Android opengles2: First SDL_RenderPresent() broken after resize Call ANativeWindow_setBuffersGeometry when size changes ( Android_NativeSurfaceResized ) NB: you can define SDL_ANDROID_GAMEPAD_AS_RPC in src/core/android/SDL_android.c so that the code that sends gamepad events is moved to main SDLThread (see libsdl-org#14925, libsdl-org#14962)
|
ok, the documentation is removed ! the variable is kept internal |
6 participants
Add some RPC commands so that SDLActivity defers most of SDL code internal execution it to the SDL main thread
see #14925
Most SDLActivity code is deferred to main SDL C thread with some command RPC.
except:
(SDL_Window *)Android_Window global variable is removed.
Android ActivityMutex is removed.
Android_LifecycleMutex, is used to lock the SDLActivity state. (Android_WaitActiveAndLockActivity()).
since it prevents the SDLActivity to add new lifecylce message, it prevents it to change state.
factorisation of EGLSurface/native_window handling. (use in surfaceChanged/Created/Destroy, and SDLCreate/DestroyWindow()
It seem to work quite nicely from my side. I tested various stuff, but not fully.
(onNativeFileDialog() was the less easy to update, and I really have to tested).
other functions are always simple. I guess, the ways it's written with named struct, prevent lot of type mistake.
Still a draft. please give a try.
I'll try it also in production maybe in a while
maybe 2 todo:
I didn't add the HID native functions .. I am not familiar with them. but then didn't rely on SDL and use no locking.
so maybe it makes not difference. (they could be added anyway afterwards).
we probably lose some event timestamp precision because event are sent from Main thread, and not SDLActivity thread. maybe it's not a big deal.
but if we need it for some command, we could add. (just enabled the timestamp, and then send it with the event).
maybe we wait to see, if this is really useful