feat: in-game quick menu fps cap #1081
Open
xXJSONDeruloXx wants to merge 14 commits intoutkarshdalal:masterfrom
Open
feat: in-game quick menu fps cap #1081xXJSONDeruloXx wants to merge 14 commits intoutkarshdalal:masterfrom
xXJSONDeruloXx wants to merge 14 commits intoutkarshdalal:masterfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an end-to-end FPS limiter: UI controls in QuickMenu, Compose state and wiring in XServerScreen, renderer post-render hook in GLRenderer, render throttling in XServerView, delayed present notify scheduling in PresentExtension, FPS helper utils and tests, plus localization strings. (34 words) Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as QuickMenu
participant Screen as XServerScreen
participant View as XServerView
participant Renderer as GLRenderer
participant Present as PresentExtension
User->>UI: Toggle/Adjust FPS limiter
UI->>Screen: onFpsLimiterEnabledChanged / onFpsLimiterChanged
Screen->>View: setFrameRateLimit(target or 0)
Screen->>Present: setFrameRateLimit(target or 0)
loop Render cycle
User->>View: events trigger requestRender()
View->>View: requestRender() (throttled/coalesced)
View->>Renderer: super.requestRender()
Renderer->>Renderer: onDrawFrame()
Renderer->>Renderer: run onFrameRenderedListener
Renderer->>Present: frame-rendered callback
Present->>Present: compute/schedule notify (maybe delayed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
xXJSONDeruloXx
changed the title
phobos665
reviewed
Apr 2, 2026
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java">
<violation number="1" location="app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java:73">
P1: FPS limiter incorrectly delays already-late frames, causing output FPS to drop below the configured cap.</violation>
<violation number="2" location="app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java:203">
P2: Disabling the FPS clamp doesn’t cancel already scheduled Present* notifies, so the limiter can keep stalling frames after the user toggles it off. Track and cancel pending ScheduledFutures (or reschedule immediately) when limit becomes 0 so “off” takes effect right away.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java
Outdated
Show resolved
Hide resolved
phobos665
reviewed
Apr 2, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
494-506: Consider reusingapplyFpsLimiterToEnginesto reduce duplication.Lines 502-505 duplicate the logic from
applyFpsLimiterToEngines(lines 475-478). Since thisLaunchedEffectruns afterxServerViewis set, the helper function should work correctly.♻️ Suggested refactor
LaunchedEffect(xServerView) { val detectedMax = detectMaxRefreshRateHz(context, xServerView) detectedMaxRefreshRateHz = detectedMax val clampedTarget = fpsLimiterTarget.coerceAtMost(detectedMax).coerceAtLeast(5) if (clampedTarget != fpsLimiterTarget) { fpsLimiterTarget = clampedTarget } - val appliedLimit = if (fpsLimiterEnabled) clampedTarget else 0 - xServerView?.setFrameRateLimit(appliedLimit) - xServerView?.getxServer() - ?.getExtension<PresentExtension>(PresentExtension.MAJOR_OPCODE.toInt()) - ?.setFrameRateLimit(appliedLimit) + applyFpsLimiterToEngines(if (fpsLimiterEnabled) clampedTarget else 0) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around lines 494 - 506, The LaunchedEffect duplicates the frame-rate application logic found in applyFpsLimiterToEngines; after computing detectedMaxRefreshRateHz and adjusting fpsLimiterTarget you should remove the duplicated block that calls xServerView?.setFrameRateLimit(...) and extension.setFrameRateLimit(...) and instead call the existing helper applyFpsLimiterToEngines (using xServerView and the current fpsLimiterEnabled/fpsLimiterTarget state as needed) so the same centralized logic is reused; keep detectMaxRefreshRateHz(context, xServerView) and the clamping/assignment of fpsLimiterTarget, then invoke applyFpsLimiterToEngines to apply the limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/component/QuickMenu.kt`:
- Around line 229-245: Normalize the input value to a valid step before using
the helpers: call fpsLimiterSteps(maxFps) and compute a normalized index (e.g.
find the index of the exact match or the indexOfLast { it <= currentValue } and
clamp it into 0..steps.lastIndex) and then use that index for
fpsLimiterProgress, nextFpsLimiterValue and previousFpsLimiterValue instead of
falling back to steps.lastIndex; reference the functions fpsLimiterProgress,
nextFpsLimiterValue, previousFpsLimiterValue and fpsLimiterSteps when applying
this change.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1481-1489: The call to frameRating?.update() is executed off the
UI thread causing a data race on non-volatile fields (e.g., lastTime,
frameCount, lastFPS)—wrap both places where update() is invoked (the renderer's
onFrameRenderedListener and the window-manager callback where
frameRatingWindowId is handled) inside (context as? Activity)?.runOnUiThread {
frameRating?.update() } so update() always runs on the UI thread, matching how
visibility changes are already posted and preventing concurrent reads in run()
from racing with update().
In `@app/src/main/java/com/winlator/renderer/GLRenderer.java`:
- Line 36: The onFrameRenderedListener field is accessed from multiple threads
(written by setOnFrameRenderedListener() and read in onDrawFrame()), so make
onFrameRenderedListener volatile to ensure visibility and atomicity, and in
onDrawFrame() take a local snapshot (e.g., Runnable listener =
onFrameRenderedListener) before checking for null and invoking it to avoid a
race where the field becomes null between the check and call; update the field
declaration and adjust onDrawFrame() to use the local variable while leaving
setOnFrameRenderedListener() unchanged.
In `@app/src/main/java/com/winlator/widget/XServerView.java`:
- Around line 79-97: When frameRateLimit changes in setFrameRateLimit(),
recompute any pending delayed render rather than only cancelling when new limit
is 0: inside the synchronized(renderThrottleLock) block after recalculating
frameRateLimit and minRenderIntervalMs, if renderRequestScheduled is true then
remove the existing throttledRenderRunnable from renderThrottleHandler and
re-post it with the newly computed minRenderIntervalMs (or cancel if
frameRateLimit==0) so the next render cadence reflects the new limit
immediately; update references to renderRequestScheduled,
throttledRenderRunnable, renderThrottleHandler, minRenderIntervalMs and ensure
requestRender() behavior remains consistent with the new scheduling.
In `@app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java`:
- Around line 57-67: setFrameRateLimit currently only clears lastScheduledUst
when disabling and updates targetIntervalUs, leaving already-queued tasks in
presentScheduler firing at the old cadence; modify setFrameRateLimit (and the
similar blocks at the other locations) to reset queued pacing state by:
acquiring scheduleLock, updating targetIntervalUs and frameRateLimit, clearing
or zeroing lastScheduledUst, and notifying/interrupting the presentScheduler so
any already-submitted lambdas recompute their next run using the new
targetIntervalUs (or cancel and reschedule them) instead of using stale timing;
reference methods/fields setFrameRateLimit, targetIntervalUs, lastScheduledUst,
scheduleLock, and presentScheduler when implementing.
- Around line 50-55: PresentExtension creates a dedicated
ScheduledExecutorService field presentScheduler but there is no lifecycle hook
to shut it down; add a close/destroy method to the Extension contract (e.g., add
void destroy() or close()) and implement it in PresentExtension to call
presentScheduler.shutdownNow() (or shutdown() + awaitTermination with timeout)
to stop the "PresentExt-FpsLimiter" thread, then ensure the application calls
the new destroy/close method when an XServer instance is torn down so threads
are not leaked.
In `@app/src/main/res/values/strings.xml`:
- Line 269: Update the string resource performance_hud_fps_limiter_description
to reflect that the feature throttles the game's actual frame/present loop, not
just the overlay; change the wording from “Caps the displayed frame rate while
in-game. Max: %1$d Hz.” to something like “Caps the in-game frame rate while
in-game. Max: %1$d Hz.” preserving the %1$d placeholder and punctuation.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 494-506: The LaunchedEffect duplicates the frame-rate application
logic found in applyFpsLimiterToEngines; after computing
detectedMaxRefreshRateHz and adjusting fpsLimiterTarget you should remove the
duplicated block that calls xServerView?.setFrameRateLimit(...) and
extension.setFrameRateLimit(...) and instead call the existing helper
applyFpsLimiterToEngines (using xServerView and the current
fpsLimiterEnabled/fpsLimiterTarget state as needed) so the same centralized
logic is reused; keep detectMaxRefreshRateHz(context, xServerView) and the
clamping/assignment of fpsLimiterTarget, then invoke applyFpsLimiterToEngines to
apply the limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd31396c-f059-4832-8863-3df86cd7b307
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/ui/component/QuickMenu.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/renderer/GLRenderer.javaapp/src/main/java/com/winlator/widget/XServerView.javaapp/src/main/java/com/winlator/xserver/extensions/PresentExtension.javaapp/src/main/res/values/strings.xml
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/component/QuickMenu.kt (1)
228-244:⚠️ Potential issue | 🟡 MinorNormalize
fpsLimiterTargetbefore using step indices.These helpers still assume
currentValueis already one of the generated steps.app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktonly range-clamps the target, so a restored or externally provided value like47can still reach this code. In that case thelastIndexfallback makes the bar render as 100%, andpreviouscan even jump upward to55. Compute the nearest valid index first and reuse it here.🛠️ Suggested fix
+private fun fpsLimiterCurrentIndex(steps: List<Int>, currentValue: Int): Int { + return steps.indexOfLast { it <= currentValue } + .takeIf { it >= 0 } + ?: 0 +} + private fun fpsLimiterProgress(currentValue: Int, maxFps: Int): Float { val steps = fpsLimiterSteps(maxFps) - val currentIndex = steps.indexOfFirst { it == currentValue }.takeIf { it >= 0 } ?: (steps.lastIndex) + val currentIndex = fpsLimiterCurrentIndex(steps, currentValue) return if (steps.lastIndex <= 0) 1f else currentIndex.toFloat() / steps.lastIndex.toFloat() } private fun nextFpsLimiterValue(currentValue: Int, maxFps: Int): Int { val steps = fpsLimiterSteps(maxFps) - val currentIndex = steps.indexOfFirst { it == currentValue }.takeIf { it >= 0 } ?: steps.lastIndex + val currentIndex = fpsLimiterCurrentIndex(steps, currentValue) return steps[(currentIndex + 1).coerceAtMost(steps.lastIndex)] } private fun previousFpsLimiterValue(currentValue: Int, maxFps: Int): Int { val steps = fpsLimiterSteps(maxFps) - val currentIndex = steps.indexOfFirst { it == currentValue }.takeIf { it >= 0 } ?: steps.lastIndex + val currentIndex = fpsLimiterCurrentIndex(steps, currentValue) return steps[(currentIndex - 1).coerceAtLeast(0)] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/component/QuickMenu.kt` around lines 228 - 244, The three helpers fpsLimiterProgress, nextFpsLimiterValue, and previousFpsLimiterValue assume currentValue is exactly one of fpsLimiterSteps; instead compute the nearest step index up front by finding the index of the closest step to currentValue (using fpsLimiterSteps(maxFps)), use that normalized index for progress calculation and to pick next/previous entries, and avoid falling back to lastIndex erroneously — i.e., replace the current index lookup with a nearest-index computation and reuse that in all three functions so an out-of-range or non-step value (e.g., 47) maps to the nearest valid step rather than forcing lastIndex or jumping incorrectly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/component/QuickMenu.kt`:
- Around line 228-244: The three helpers fpsLimiterProgress,
nextFpsLimiterValue, and previousFpsLimiterValue assume currentValue is exactly
one of fpsLimiterSteps; instead compute the nearest step index up front by
finding the index of the closest step to currentValue (using
fpsLimiterSteps(maxFps)), use that normalized index for progress calculation and
to pick next/previous entries, and avoid falling back to lastIndex erroneously —
i.e., replace the current index lookup with a nearest-index computation and
reuse that in all three functions so an out-of-range or non-step value (e.g.,
47) maps to the nearest valid step rather than forcing lastIndex or jumping
incorrectly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a4d644b-bc4a-463e-b8ba-8f5cf06ca5f5
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/component/QuickMenu.kt
xXJSONDeruloXx
force-pushed
the
feat/quick-menu-fps-clamp
branch
from
8db0781 to
53465f5
Compare
Contributor
There was a problem hiding this comment.
3 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:1924">
P1: Do not unconditionally close `PresentExtension` in `onRelease`; this view can be recreated while reusing the same XServer, so closing the shared extension can break FPS limiting after recreation.</violation>
</file>
<file name="app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java">
<violation number="1" location="app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java:67">
P2: FPS limit updates do not take effect immediately for already queued frame notifications because scheduled tasks are not canceled/rescheduled.</violation>
<violation number="2" location="app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java:211">
P1: Generation check is insufficient to detect all limit-change races; a queued frame can still be emitted on a stale interval after the FPS limit changes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
1479-1485:⚠️ Potential issue | 🟠 Major
FrameRating.update()is still off the UI thread here.The renderer callback was fixed, but Line 1484 still calls
frameRating?.update()directly fromWindowManager.OnWindowModificationListener. That listener is not UI-thread confined, so this reintroduces the same race onFrameRating’s mutable fields.Suggested fix
shouldTrackDisplayedFrames.set(true) (context as? Activity)?.runOnUiThread { frameRating?.visibility = View.VISIBLE + frameRating?.update() } - frameRating?.update() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around lines 1479 - 1485, The call to frameRating?.update() is happening off the UI thread inside the WindowManager.OnWindowModificationListener, causing races on FrameRating's mutable fields; change it to run on the UI thread (use the same pattern as the runOnUiThread block used for setting visibility) so that frameRating?.update() is invoked inside Activity.runOnUiThread (or equivalent) after setting frameRatingWindowId and shouldTrackDisplayedFrames.set(true), ensuring all FrameRating mutations occur on the UI thread.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1922-1924: The onRelease handler in XServerScreen is closing the
PresentExtension
(binding.xServerView.getxServer().getExtension<PresentExtension>(...)?
.close()), which prematurely shuts down the shared scheduler when the
AndroidView is temporarily released; remove the .close() call from this
view-lifecycle onRelease and instead invoke PresentExtension.close() only from
the real X server/environment teardown path where existingXServer is actually
disposed (the same place you handle full XServer shutdown or cleanup), ensuring
getExtension<PresentExtension>() is closed during actual XServer teardown rather
than during view recreation.
- Around line 217-221: The current return chain on refreshRate unnecessarily
forces any finite positive refreshRate up to at least 30 via .coerceAtLeast(30),
which inflates real 24/25 Hz panels; remove the .coerceAtLeast(30) so the result
uses roundToInt() (and falls back to DEFAULT_FPS_LIMITER_MAX_HZ when invalid)
and let the existing limiter/helpers enforce the 5 FPS floor; update the
expression using the refreshRate variable and DEFAULT_FPS_LIMITER_MAX_HZ
accordingly (i.e., remove the coerceAtLeast(30) call in the
takeIf/.../roundToInt() chain).
In `@app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java`:
- Around line 196-223: The MSC (msc) is computed using the current
targetInterval, which can change via limitGeneration and cause msc to go
backwards; instead compute MSC from a stable interval so it remains monotonic
across live cap changes. Modify the scheduled task and the immediate branch in
PresentExtension so sendCompleteNotify(...) uses an msc derived from a
consistent reference interval (e.g. compute and capture a stableInterval before
scheduling or use FAKE_INTERVAL_DEFAULT_US/constants) rather than targetInterval
at the time of notification; keep the existing limitGeneration capture and
fallback branch but compute msc using the captured/stable interval (and for the
"limit changed" branch compute using the same stable basis so MSC only ever
increases).
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1479-1485: The call to frameRating?.update() is happening off the
UI thread inside the WindowManager.OnWindowModificationListener, causing races
on FrameRating's mutable fields; change it to run on the UI thread (use the same
pattern as the runOnUiThread block used for setting visibility) so that
frameRating?.update() is invoked inside Activity.runOnUiThread (or equivalent)
after setting frameRatingWindowId and shouldTrackDisplayedFrames.set(true),
ensuring all FrameRating mutations occur on the UI thread.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7332edf7-a65d-486a-b66c-9032e4b66add
📒 Files selected for processing (7)
app/src/main/java/app/gamenative/ui/component/FpsLimiterUtils.ktapp/src/main/java/app/gamenative/ui/component/QuickMenu.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/widget/XServerView.javaapp/src/main/java/com/winlator/xserver/extensions/Extension.javaapp/src/main/java/com/winlator/xserver/extensions/PresentExtension.javaapp/src/test/java/app/gamenative/ui/component/FpsLimiterUtilsTest.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/winlator/xserver/extensions/Extension.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/ui/component/QuickMenu.kt
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java`:
- Around line 193-209: The bug is that capturedGen is read after calling
nextScheduledUst(nowUst), so a cap change between those calls can make the
scheduled timing stale; to fix, read limitGeneration.get() (the generation)
before computing nextScheduledUst(nowUst) or compute both under the same
lock/atomic section so the generation and scheduledUst are derived from the same
state; update the code around nextScheduledUst(...), capturedGen, and the lambda
passed to presentScheduler.schedule(...) so capturedGen always reflects the
state used to compute finalScheduledUst/finalInterval.
- Around line 209-227: The scheduled lambda in PresentExtension (the
presentScheduler block that calls sendIdleNotify/sendCompleteNotify) can race
with request-thread writes because XClient.sendEvent writes to XOutputStream
without the stream lock; change the notify path to use a thread-safe send API on
XClient instead of calling the non-synchronized sendEvent directly: add or use a
synchronized method on XClient (e.g., sendEventThreadSafe/sendEventLocked or
enqueueEvent) that acquires the XOutputStream lock (or the client's send lock)
before writing, update sendIdleNotify and sendCompleteNotify to call that
thread-safe method, and ensure the scheduled lambda uses this new safe send path
and preserves the existing exception handling for disconnected clients.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f425e056-f5fd-445a-8709-aba29974b784
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/xserver/extensions/PresentExtension.java
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
…ps-clamp # Conflicts: # app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
xXJSONDeruloXx
force-pushed
the
feat/quick-menu-fps-clamp
branch
from
d6ac999 to
42c860c
Compare
Owner
|
How does this interact with the existing default DXVK_FRAME_RATE variable? Does it make sense to remove DXVK_FRAME_RATE from the default env vars? And clamp games to 60 fps here by default? |
Contributor
Author
|
@utkarshdalal yes DXVK launch arg will be the ceiling, and this limiter can lower from there. I left the removal of that var out of this scope but could roll it in if you like |
… back-pressure The previous implementation only throttled XServerView.requestRender() which controls Android GL compositing, not the game's actual render loop. Games are paced by PresentIdleNotify/PresentCompleteNotify events from the X Present extension - since these fired immediately, games rendered at full speed regardless. Fix: PresentExtension.presentPixmap() now schedules IdleNotify/CompleteNotify at the target frame interval using a monotonic watermark (nextScheduledUst). Pixels are still copied immediately so the drawable stays current, but the completion signals that unblock the game's render loop are delayed to match the configured FPS limit. This creates real back-pressure that reduces CPU/GPU usage. Also wires setFrameRateLimit() through to PresentExtension in both applyFpsLimiter() and the LaunchedEffect(xServerView) reattach path.
- FPS limiter is now a toggle at the TOP of the HUD tab (above Perf HUD toggle). When OFF: game renders unlimited, slider is hidden. When ON: target slider appears via AnimatedVisibility. Toggling OFF and back ON restores the last-set target (e.g. 25 FPS stays 25). - Slider steps are 5, 10, 15, … up to display max Hz (no more arbitrary 15 floor or the confusing 0/Unlimited end-step — on/off is the toggle's job now). - State split: fpsLimiterEnabled (toggle) + fpsLimiterTarget (last target, default 30). applyFpsLimiterEnabled() / applyFpsLimiterTarget() replace the old applyFpsLimiter(). Both push the resolved limit (0 when disabled) to XServerView and PresentExtension.
Contributor
There was a problem hiding this comment.
4 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java">
<violation number="1" location="app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java:67">
P2: FPS-limit changes do not take effect immediately for already queued presents because stale scheduled tasks are not canceled and generation is checked only at execution time.</violation>
<violation number="2" location="app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java:207">
P2: The microsecond-to-millisecond truncation (`delayUs / 1_000L` with `TimeUnit.MILLISECONDS`) consistently under-delays each frame, making the actual FPS cap slightly higher than intended. Use `TimeUnit.MICROSECONDS` directly to preserve the precision you already computed.</violation>
<violation number="3" location="app/src/main/java/com/winlator/xserver/extensions/PresentExtension.java:222">
P2: Catching bare `Exception` and ignoring it silently can hide programming errors. Consider narrowing to `IOException` for the expected disconnection case, or at minimum logging unexpected exception types at a warning level so they don't vanish silently.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/component/FpsLimiterUtils.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/component/FpsLimiterUtils.kt:7">
P2: FPS step generation starts at 5 instead of 15, so the limiter can expose unintended 5/10 FPS options.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:222">
P2: Legacy DXVK FPS cap is not migrated/cleared, so disabling the new quick-menu limiter may still leave FPS capped via `DXVK_FRAME_RATE`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Open
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
added an in game fps clamp which properly frame limits the running game in real time without requiring a game restart.
this properly reduces CPU and GPU usage as well as you can see in the demo video, so you can throttle the FPS and get more predictable frame pacing, especially for games without their own limits, or without having to restart the game every time you want to make a tweak.
also, made this reflect the way steam deck does it, so we have a global toggle, then a remembered setting for the clamp, when restored, if was 25 fps, will then be 25 on re-toggle. toggling off removes the clamp entirely.
(I was considering removing the dxvd var in container settings that defaults to lock 60, but decided against it, may follow up with that one later)
This is a big QoL feature especially for those without active cooling. and for games that dont need a lot of motion, dialing down to 5-10 fps for things like visual novels will really help battery and heat
Recording
https://youtu.be/4w3rTZXZvY0
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Adds an in-game FPS limiter in the Quick Menu that caps the game’s frame rate live to reduce CPU/GPU load and smooth pacing. Uses Present back-pressure plus
XServerViewthrottling, applies instantly, clamps to the detected max display refresh, and persists per-container (respects legacyDXVK_FRAME_RATEif set).New Features
DisplayManager/attached display; limit is applied to bothXServerViewandPresentExtensionfor real pacing.DXVK_FRAME_RATEwhen present and persists per-container state (enabled by default unless a legacy DXVK cap exists).Bug Fixes
PresentExtensionnow delays Idle/Complete notifications to back-pressure the game’s render loop; fixed MSC monotonicity and X stream locking.DXVK_FRAME_RATE=60from container env to avoid unintended caps.Written for commit 44db81d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests