Android : Fixing rendering for all FPS (> 60 or < 60) - Thread to trigger rendering manually

[AlexandreK38]

Describe your changes

Hi!
I found a rendering issue again on a Pixel 6 with 120FPS rendering, and in fact I realized I made such changes on my old Cocos2dx, and forgot to add it in axmol when we switched few months ago.

So, this PR is a fix so the Android rendering is called following the interval we give as expected, and not 60.
The surface view is now using RENDERMODE_WHEN_DIRTY rendering mode and not RENDERMODE_CONTINUOUS so the onDrawFrame() is called only when we ask the surface view we need to update the UI (using requestRender()). A thread is used to check the intervals so it works for any FPS (lower like 30FPS or greater like 120FPS).
Why a thread? We could have computed the intervals in the renderer class itself like it is now, but unfortunately sleeping the thread that is rendering is not precise.

[rh101]

The changes in this PR overwrite some changes made in PR #1942, which was made to fix certain issues. Please ensure that the changes made here do not break the fixes that were introduced in #1942.

[AlexandreK38]

The changes in this PR overwrite some changes made in PR #1942, which was made to fix certain issues. Please ensure that the changes made here do not break the fixes that were introduced in #1942.

Yeah I’ll have a closer look and change 🙏

[AlexandreK38]

@rh101 I think it's working as before now. I'm pausing the thread when the activity onPause() is called too as you were pausing the rendering too by switching the rendering mode.
To be sure, if you or anyone else could also test this - especially with the onPause() - we could be sure it's working as it was before it would be great.

[rh101]

I'm pausing the thread when the activity onPause() is called too as you were pausing the rendering too by switching the rendering mode.

