Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Android: refactor frame loop to use vsync #2142

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

smilediver
Copy link
Contributor

@smilediver smilediver commented Sep 13, 2024

Refactors Android's frame loop to use vsync.

Related other PR: #2125
Issue: #2129

It would be nice if more people tested this on different devices.

scheduleUpdate();
}

void MainLoopTest::update(float dt)
Copy link
Contributor

@solan-solan solan-solan Sep 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I do not clearly understand what about this fix. Does it help to get requested fps for high frequency devices?

Why such implementation of update? As I see:

  1. Remember now time with some Delta
  2. Wait while this Delta expiration nothing doing

You can use Node::schedule with selector for this purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fix the issue of requested animation interval not working on devices with high refresh rate displays. And it also fixes frames not starting on vsync.

The MainLoopTest::update() thing is the simplest thing to add simulation of slow frame updates.

@AlexandreK38
Copy link
Contributor

AlexandreK38 commented Sep 16, 2024

@smilediver Hi! Nice job on the PR

  • It works well on the Samsung S8 (60Hz)
  • I just tried on my Pixel 6 (120Hz) but it's not working as expected, it's like it is running at 30FPS instead of 60FPS (2x slower). I'm trying to look why it behaves like this. The sInterval is computed as expected with 16666667

Edit: I tried to look what the issue was but not found yet; in both P6 or S8 the interval between each call to renderNative() is about 20ms; so it seems fine. On the P6 the onVsync() returns once false, once true (so 1/2 so as expected too). If the native is called at the right frequency then it might be the UI refreshing the issue
Setting the interval to 1/120 on the P6 makes it run at "full" speed with about 10ms delay between each call to renderNative() and UI is refreshed as expected too.

  • Why did you use the JNI to set the Android representation time instead of calling it in Java directly (because of EGL14 and/or getCurrentDisplay issues?)

@smilediver
Copy link
Contributor Author

Can you profile and do a System Trace? You can use Main Loop test I have added to cpp-tests to play with animation interval or simulate slow update. For 120Hz and interval 1/60 you should get something like this (this is example of 60Hz with interval 1/30):

Screenshot 2024-09-16 at 15 35 27

Why did you use the JNI to set the Android representation time instead of calling it in Java directly (because of EGL14 and/or getCurrentDisplay issues?)

I found some worrying comments that it might not be a good idea to mix EGL10 (used by GLSurfaceView) and EGL14 required for eglPresentationTime(). It may be fine, but to be on the safer side I chose to do it on the native side.

@AlexandreK38
Copy link
Contributor

Can you profile and do a System Trace? You can use Main Loop test I have added to cpp-tests to play with animation interval or simulate slow update. For 120Hz and interval 1/60 you should get something like this (this is example of 60Hz with interval 1/30):

I couldn't compile cpp-tests yet but on my game it looks like this:
Screenshot 2024-09-16 at 15 55 05

I found some worrying comments that it might not be a good idea to mix EGL10 (used by GLSurfaceView) and EGL14 required for eglPresentationTime(). It may be fine, but to be on the safer side I chose to do it on the native side.

Yeah I think so

@smilediver
Copy link
Contributor Author

Strange... it updates and shows frames every other frame, just as it's supposed to do. Maybe try running the game on 120 and 60 Hz devices at the same time and see if there's any visual/feel difference between them?

@AlexandreK38
Copy link
Contributor

Strange... it updates and shows frames every other frame, just as it's supposed to do. Maybe try running the game on 120 and 60 Hz devices at the same time and see if there's any visual/feel difference between them?

It’s what I said earlier, when requesting 60Hz interval on the 120Hz device it’s like it runs at lower frequency than 60Hz, but I can’t explain why. The code seems fine to me that’s quite strange.
Also why do you add 2*mVsyncPeriod?

@smilediver
Copy link
Contributor Author

Also why do you add 2*mVsyncPeriod?

1*mVsyncPeriod would mean that the frame has to be finished and displayed at the next vsync, which means 60 FPS game would have less than 16ms for full update, render and all GPU work cycle to happen, including any delays from vsync. So it would be pretty hard to hit, unless the game is pretty trivial. 2*mVsyncPeriod delays it to the end of next frame, so that your game gets a full 16ms for update and render part.

Can you check what values frameStartTime and framePresentationTime get in AxmolRenderer.onDrawFrame()?

@AlexandreK38
Copy link
Contributor

AlexandreK38 commented Sep 16, 2024

Can you check what values frameStartTime and framePresentationTime get in AxmolRenderer.onDrawFrame()?

I’ll get few rows of values tomorrow but I’m sure mVsyncPeriod is 16666667

@smilediver
Copy link
Contributor Author

For 120Hz display mVsyncInterval should be somewhere around 8333333.

@AlexandreK38
Copy link
Contributor

AlexandreK38 commented Sep 17, 2024

For 120Hz display mVsyncInterval should be somewhere around 8333333.

This value made me tilt; I checked and I got 11111111 which was surprising. The reason is simple, I thought the Pixel 6 was 120Hz but it's 90Hz... My bad on this. But that's why we have this value and why we have an issue on the display with desync and lags sometimes as it's not a modulo of 60Hz. (Just as a note the Pixel 6 PRO is a 120Hz device which it's why it confused me).
Here are some values from onDrawFrame():

Pixel 6 (90Hz)
mVsyncInterval is 11111111

--
Interval 1/60 (16666667)
System.nanoTime,frameStartTime,framePresentationTime
28346540446742,28346539259946,28346561482168
28346562306402,28346561381596,28346583603818
28346584461594,28346583495555,28346605717777
28346606551030,28346605609332,28346627831554
28346628746913,28346627728735,28346649950957
28346650604701,28346649847413,28346672069635
28346672661830,28346671968155,28346694190377
28346694877121,28346694083374,28346716305596
28346717811000,28346716201313,28346738423535
28346739225672,28346738323843,28346760546065
28346761985682,28346760444048,28346782666270
28346783530116,28346782557095,28346804779317
--
Interval 1/30 (33333335)
30163283499468,30163282991375,30163305213597
30163316570554,30163316130875,30163338353097
30163350269528,30163349265992,30163371488214
30163382906247,30163382415329,30163404637551
30163416496579,30163415551173,30163437773395
30163450115028,30163448691197,30163470913419
30163483025713,30163481839646,30163504061868
30163515459877,30163514985546,30163537207768
30163548805295,30163548134175,30163570356397
30163581703285,30163581280209,30163603502431
30163615010739,30163614420178,30163636642400
30163648202227,30163647558323,30163669780545
30163681363197,30163680701447,30163702923669
--
Interval 1/90 (11111111)

32630885048280,32630884445647,32630906667869
32630896038433,32630895494878,32630917717100
32630907219423,32630906547163,32630928769385
32630918395814,32630917599042,32630939821264
32630929359722,32630928657248,32630950879470
32630940461610,32630939709581,32630961931803
32630951420595,32630950758842,32630972981064
32630962431296,32630961811641,32630984033863
32630973368756,32630972862944,32630995085166

--
Interval 1/120 (8333333) (Of course there are errors as 120HZ is greater than the device refresh rate)
29921462852092,29921462211244,29921484433466
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921473950196,29921473244766,29921495466988
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921485318767,29921484287430,29921506509652
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921496917441,29921495326130,29921517548352
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921507356324,29921506393832,29921528616054
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921518406943,29921517431403,29921539653625
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921529448162,29921528468210,29921550690432
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921540363242,29921539509145,29921561731367
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921551611410,29921550552528,29921572774750
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921562138836,29921561589510,29921583811732
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921574028403,29921572632059,29921594854281
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921584806235,29921583680162,29921605902384
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921595624024,29921594722142,29921616944364
eglPresentationTimeANDROIDImpl:2107 error 300d (EGL_BAD_SURFACE)
29921606640585,29921605757240,29921627979462

------------------
Samsung S8 (60Hz)
mVSyncInterval 16666665


Interval 1/60 (16666667)
390206579643,390205925280,390239258610
390223404873,390222586200,390255919530
390240340412,390239248820,390272582150
390256725027,390255909367,390289242697
390273321950,390272573687,390305907017
390290025489,390289234506,390322567836
390306808104,390305893335,390339226665
390323494066,390322554004,390355887334
390339969296,390339224571,390372557901
390356865258,390355885537,390389218867

@smilediver
Copy link
Contributor Author

I think that presentation time might be getting ahead of slow updates on fast displays disabling any buffering, so I'm thinking that more correct way to do it would be this:

long frameTime = Math.Max(sAnimationInterval.get(), mVsyncInterval);
long frameTimeWithVsync = frameTime + frameTime % mVsyncInterval;
long framePresentationTime = frameStartTime + frameTimeWithVsync + mVsyncInterval;

Basically frameTime is how long we expect one frame update to be, and then frameTimeWithVsync will extend it to the next vsync. So in normal situations you get frameTimeWithVsync == mVsyncInterval, and in requested lower fps you get sAnimationInterval extended to next vsync. Then framePresentationTime will add another mVsyncInterval to introduce one frame buffering. So we should get something like this:

sAnimationInterval 16666667 11111111 8333333
mVsyncInterval 11111111 11111111 11111111
frameTime 16666667 11111111 11111111
frameTimeVsync 22222223 11111111 11111111
frameStartTime 1000000000 1000000000 1000000000
framePresentationTime 1033333334 1022222222 1022222222

In theory this should help with jitters. Can you test this? I'll do some testing tomorrow too.

Also, on 90 Hz device you can't run at 60 FPS, as the closest you can get is 45 FPS, and that's why it feels that the game is running slower than 60 FPS. The only FPS' for 90 Hz you can run at are 90 / N, and in this case closest is N = 2. If you requested 40 FPS, your game would be running at 30 FPS, ie N = 3.

@AlexandreK38
Copy link
Contributor

@smilediver I tried and it works as expected so 45Hz on the Pixel 6 for 60Hz asked, works for 90Hz or 30Hz requested too.
Anyway the issue (for me at least) is that we ask for 60Hz but we have 45Hz. The way you have done is great but I’m not sure we can make assumption that it’s what everyone need. The simple solution or my solution thread tends towards 60Hz even if it’s not as precise as yours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants