Skip to content

Commit 8db0781

Browse files
fix: address fps limiter review bugs + add unit tests
Bug fixes: - PresentExtension: nextScheduledUst now uses max(last+interval, now) so already-late frames are not penalised with an extra full interval of delay - PresentExtension: setFrameRateLimit increments a generation counter so in-flight scheduled lambdas detect the change and fire immediately instead of stalling the game at the old cadence - PresentExtension: lastScheduledUst watermark is reset on every limit change (not just when going to 0) so the new cadence starts clean - PresentExtension: add close() / Extension.close() default so the scheduler thread is shut down when the XServer is released (no more thread leak per game session) - XServerView: setFrameRateLimit now cancels any pending delayed render and re-kicks requestRender() on any change, not just when going to 0 — fixes stale render cadence when changing e.g. 5fps -> 60fps - XServerScreen: frameRating.update() moved to runOnUiThread() — was being called from the GL thread with no synchronisation - XServerScreen: LaunchedEffect uses applyFpsLimiterToEngines() helper to remove duplicated set calls Refactor: - Extract fpsLimiterSteps/CurrentIndex/Progress/Next/Previous to FpsLimiterUtils.kt (internal) so they are testable without Compose - fpsLimiterCurrentIndex uses indexOfLast { it <= value } floor lookup instead of exact-match, fixing wrong direction on a restored non-step value Tests (FpsLimiterUtilsTest — 20 cases): - fpsLimiterSteps: 60/90/120/144Hz, exact multiples, sub-5 clamp, ordering - fpsLimiterCurrentIndex: exact match, floor, below-min clamp, 144Hz edge - nextFpsLimiterValue: normal, clamped at max, non-step floor, 144Hz top - previousFpsLimiterValue: normal, clamped at min, non-step floor, 144Hz top - fpsLimiterProgress: min=0, max=1, midpoint, 144Hz, non-step floor
1 parent 8c90c40 commit 8db0781

7 files changed

Lines changed: 268 additions & 55 deletions

File tree

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
package app.gamenative.ui.component
2+
3+
internal fun fpsLimiterSteps(maxFps: Int): List<Int> {
4+
val sanitizedMax = maxFps.coerceAtLeast(5)
5+
val flooredMax = (sanitizedMax / 5) * 5
6+
return buildList {
7+
var value = 5
8+
while (value <= flooredMax) {
9+
add(value)
10+
value += 5
11+
}
12+
if (sanitizedMax != flooredMax) add(sanitizedMax)
13+
}
14+
}
15+
16+
/**
17+
* Returns the index of the step that is the floor of [currentValue] — i.e. the highest
18+
* step that is still ≤ currentValue. Falls back to 0 if currentValue is below the
19+
* first step, so navigation never goes in the wrong direction on a restored value that
20+
* isn't an exact multiple of 5.
21+
*/
22+
internal fun fpsLimiterCurrentIndex(steps: List<Int>, currentValue: Int): Int =
23+
steps.indexOfLast { it <= currentValue }.coerceAtLeast(0)
24+
25+
internal fun fpsLimiterProgress(currentValue: Int, maxFps: Int): Float {
26+
val steps = fpsLimiterSteps(maxFps)
27+
val currentIndex = fpsLimiterCurrentIndex(steps, currentValue)
28+
return if (steps.lastIndex <= 0) 1f else currentIndex.toFloat() / steps.lastIndex.toFloat()
29+
}
30+
31+
internal fun nextFpsLimiterValue(currentValue: Int, maxFps: Int): Int {
32+
val steps = fpsLimiterSteps(maxFps)
33+
val currentIndex = fpsLimiterCurrentIndex(steps, currentValue)
34+
return steps[(currentIndex + 1).coerceAtMost(steps.lastIndex)]
35+
}
36+
37+
internal fun previousFpsLimiterValue(currentValue: Int, maxFps: Int): Int {
38+
val steps = fpsLimiterSteps(maxFps)
39+
val currentIndex = fpsLimiterCurrentIndex(steps, currentValue)
40+
return steps[(currentIndex - 1).coerceAtLeast(0)]
41+
}

app/src/main/java/app/gamenative/ui/component/QuickMenu.kt

Lines changed: 2 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -212,36 +212,8 @@ private fun matchesPerformanceHudPreset(
212212
currentConfig.showGpuUsageGraph == presetConfig.showGpuUsageGraph
213213
}
214214

