asr: add setFrameRate function#1612
Open
AndreVto wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds per-surface frame rate control to ChangesASurfaceRenderer per-surface frame rate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
on some devices it affects what refresh rate the display is supposed to use with this app
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/cpp/asurfacerenderer/ASurfaceRendererContext.cpp`:
- Around line 412-419: The code has thread safety issues where pending state
variables (pendingFrameRate, pendingCompatibility, pendingChangeStrategy) are
read in the oneShot lambda without synchronization while being written
elsewhere, and the sc SurfaceControl pointer snapshots are used after the lock
is released while concurrent operations may release these handles. Fix this by
extending the mutex lock to cover all access to these pending state variables
and ensuring the sc pointer snapshots and their usage in the SurfaceTransaction
remain protected under the same mutex throughout, preventing both data races on
the pending variables and use-after-free on the SurfaceControl handles.
In `@app/src/main/java/com/winlator/renderer/ASurfaceRenderer.java`:
- Around line 651-653: The frame-rate request made before surface initialization
in the conditional check (line 651) is silently dropped, and there is no
mechanism to replay it once the surface is created. Cache the frameRate,
compatibility, and changeStrategy parameters as instance variables whenever they
are set, and then replay the cached values by calling nativeSetFrameRate with
the stored parameters in the onSurfaceCreated method after surfaceInitialized is
set to true.
🪄 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: d1f07993-b67f-4b19-9314-b0fe45a3e976
⛔ Files ignored due to path filters (1)
app/src/main/jniLibs/arm64-v8a/libasurface_renderer.sois excluded by!**/*.so
📒 Files selected for processing (5)
app/src/main/cpp/asurfacerenderer/ASurfaceRendererContext.cppapp/src/main/cpp/asurfacerenderer/ASurfaceRendererContext.happ/src/main/cpp/asurfacerenderer/asurface_jni.cppapp/src/main/java/com/winlator/renderer/ASurfaceRenderer.javaapp/src/main/java/com/winlator/widget/XServerView.java
Comment on lines
+412
to
+419
| if (pendingFrameRate != 0.f && (fnSTSetFrameRate || fnSTSetFrameRateWithChangeStrategy)) { | ||
| oneShot([&](void* tx) { | ||
| if (fnSTSetFrameRateWithChangeStrategy) | ||
| ST_SETFRAMERATE_CS(tx, sc, pendingFrameRate, pendingCompatibility, pendingChangeStrategy); | ||
| else | ||
| ST_SETFRAMERATE(tx, sc, pendingFrameRate, pendingCompatibility); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Strengthen lock coverage for pending state and SurfaceControl handle lifetime.
Line 455–457 writes pendingFrameRate/pendingCompatibility/pendingChangeStrategy without synchronization, while Line 412–417 reads them on another path. Also, Line 459–465 snapshots raw sc pointers and Line 469–474 uses them after unlock, while concurrent registration/unregistration can release handles (e.g., Line 426). This can cause data races and use-after-free.
🔧 Suggested fix (serialize state + handle usage under the same mutex)
void ASurfaceRendererContext::setFrameRate(float frameRate,
int8_t compatibility, int8_t changeStrategy)
{
if (!fnSTSetFrameRate && !fnSTSetFrameRateWithChangeStrategy) return;
-
- pendingFrameRate = frameRate;
- pendingCompatibility = compatibility;
- pendingChangeStrategy = changeStrategy;
-
- std::vector<void*> scs;
- {
- std::lock_guard<std::mutex> lk(windowScMutex);
- scs.reserve(windowScMap.size());
- for (auto& [id, sc] : windowScMap)
- scs.push_back(sc);
- }
- if (scs.empty()) return;
-
- void* tx = ST_CREATE();
- for (void* sc : scs) {
- if (fnSTSetFrameRateWithChangeStrategy)
- ST_SETFRAMERATE_CS(tx, sc, frameRate, compatibility, changeStrategy);
- else
- ST_SETFRAMERATE(tx, sc, frameRate, compatibility);
- }
- ST_APPLY(tx);
- ST_DELETE(tx);
+ oneShot([&](void* tx) {
+ std::lock_guard<std::mutex> lk(windowScMutex);
+ pendingFrameRate = frameRate;
+ pendingCompatibility = compatibility;
+ pendingChangeStrategy = changeStrategy;
+ for (auto& [id, sc] : windowScMap) {
+ if (fnSTSetFrameRateWithChangeStrategy)
+ ST_SETFRAMERATE_CS(tx, sc, frameRate, compatibility, changeStrategy);
+ else
+ ST_SETFRAMERATE(tx, sc, frameRate, compatibility);
+ }
+ });
}Also applies to: 455-475
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/cpp/asurfacerenderer/ASurfaceRendererContext.cpp` around lines
412 - 419, The code has thread safety issues where pending state variables
(pendingFrameRate, pendingCompatibility, pendingChangeStrategy) are read in the
oneShot lambda without synchronization while being written elsewhere, and the sc
SurfaceControl pointer snapshots are used after the lock is released while
concurrent operations may release these handles. Fix this by extending the mutex
lock to cover all access to these pending state variables and ensuring the sc
pointer snapshots and their usage in the SurfaceTransaction remain protected
under the same mutex throughout, preventing both data races on the pending
variables and use-after-free on the SurfaceControl handles.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 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/renderer/ASurfaceRenderer.java">
<violation number="1" location="app/src/main/java/com/winlator/renderer/ASurfaceRenderer.java:649">
P2: Frame-rate request is silently dropped when the surface is not initialized; no pending state is stored for later application.</violation>
<violation number="2" location="app/src/main/java/com/winlator/renderer/ASurfaceRenderer.java:649">
P2: Unchecked int-to-byte narrowing may corrupt frame-rate mode parameters passed to native layer</violation>
</file>
<file name="app/src/main/cpp/asurfacerenderer/ASurfaceRendererContext.cpp">
<violation number="1" location="app/src/main/cpp/asurfacerenderer/ASurfaceRendererContext.cpp:412">
P1: Unsynchronized cross-thread access to pending frame-rate state causes a C++ data race and potentially inconsistent tuples applied to new windows.</violation>
<violation number="2" location="app/src/main/cpp/asurfacerenderer/ASurfaceRendererContext.cpp:455">
P2: No validation of frameRate, compatibility, or changeStrategy before persisting and applying them globally to all surfaces. Negative or NaN frame rates and invalid enum values pass through to the Android compositor.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| windowScMap[contentId] = sc; | ||
| } | ||
|
|
||
| if (pendingFrameRate != 0.f && (fnSTSetFrameRate || fnSTSetFrameRateWithChangeStrategy)) { |
Contributor
There was a problem hiding this comment.
P1: Unsynchronized cross-thread access to pending frame-rate state causes a C++ data race and potentially inconsistent tuples applied to new windows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/cpp/asurfacerenderer/ASurfaceRendererContext.cpp, line 412:
<comment>Unsynchronized cross-thread access to pending frame-rate state causes a C++ data race and potentially inconsistent tuples applied to new windows.</comment>
<file context>
@@ -402,6 +408,16 @@ void ASurfaceRendererContext::registerWindowSC(int64_t contentId, const char* de
windowScMap[contentId] = sc;
}
+
+ if (pendingFrameRate != 0.f && (fnSTSetFrameRate || fnSTSetFrameRateWithChangeStrategy)) {
+ oneShot([&](void* tx) {
+ if (fnSTSetFrameRateWithChangeStrategy)
</file context>
| return false; | ||
| } | ||
|
|
||
| public void setFrameRate(float frameRate, int compatibility, int changeStrategy) { |
Contributor
There was a problem hiding this comment.
P2: Unchecked int-to-byte narrowing may corrupt frame-rate mode parameters passed to native layer
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/com/winlator/renderer/ASurfaceRenderer.java, line 649:
<comment>Unchecked int-to-byte narrowing may corrupt frame-rate mode parameters passed to native layer</comment>
<file context>
@@ -645,6 +646,13 @@ private boolean isFullscreenWindow(Window window) {
return false;
}
+ public void setFrameRate(float frameRate, int compatibility, int changeStrategy) {
+ Timber.d("setFrameRate frameRate=%f compatibility=%d changeStrategy=%d", frameRate, compatibility, changeStrategy);
+ if (surfaceInitialized) {
</file context>
| { | ||
| if (!fnSTSetFrameRate && !fnSTSetFrameRateWithChangeStrategy) return; | ||
|
|
||
| pendingFrameRate = frameRate; |
Contributor
There was a problem hiding this comment.
P2: No validation of frameRate, compatibility, or changeStrategy before persisting and applying them globally to all surfaces. Negative or NaN frame rates and invalid enum values pass through to the Android compositor.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/cpp/asurfacerenderer/ASurfaceRendererContext.cpp, line 455:
<comment>No validation of frameRate, compatibility, or changeStrategy before persisting and applying them globally to all surfaces. Negative or NaN frame rates and invalid enum values pass through to the Android compositor.</comment>
<file context>
@@ -430,3 +446,32 @@ void ASurfaceRendererContext::unregisterWindowSC(int64_t contentId) {
+{
+ if (!fnSTSetFrameRate && !fnSTSetFrameRateWithChangeStrategy) return;
+
+ pendingFrameRate = frameRate;
+ pendingCompatibility = compatibility;
+ pendingChangeStrategy = changeStrategy;
</file context>
record frame rate changes is necessary to replay it after opening the app back from background
limit it to 30 FPS so the app is not unusable on lower values example: my phone can go to as low as 10hz, limit it to 30hz at least
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (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/renderer/ASurfaceRenderer.java">
<violation number="1" location="app/src/main/java/com/winlator/renderer/ASurfaceRenderer.java:658">
P2: Hardcoded minimum 30 FPS silently overrides valid lower requested frame-rate limits</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| public void setFrameRate(float frameRate, int compatibility, int changeStrategy) { | ||
| Timber.d("setFrameRate frameRate=%f compatibility=%d changeStrategy=%d", frameRate, compatibility, changeStrategy); | ||
| if (frameRate > 0 && frameRate < 30) frameRate = 30; |
Contributor
There was a problem hiding this comment.
P2: Hardcoded minimum 30 FPS silently overrides valid lower requested frame-rate limits
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/com/winlator/renderer/ASurfaceRenderer.java, line 658:
<comment>Hardcoded minimum 30 FPS silently overrides valid lower requested frame-rate limits</comment>
<file context>
@@ -655,6 +655,7 @@ private boolean isFullscreenWindow(Window window) {
public void setFrameRate(float frameRate, int compatibility, int changeStrategy) {
Timber.d("setFrameRate frameRate=%f compatibility=%d changeStrategy=%d", frameRate, compatibility, changeStrategy);
+ if (frameRate > 0 && frameRate < 30) frameRate = 30;
pendingFrameRate = frameRate;
pendingFrameRateCompatibility = (byte) compatibility;
</file context>
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
1 participant
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
Affects what refresh rate the display is supposed to use with the app
Recording
Tried a recording but the green counter didn't show up in the video
First image is uncapped FPS, 120hz, max refresh rate
Second image is capped at 50 fps, the screen decides to use 48hz
Third image is capped at 30 fps, the screen changes to 30hz
Type of Change
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
Add frame rate control so the app can request a display refresh rate on supported devices. Adds a Java API, applies the setting to current and future windows, and persists it across app resume.
New Features
nativeSetFrameRateand publicASurfaceRenderer.setFrameRate(float, int, int)incom.winlator.renderer.XServerView.setFrameRateLimitincom.winlator.widgetforwards the limit with compatibility=0 (DEFAULT) and changeStrategy=1 (ALWAYS).Bug Fixes
Written for commit 533b1f4. Summary will update on new commits.
Summary by CodeRabbit
New Features
Integration
XServerViewso its FPS limit now applies toASurfaceRendererin addition to Vulkan.