Perhaps I am misunderstanding what you mean by this, but that is not how the implementation worked (after the changes in #1942). If the application activity loses focus, then the GLSurfaceView (rendering) should not be paused. The changes made in PR #1942 simply switched it to using RENDERMODE_WHEN_DIRTY. The rendering should only be paused when the application is put into the background (such as on Activity.onStop()), so that is when GLSurfaceView.onPause() is called.

The reason for this is that calling GLSurfaceView.onPause() can trigger an EGL context loss, and if the activity simply loses focus, but not yet put into the background, then that is not ideal. That is why the renderer should only be paused on Activity.onStop() instead (as recommended in the Android documentation).

You easily test this by finding this function call, setPreserveEGLContextOnPause(true);, and changing it to setPreserveEGLContextOnPause(false);. This will force a context loss when the render is paused, so if the application loses focus, and the context is not lost, then your changes are fine, but if the context is lost, then the changes are not correct.

The other to test will be to ensure applicationWillEnterForeground() is not called initial app start-up.

To be sure, if you or anyone else could also test this - especially with the onPause() - we could be sure it's working as it was before it would be great.

I'll be able to test it in 1-2 days time.

[AlexandreK38]

I'm pausing the thread when the activity onPause() is called too as you were pausing the rendering too by switching the rendering mode.

Perhaps I am misunderstanding what you mean by this, but that is not how the implementation worked (after the changes in #1942). If the application activity loses focus, then the GLSurfaceView (rendering) should not be paused. The changes made in PR #1942 simply switched it to using RENDERMODE_WHEN_DIRTY. The rendering should only be paused when the application is put into the background (such as on Activity.onStop()), so that is when GLSurfaceView.onPause() is called.

Yeah I think you misunderstood, but it's my fault too as it wasn't clear enough. Let me explain:
With the current implementation, when Activity.onPause() is called the renderer is not stopped but the UI won't be updated because the renderer mode is switched from RENDERMODE_CONTINUOUSLY to RENDERMODE_WHEN_DIRTY. In details, RENDERMODE_CONTINUOUSLY tells to call onDrawFrame() on the renderer no matter what aka in continue whereas RENDERMODE_WHEN_DIRTY needs us to ask the surface view to update explicitly (using requestRender()) and trigger onDrawFrame(). As you don't ask, nothing is updated as expected after the pause.

My changes are based to always use RENDERMODE_WHEN_DIRTY, and have an "inner" thread (private static class RendererThread extends Thread) to manage when to ask to update and avoid Thread.sleep() calls. This is the thread I was referring to, not the renderer itself. It's this thread that I need to pause on Activity.onPause / onStop (using wait()) so we don't ask the surface view to update and call onDrawFrame(); and to resume (using notify()) on Activity.onResume so the surface view needs to update and call onDrawFrame() again.

The reason for this is that calling GLSurfaceView.onPause() can trigger an EGL context loss, and if the activity simply loses focus, but not yet put into the background, then that is not ideal. That is why the renderer should only be paused on Activity.onStop() instead (as recommended in the Android documentation).

You easily test this by finding this function call, setPreserveEGLContextOnPause(true);, and changing it to setPreserveEGLContextOnPause(false);. This will force a context loss when the render is paused, so if the application loses focus, and the context is not lost, then your changes are fine, but if the context is lost, then the changes are not correct.

I'll try this

The other to test will be to ensure applicationWillEnterForeground() is not called initial app start-up.

I'll ensure about this too but as far as I understood and coded, your logic is not changed by my PR, only the way to trigger the surface view update calls.

To be sure, if you or anyone else could also test this - especially with the onPause() - we could be sure it's working as it was before it would be great.

I'll be able to test it in 1-2 days time.

Nice :)

[smilediver]

It looks like that thread adds 100% CPU usage for one core, or I'm missing something?

What is the actual problem you are having?

[AlexandreK38]

It looks like that thread adds 100% CPU usage for one core, or I'm missing something?

It always runs unless paused yes, but it just calls a check so nothing heavy to me and not using 100% from what I saw, but yeah we should sleep it for the interval.

What is the actual problem you are having?

The actual rendering is not working well with 120FPS (and low FPS too), the native render is called twice and so the update(float dt) in C++. If we ask for 1.f/60.f as interval, we want these called at this value too. The first idea was to compute the sleep correctly but it wasn't working well.

[smilediver]

I don't think this is a good solution overall. Adding another thread to an already complicated problem of syncing will make it even more complicated.

Maybe we should create an issue for this, do some research and have a discussion to find the best way to implement this.

[AlexandreK38]

To me it’s not more complicated than before once you know how it works

• on a side you have Java calling the draw frame at the frequency it wants and you try to sleep the rendering thread itself but you can’t manage correctly low or high fps devices
• on the other side you make your own thread to trigger yourself when the frame should be rendered, working for any fps and you don’t sleep the renderer itself after drawing a frame.

Anyway I still need to test what @rh101 suggested before to ensure it’s working as before

[smilediver]

I'm talking about complexity of understanding behavior and what the consequences of the code are, not what the code does.

[rh101]

@AlexandreK38 Using the changes in this PR is causing the application to freeze, and attempting to pause it in the debugger to see where it is stuck just shows it in a wait state. The only way to recover is to put the app into the background, then bring it back to the foreground, and suddenly it recovers and starts responding again.

This does not happen with the existing Axmol code, so it seems like something in this PR is causing the issue. Perhaps it is something to do with this section:

long wait = (AxmolRenderer.sAnimationInterval - (System.nanoTime()-now)) / AxmolRenderer.NANOSECONDSPERMICROSECOND;
if (wait > 0) {
    sleep(wait);
}

Is there any way wait can end up as a very large value?

Aside from this, I have to agree with @smilediver. Please log an issue, and explain the problem you are having (the reason for this PR) in as much detail as possible.

[thomascastel]

Alexandre is going to submit a fix for stuck issue (there was indeed a problem with the thread)
Also we have several games with heavy rendering, we compared both solution (Thread and no thread) it seems to be smoother with the separate thread

[rh101]

Alexandre is going to submit a fix for stuck issue (there was indeed a problem with the thread)

Excellent!

Also we have several games with heavy rendering, we compared both solution (Thread and no thread) it seems to be smoother with the separate thread

I did notice that the performance is smoother, but given that this change will impact all applications targeting Android, it does need to be tested as thoroughly as possible.

[AlexandreK38]

Alexandre is going to submit a fix for stuck issue (there was indeed a problem with the thread)

Excellent!

We identified that the first freeze you had could come from a deadlock, so please try again if it works on your device. On my Pixel 6 and Samsung S8 I don't see any freeze. I'll test on several (heavy) games we have.

Is there any way wait can end up as a very large value?

No it's the interval at most

Also we have several games with heavy rendering, we compared both solution (Thread and no thread) it seems to be smoother with the separate thread

I did notice that the performance is smoother, but given that this change will impact all applications targeting Android, it does need to be tested as thoroughly as possible.

The current implementation use sleep() in the onDrawFrame() causing the java thread to sleep and it has a little impact. In my solution it's my thread that is sleeping not the java thread and I think it's why it's smoother.

This is clearly a big impact and it definitely needs several devices to test, any help is welcome of course 🙏

Also I opened the issue #2129 (not sure I explained the best I could) as suggested and posted the simple fix I had at first in it.

[AlexandreK38]

For everyone interested in this PR we discuss the potential other solutions in the issue #2129, dont hesitate to have a look and give your advice.