215-
private fun fpsLimiterSteps(maxFps: Int): List<Int> {
216-
val sanitizedMax = maxFps.coerceAtLeast(5)
217-
val flooredMax = (sanitizedMax / 5) * 5
218-
return buildList {
219-
var value = 5
220-
while (value <= flooredMax) {
221-
add(value)
222-
value += 5
223-
}
224-
if (sanitizedMax != flooredMax) add(sanitizedMax)
225-
}
226-
}
227-
228-
private fun fpsLimiterProgress(currentValue: Int, maxFps: Int): Float {
229-
val steps = fpsLimiterSteps(maxFps)
230-
val currentIndex = steps.indexOfFirst { it == currentValue }.takeIf { it >= 0 } ?: (steps.lastIndex)
231-
return if (steps.lastIndex <= 0) 1f else currentIndex.toFloat() / steps.lastIndex.toFloat()
232-
}
233-
234-
private fun nextFpsLimiterValue(currentValue: Int, maxFps: Int): Int {
235-
val steps = fpsLimiterSteps(maxFps)
236-
val currentIndex = steps.indexOfFirst { it == currentValue }.takeIf { it >= 0 } ?: steps.lastIndex
237-
return steps[(currentIndex + 1).coerceAtMost(steps.lastIndex)]
238-
}
239-
240-
private fun previousFpsLimiterValue(currentValue: Int, maxFps: Int): Int {
241-
val steps = fpsLimiterSteps(maxFps)
242-
val currentIndex = steps.indexOfFirst { it == currentValue }.takeIf { it >= 0 } ?: steps.lastIndex
243-
return steps[(currentIndex - 1).coerceAtLeast(0)]
244-
}
215+
// fpsLimiterSteps / fpsLimiterCurrentIndex / fpsLimiterProgress /
216+
// nextFpsLimiterValue / previousFpsLimiterValue live in FpsLimiterUtils.kt
245217

246218
@Composable
247219
fun QuickMenu(

app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -499,10 +499,7 @@ fun XServerScreen(
499499
fpsLimiterTarget = clampedTarget
500500
}
501501
val appliedLimit = if (fpsLimiterEnabled) clampedTarget else 0
502-
xServerView?.setFrameRateLimit(appliedLimit)
503-
xServerView?.getxServer()
504-
?.getExtension<PresentExtension>(PresentExtension.MAJOR_OPCODE.toInt())
505-
?.setFrameRateLimit(appliedLimit)
502+
applyFpsLimiterToEngines(appliedLimit)
506503
}
507504

508505
fun restorePerformanceHudPosition() {
@@ -1426,7 +1423,9 @@ fun XServerScreen(
14261423
renderer.isCursorVisible = false
14271424
renderer.setOnFrameRenderedListener {
14281425
if (shouldTrackDisplayedFrames.get()) {
1429-
frameRating?.update()
1426+
(context as? Activity)?.runOnUiThread {
1427+
frameRating?.update()
1428+
}
14301429
}
14311430
}
14321431
getxServer().renderer = renderer
@@ -1920,6 +1919,9 @@ fun XServerScreen(
19201919
// Remove the WindowManager listener associated with the released AndroidView.
19211920
binding.xServerView.renderer.setOnFrameRenderedListener(null)
19221921
binding.xServerView.getxServer().windowManager.removeOnWindowModificationListener(binding.windowModificationListener)
1922+
binding.xServerView.getxServer()
1923+
.getExtension<PresentExtension>(PresentExtension.MAJOR_OPCODE.toInt())
1924+
?.close()
19231925
if (PluviaApp.xServerView === binding.xServerView) {
19241926
PluviaApp.xServerView = null
19251927
}

app/src/main/java/com/winlator/widget/XServerView.java

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,22 +77,23 @@ public int getFrameRateLimit() {
7777
}
7878

7979
public void setFrameRateLimit(int frameRateLimit) {
80-
final boolean shouldKickRender;
80+
final boolean shouldRecomputeRender;
8181
synchronized (renderThrottleLock) {
82+
boolean hadScheduledRender = renderRequestScheduled;
8283
this.frameRateLimit = Math.max(0, frameRateLimit);
8384
minRenderIntervalMs = this.frameRateLimit > 0
8485
? Math.max(1L, Math.round(1000f / (float) this.frameRateLimit))
8586
: 0L;
8687

87-
if (this.frameRateLimit == 0 && renderRequestScheduled) {
88+
if (hadScheduledRender) {
8889
renderThrottleHandler.removeCallbacks(throttledRenderRunnable);
8990
renderRequestScheduled = false;
9091
}
91-
shouldKickRender = this.frameRateLimit == 0;
92+
shouldRecomputeRender = this.frameRateLimit == 0 || hadScheduledRender;
9293
}
9394

94-
if (shouldKickRender) {
95-
super.requestRender();
95+
if (shouldRecomputeRender) {
96+
requestRender();
9697
}
9798
}
9899

app/src/main/java/com/winlator/xserver/extensions/Extension.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,4 +17,6 @@ public interface Extension {
1717
byte getFirstEventId();
1818

1919
void handleRequest(XClient client, XInputStream inputStream, XOutputStream outputStream) throws IOException, XRequestError;
20+
21+
default void close() {}
2022
}

app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import java.util.concurrent.Executors;
3232
import java.util.concurrent.ScheduledExecutorService;
3333
import java.util.concurrent.TimeUnit;
34+
import java.util.concurrent.atomic.AtomicInteger;
3435

3536
public class PresentExtension implements Extension {
3637
public static final byte MAJOR_OPCODE = -103;
@@ -47,6 +48,9 @@ public enum Mode {COPY, FLIP, SKIP}
4748
private volatile long targetIntervalUs = 0L;
4849
private long lastScheduledUst = 0L; // guarded by scheduleLock
4950
private final Object scheduleLock = new Object();
51+
// Incremented on every setFrameRateLimit call so in-flight lambdas can detect
52+
// that the limit changed and fire immediately instead of stalling the game.
53+
private final AtomicInteger limitGeneration = new AtomicInteger(0);
5054
private final ScheduledExecutorService presentScheduler =
5155
Executors.newSingleThreadScheduledExecutor(r -> {
5256
Thread t = new Thread(r, "PresentExt-FpsLimiter");
@@ -55,22 +59,23 @@ public enum Mode {COPY, FLIP, SKIP}
5559
});
5660

5761
public void setFrameRateLimit(int limit) {
58-
frameRateLimit = Math.max(0, limit);
59-
if (frameRateLimit > 0) {
60-
targetIntervalUs = 1_000_000L / frameRateLimit;
61-
} else {
62-
targetIntervalUs = 0L;
63-
synchronized (scheduleLock) {
64-
lastScheduledUst = 0L;
65-
}
62+
synchronized (scheduleLock) {
63+
frameRateLimit = Math.max(0, limit);
64+
targetIntervalUs = frameRateLimit > 0 ? 1_000_000L / frameRateLimit : 0L;
65+
lastScheduledUst = 0L; // reset pacing watermark on every change
6666
}
67+
limitGeneration.incrementAndGet(); // invalidate any in-flight scheduled notifies
68+
}
69+
70+
public void close() {
71+
presentScheduler.shutdownNow();
6772
}
6873

69-
/** Returns the UST (microseconds) at which this present should be signalled,
70-
* advancing the internal watermark by targetIntervalUs each call. */
7174
private long nextScheduledUst(long nowUst) {
7275
synchronized (scheduleLock) {
73-
long next = Math.max(nowUst, lastScheduledUst) + targetIntervalUs;
76+
// Use the later of (last watermark + interval) or now so that already-late
77+
// frames are not penalised by an extra full interval of delay.
78+
long next = Math.max(lastScheduledUst + targetIntervalUs, nowUst);
7479
lastScheduledUst = next;
7580
return next;
7681
}
@@ -200,12 +205,22 @@ private void presentPixmap(XClient client, XInputStream inputStream, XOutputStre
200205
final int finalIdleFence = idleFence;
201206
final long finalScheduledUst = scheduledUst;
202207
final long finalInterval = targetInterval;
208+
final int capturedGen = limitGeneration.get();
203209
presentScheduler.schedule(() -> {
204210
try {
205-
long msc = finalScheduledUst / finalInterval;
206-
sendIdleNotify(finalWindow, finalPixmap, finalSerial, finalIdleFence);
207-
sendCompleteNotify(finalWindow, finalSerial, Kind.PIXMAP, Mode.COPY,
208-
finalScheduledUst, msc);
211+
if (limitGeneration.get() == capturedGen) {
212+
long msc = finalScheduledUst / finalInterval;
213+
sendIdleNotify(finalWindow, finalPixmap, finalSerial, finalIdleFence);
214+
sendCompleteNotify(finalWindow, finalSerial, Kind.PIXMAP, Mode.COPY,
215+
finalScheduledUst, msc);
216+
} else {
217+
// Limit changed while this frame was queued — fire immediately
218+
// so the game is not stalled at the old cadence.
219+
long ustNow = System.nanoTime() / 1000;
220+
sendIdleNotify(finalWindow, finalPixmap, finalSerial, finalIdleFence);
221+
sendCompleteNotify(finalWindow, finalSerial, Kind.PIXMAP, Mode.COPY,
222+
ustNow, ustNow / FAKE_INTERVAL_DEFAULT_US);
223+
}
209224
} catch (Exception ignored) {
210225
// Client may have disconnected before the scheduled notify fired.
211226
}

0 commit comments

Comments
 (0)