feat: AI Frame Generation menu in sidebar#129
Conversation
Adds an AI Frame Generation switch + settings dialog to the in-game
sidebar (SidebarPerformanceFragment / winemu_sidebar_hub_type_fragment).
Toggling the switch writes byte 2 of the 10-byte mmap'd gamescope.control
file that libGameScopeVK.so polls. Gear button opens a dialog with 6
presets (Eco / Flow / Bal / Boost / Clear / Max) and a flow-scale slider;
settings persist to SharedPreferences and re-apply on every
WineActivity.onCreate so they survive game restarts.
Why it works against stock imagefs:
gamehub-lite-api's imagefs.zst v1.4.1 already ships libGameScopeVK.so
with frame interpolation compiled in (strings: "Frame interpolation
backend created", "GAMESCOPE_CONTROL_PATH", "GameScope control enabled").
The IPC is env-var-driven, so the runtime is fully in place — only the
UI + mmap writer + Vulkan ICD JSON writer were missing. No .so bundling,
no imagefs change, no API endpoint change.
What's added:
- extension/: 5 Java sources (BhFrameGenSettings, BhFrameGenWriter,
BhFrameGenWiring, BhFrameGenDialog, BhVulkanIcdWriter); all paths use
ctx.getPackageName() so they auto-adapt across the 5 package variants
- Smali injections (2): SidebarPerformanceFragment.onResume() override
calling BhFrameGenWiring.bindFromFragment(this); WineActivity.onCreate
insertion of BhFrameGenWriter.applyFromPrefs(p0) after invoke-super
- Resource patches: 3 IDs, 2 strings, 1 layout row after crt_switch
- Build pipeline: build-debug.yml adds Android SDK setup + javac + d8
step gated on hashFiles('extension/*.java') != '' so it's a no-op for
branches without the extension; patch.sh gains inject_extension_dex()
that probes for next-free classesN.dex slot in the rebuilt APK and
zips the compiled dex in before align/sign (no-op if
EXTENSION_DEX_PATH unset)
Verified:
- CI green via the patched build-debug.yml
- Built APK contains all 5 Bh* classes in classes11.dex
- Device-tested on real hardware: sidebar shows the row, toggle works,
frame interpolation activates in-game
WalkthroughAdds an AI frame-generation settings subsystem (settings model, dialog, sidebar wiring, control-file writer, and Vulkan ICD selector), integrates an RTS touch overlay into WineActivity with input hooks, extends CI to compile/inject an extension dex into the APK, and updates UI resources and the patch manifest. ChangesFrame Generation Settings System
CI Build Pipeline and Extension Dex Injection
RTS Touch Overlay Integration and Performance Management
Patch Manifest Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch (1)
286-319:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose
PerformanceHintManager$Sessionafter reporting work duration.The created hint session (v3) is never closed.
PerformanceHintManager.SessionimplementsAutoCloseableand holds native system resources that must be explicitly released viaclose(). Failing to close leaks binder objects and system memory, particularly problematic across activity recreations. Add proper cleanup in a try-finally or exception handler afterreportActualWorkDuration.🤖 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 `@patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch` around lines 286 - 319, The PerformanceHintManager.Session returned by PerformanceHintManager.createHintSession (stored in v3) is never closed; wrap the createHintSession/reportActualWorkDuration sequence in a try/finally (or equivalent error-path handling) and always call Session.close() on the v3 instance in the finally block to release native resources; locate the createHintSession call and the subsequent reportActualWorkDuration invocation and insert guaranteed cleanup (invoke close on the PerformanceHintManager$Session) regardless of success or exceptions.
🤖 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 @.github/workflows/build-debug.yml:
- Line 61: Replace the floating tag "uses: android-actions/setup-android@v3"
with a pinned full commit SHA for the android-actions/setup-android action;
locate the desired stable commit SHA on the android-actions/setup-android
repository (e.g., from the v3 release tag), and update the workflow entry to
"uses: android-actions/setup-android@<commit-sha>" so the CI uses an immutable
reference; optionally add a short comment noting which release/tag that SHA
corresponds to for future updates.
In `@extension/BhFrameGenDialog.java`:
- Line 90: Replace all hardcoded user-facing strings in BhFrameGenDialog (e.g.,
the title.setText("AI Frame Generation") call and the other hardcoded strings at
lines referenced) with lookups to string resources instead of inline literals;
add or reuse a helper like
getStringByName/getContext().getString(R.string.some_name) to fetch resources
and pass the fallback for safety, then change calls such as title.setText(...),
button.setText(...), and any label.setText(...) to use those resource lookups
(use unique resource names like bh_title_ai_frame_generation, bh_button_accept,
etc.), and ensure the helper is available in the class context so all
occurrences (title, the button labels, dialog body text) use resource lookups
rather than hardcoded text.
In `@extension/BhFrameGenWriter.java`:
- Around line 93-96: resolveControlPath currently builds the control file path
with a hardcoded "/data/data/" prefix; instead obtain the app storage root from
the Context (use ctx.getFilesDir() or equivalent) and construct the final path
by appending "usr/etc/gamescope.control" relative to that directory so it works
for multi-user/work profiles; update the resolveControlPath method to use
ctx.getFilesDir() as the base (or new File(ctx.getFilesDir(), ...)) rather than
concatenating "/data/data/" + pkg.
In `@extension/BhVulkanIcdWriter.java`:
- Around line 119-121: The dataPath(Context ctx) helper currently hardcodes
"/data/data/" + ctx.getPackageName(); change it to resolve the app data root
from the Context (e.g., use ctx.getApplicationInfo().dataDir or an equivalent
Context-derived API) so the ICD path is correct for different users/profiles;
update the dataPath method to return the application dataDir (falling back to
ctx.getPackageName() concatenation only if dataDir is null) and ensure callers
of dataPath(Context) continue to work with the returned stable path.
In `@patch.sh`:
- Around line 341-364: The script currently uses zip and unzip in
inject_extension_dex() but does not verify they are installed; update
check_dependencies() to explicitly verify both zip and unzip are present (e.g.,
using command -v or which) and call process failure (print error and exit
non‑zero) if either is missing so the build fails early with a clear message;
keep references to inject_extension_dex(), EXTENSION_DEX_PATH and WORK_DIR so
reviewers can locate the affected logic and ensure inject_extension_dex() can
assume those binaries exist afterwards.
In `@patches/diffs/res/layout/winemu_sidebar_hub_type_fragment.xml.patch`:
- Line 9: The layout references drawable `@drawable/winemu_ic_setting_focus_white`
for the ImageButton with id btn_frame_gen_settings but that drawable is not
included in this patch; ensure the drawable exists in the base APK or add it to
this PR (or change the reference to an existing drawable) so the layout can
inflate; locate the ImageButton by its id btn_frame_gen_settings in
winemu_sidebar_hub_type_fragment.xml and either add the missing resource named
winemu_ic_setting_focus_white to the project's drawable resources or update the
android:src to a valid existing drawable or themed attribute before merging.
---
Outside diff comments:
In `@patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch`:
- Around line 286-319: The PerformanceHintManager.Session returned by
PerformanceHintManager.createHintSession (stored in v3) is never closed; wrap
the createHintSession/reportActualWorkDuration sequence in a try/finally (or
equivalent error-path handling) and always call Session.close() on the v3
instance in the finally block to release native resources; locate the
createHintSession call and the subsequent reportActualWorkDuration invocation
and insert guaranteed cleanup (invoke close on the
PerformanceHintManager$Session) regardless of success or exceptions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9aea3e1-a4e9-48bb-ad85-9d5fc37352de
📒 Files selected for processing (13)
.github/workflows/build-debug.ymlextension/BhFrameGenDialog.javaextension/BhFrameGenSettings.javaextension/BhFrameGenWiring.javaextension/BhFrameGenWriter.javaextension/BhVulkanIcdWriter.javapatch.shpatches/diffs/res/layout/winemu_sidebar_hub_type_fragment.xml.patchpatches/diffs/res/values/ids.xml.patchpatches/diffs/res/values/strings.xml.patchpatches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patchpatches/diffs/smali_classes4/com/xj/winemu/sidebar/SidebarPerformanceFragment.smali.patchpatches/files_to_patch.txt
inject_extension_dex() uses both `unzip` (to probe the next-free classesN.dex slot in the rebuilt APK) and `zip` (to add the compiled extension dex back in) but neither was validated up-front. Without them the build fails late and opaquely. Adding both to check_dependencies() so the missing-deps banner names the actual problem. Addresses CodeRabbit Producdevity#5 on Producdevity#129.
Replace floating @vn tags with full commit SHAs. The trailing comment records which release tag the SHA corresponds to so future updates can re-pin to a newer release without re-resolving by hand. actions/checkout v4 → 34e1148… (v4.3.1) actions/setup-java v4 → c1e3236… (v4.8.0) android-actions/setup-android v3 → 9fc6c4e… (v3.2.2) actions/upload-artifact v4 → ea165f8… (v4.6.2) Addresses CodeRabbit Producdevity#1 on Producdevity#129; resolves the zizmor unpinned-uses policy hit at line 61 and applies the same hardening to the three other previously-unpinned references in the same workflow.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/build-debug.yml:
- Line 28: Update the GitHub Actions checkout step that currently uses
"actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5" to explicitly set
persist-credentials: false so the workflow does not leave the GITHUB_TOKEN in
the local git config; locate the checkout step in the workflow and add the
persist-credentials: false input under that step to disable credential
persistence for subsequent steps.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d3021b6-d706-4bae-b64d-67ab809dc209
📒 Files selected for processing (1)
.github/workflows/build-debug.yml
|
Both CodeRabbit items from earlier are addressed:
Both CI jobs (Build Debug APK + Validate Patches) green on the latest commit. Separately, CI is flagging that
After that the drift annotation should go silent and the patches become deterministic again. Want me to open that as a follow-up? |
|
Skipping dex injection is safe only when nothing in the patched APK references the extension classes. In this PR the smali hooks are unconditional, so a normal documented local build ( Could we either compile I am open for discussing the solution, but this will break things on managed sub branches |
|
I think there is an CI issue introduced: this PR adds This PR still runs CI because it also touches |
|
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch (1)
261-279: 💤 Low valueThermal headroom result is fetched but never used.
PowerManager.getThermalHeadroom(I)Fis called but the float result is discarded. If this is placeholder code for future use, consider removing it to avoid confusion; if it's meant to influence behavior, the result needs to be stored/checked.🤖 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 `@patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch` around lines 261 - 279, The call to PowerManager.getThermalHeadroom(I)F in WineActivity is discarding its float result; either remove this unused call or capture and use the returned thermal headroom value: after invoking getThermalHeadroom on the PowerManager obtained via getSystemService, store the float result and then handle it (e.g., log it, compare against a threshold and branch, or otherwise influence behavior) so the call is not a no-op; ensure the SDK_INT check around the block remains and update the registers/locals used (the result of getThermalHeadroom) accordingly.patch.sh (1)
369-383: 💤 Low valueConsider using
findinstead oflsfor safer path handling.Static analysis flags these
lsusages (SC2012). While the current code works for controlled SDK paths, usingfindwould be more robust against edge cases with unusual characters in paths.♻️ Suggested refactor
local android_jar - android_jar=$(ls -1 "$sdk_root"/platforms/android-*/android.jar 2>/dev/null | sort -V | tail -1) + android_jar=$(find "$sdk_root/platforms" -maxdepth 2 -name 'android.jar' 2>/dev/null | sort -V | tail -1) if [ -z "$android_jar" ] || [ ! -f "$android_jar" ]; then print_error "android.jar not found under $sdk_root/platforms/." print_error "Install a platform, e.g. sdkmanager 'platforms;android-34'." return 1 fi local d8_bin - d8_bin=$(ls -1 "$sdk_root"/build-tools/*/d8 2>/dev/null | sort -V | tail -1) + d8_bin=$(find "$sdk_root/build-tools" -maxdepth 2 -name 'd8' -type f 2>/dev/null | sort -V | tail -1)
🤖 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.
Nitpick comments:
In `@patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch`:
- Around line 261-279: The call to PowerManager.getThermalHeadroom(I)F in
WineActivity is discarding its float result; either remove this unused call or
capture and use the returned thermal headroom value: after invoking
getThermalHeadroom on the PowerManager obtained via getSystemService, store the
float result and then handle it (e.g., log it, compare against a threshold and
branch, or otherwise influence behavior) so the call is not a no-op; ensure the
SDK_INT check around the block remains and update the registers/locals used (the
result of getThermalHeadroom) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db8fee53-2c40-4895-b7d2-7c2ba5968838
📒 Files selected for processing (3)
.github/workflows/build-debug.ymlpatch.shpatches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch
|
All 3 addressed on
Happy to flip the |
|
PowerManager.getThermalHeadroom(I)F is called but the float result is discarded. is this intentional? do we need it here? (edit: resolved) |
Walk through every comment from Producdevity's review on PR Producdevity#129 and resolve each (no nitpick skipping): CodeRabbit findings: - BhFrameGenDialog.java: route hardcoded dialog strings through string resources (title, preset header, flow-scale header, close button, preset mode suffix) with safe fallbacks; reuse rts_gesture_settings_close for the Close button. - BhFrameGenWriter.java: build control path from ctx.getFilesDir() instead of "/data/data/<pkg>/files/..." so multi-user/work profiles resolve correctly. - BhVulkanIcdWriter.java: dataPath() now returns ctx.getApplicationInfo() .dataDir for the same reason. - build-debug.yml: persist-credentials: false on actions/checkout so GITHUB_TOKEN does not survive into the local git config. - patch.sh: switch ls -1 globbing to find -name for android.jar / d8 discovery. - WineActivity.smali.patch: * Drop dead getThermalHeadroom() call (result was discarded). * Wrap reportActualWorkDuration in a try/catch and close() PerformanceHintManager.Session on both success and failure paths. Java code comment sweep across extension/*.java: Producdevity asked that comments either explain a non-obvious "why" or flag that the code is not self-descriptive. Stripped narrating-style banners, section dividers, planning/phase references, and chat-context master-map citations; kept comments documenting the on-disk gamescope protocol layout, the reasons for reflection-based dispatch, idempotency invariants, and ownership boundaries. Also removed unused clampInt helper in BhFrameGenWriter. New string resources added in strings.xml.patch: bh_framegen_dialog_flow_scale, bh_framegen_dialog_mode_suffix, bh_framegen_dialog_preset
…il loudly on apktool rebuild WineActivity.onCreate already defines :try_start_0/_1/_2 and :catch_0/_1, so the previously-added :try_start_0/:catch_0 around reportActualWorkDuration collided with the existing labels and made apktool reassembly fail. Rename to :try_start_3 / :try_end_3 / :catch_2. While here, drop the `| tail -5` pipe on the apktool rebuild call so the next failure surfaces the actual error in CI logs rather than the end of the wrapping stack trace, and fail the script when apktool exits non-zero (the pipe was masking the exit code).
|
Thanks for the detailed feedback — sorry about the previous miss on the nitpicks and the comment noise. I walked through every CodeRabbit comment on the PR with the explicit rule that nothing gets skipped, and did a separate pass over Commit CodeRabbit items resolved:
Java comment cleanup pass: Stripped from
Kept as load-bearing why comments:
Also dropped Let me know if there's anything I trimmed that you'd want back, or any other tone/style preference for the comments that should be in the next pass. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
extension/BhFrameGenDialog.java (1)
274-276:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse an explicit
LocaleinformatFloatto avoid comma-decimal in non-en locales.
String.format("%.2f", v)falls back toLocale.getDefault(). On devices with locales such asfr/de/ru, the flow-scale label will render as0,20while the rest of the slider semantics (range0.2..1.0, internal floats) use a dot. For a technical scalar like this,Locale.ROOT(orLocale.US) keeps the displayed value consistent with the underlying numeric contract.🧰 Proposed fix
- private static String formatFloat(float v) { - return String.format("%.2f", v); - } + private static String formatFloat(float v) { + return String.format(java.util.Locale.ROOT, "%.2f", v); + }🤖 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 `@extension/BhFrameGenDialog.java` around lines 274 - 276, The formatting of floats in formatFloat currently relies on the default locale which can produce comma decimals; update formatFloat to use an explicit locale (e.g., Locale.ROOT or Locale.US) by calling String.format(Locale.ROOT, "%.2f", v) and add the necessary java.util.Locale import so the displayed flow-scale uses a dot decimal consistently across locales.
🤖 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.
Outside diff comments:
In `@extension/BhFrameGenDialog.java`:
- Around line 274-276: The formatting of floats in formatFloat currently relies
on the default locale which can produce comma decimals; update formatFloat to
use an explicit locale (e.g., Locale.ROOT or Locale.US) by calling
String.format(Locale.ROOT, "%.2f", v) and add the necessary java.util.Locale
import so the displayed flow-scale uses a dot decimal consistently across
locales.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd428c26-2da8-46fe-9ebd-5d8fa970c75e
📒 Files selected for processing (9)
.github/workflows/build-debug.ymlextension/BhFrameGenDialog.javaextension/BhFrameGenSettings.javaextension/BhFrameGenWiring.javaextension/BhFrameGenWriter.javaextension/BhVulkanIcdWriter.javapatch.shpatches/diffs/res/values/strings.xml.patchpatches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch
💤 Files with no reviewable changes (1)
- extension/BhFrameGenSettings.java
✅ Files skipped from review due to trivial changes (1)
- patches/diffs/res/values/strings.xml.patch
…decimal on non-en locales
String.format("%.2f", v) falls back to Locale.getDefault(), which
renders e.g. 0,20 on fr/de/ru devices while the slider range and
preset values use a dot internally. Pin to Locale.ROOT so the
displayed flow-scale matches the underlying numeric contract.
Addresses CodeRabbit outside-diff comment on PR Producdevity#129.
is this fixed? |
is this fixed? |
| public static String resolveVkLibraryPath(Context ctx) { | ||
| return dataPath(ctx) + "/files/usr/lib/libGameScopeVK.so"; | ||
| } |
There was a problem hiding this comment.
what happens if a user is on an older firmware (imagefs) version that doesn't have this lib available? or any of the other libs that this PR assumes exists.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat extension/BhVulkanIcdWriter.java
echo "---"
cat extension/BhFrameGenWiring.java
echo "---"
cat extension/BhFrameGenWriter.java
Length of output: 10854
@Producdevity Great question — here's the full picture after reading the code:
Scenario 1 — .so entirely absent (very old imagefs) ✅ Handled
BhFrameGenWiring.bind() gates everything on BhVulkanIcdWriter.isVkAvailable(ctx), which is a simple new File(resolveVkLibraryPath(ctx)).exists() check:
if (!BhVulkanIcdWriter.isVkAvailable(ctx)) {
if (container != null) container.setVisibility(View.GONE);
return;
}The entire sidebar row is hidden and nothing else runs. ensureIcdJson also no-ops when the .so is missing. The only minor side-effect is that BhFrameGenWriter.applyFromPrefs(ctx) is still called unconditionally in WineActivity.onCreate, which will create the gamescope.control file even though nothing will ever read it — harmless, but unnecessary.
Scenario 2 — .so present but old version (pre-imagefs v1.4.1, no FrameGen IPC) ⚠️ Not handled
This is the real gap. isVkAvailable is a blunt file-existence check, not a capability/version check. An older imagefs might ship a libGameScopeVK.so that lacks frame interpolation and the 10-byte IPC protocol entirely. In that case:
- The UI row will be shown (lib file exists)
- The user can toggle the switch and open the dialog
- The control file is written correctly
- But the old
.sosilently ignores it — no crash, but the feature does nothing
There's no version probe or symbol check to distinguish an old lib from v1.4.1+.
Suggested mitigations
-
Guard
applyFromPrefsinonCreatebehindisVkAvailable— avoids the unnecessary file write on old firmware. -
Version/capability check — the cleanest approach would be checking a version marker in the imagefs (e.g. a known file like
/files/usr/etc/imagefs.versionor a sentinel byte in the.soitself), and surfacing it viaisFrameGenCapable(ctx)used alongsideisVkAvailable. Would the imagefs ship a version file you could read? -
Alternative: gate on control file readability — if
libGameScopeVK.sopolls the control file, you could check whether the control file is created by the runtime (i.e. by the.so) rather than creating it proactively. But this may complicate the bootstrap flow.
Would you like to open a follow-up issue to track the version-check gap, or address it within this PR?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| /** SidebarSwitchItemView (com.xj.winemu.view) renders the switch as an ImageView, | ||
| * not a CompoundButton; driven through its public setSwitch(boolean). */ |
There was a problem hiding this comment.
why do we need to know what it is not? please keep comments concise and only included when the code itself isn't descriptive enough for whatever reason
|
|
||
| /** Bind switch + gear button. Idempotent — onResume can call repeatedly. | ||
| * Gear visibility mirrors RTS pattern: gone by default, visible only when ON. */ | ||
| /** Idempotent — onResume can call repeatedly. */ |
There was a problem hiding this comment.
What is the point of this comment?
| ECO (0, 0.2f, "Eco", "Lowest overhead, suitable for lower-end devices or battery-sensitive play."), | ||
| FLOW (0, 0.4f, "Flow", "Low overhead smoothness boost for most lightweight games."), |
There was a problem hiding this comment.
can we use resource strings please?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch (1)
299-311: 💤 Low valueOptional: the PerformanceHintManager session is created, reports 1 µs of "actual" work, then immediately closed.
reportActualWorkDuration(0x3e8)is 1000 ns ≈ 1 µs and is followed byclose()on both paths, so the hint never represents real frame work. If the goal is just to verify the API is reachable, the report call can be dropped; if the goal is to drive thermal/perf hinting for the game thread, the session needs to be held and updated with real per-frame durations. Either way, this block currently does very little besides validating that the binder calls don't throw.Not blocking — safe to defer or land as-is.
🤖 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 `@patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch` around lines 299 - 311, The code creates an android.os.PerformanceHintManager$Session (v3), calls reportActualWorkDuration(0x3e8) (≈1µs) then immediately close()s the session, so the hint never represents real work; either remove the artificial reportActualWorkDuration call if you only need to validate the API, or keep the session open and call reportActualWorkDuration(J) with real per-frame durations on each frame and only call close() when the game thread is finished; locate the session usage around PerformanceHintManager$Session (v3), the reportActualWorkDuration invocation and the paired close() calls and update accordingly.
🤖 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.
Nitpick comments:
In `@patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch`:
- Around line 299-311: The code creates an
android.os.PerformanceHintManager$Session (v3), calls
reportActualWorkDuration(0x3e8) (≈1µs) then immediately close()s the session, so
the hint never represents real work; either remove the artificial
reportActualWorkDuration call if you only need to validate the API, or keep the
session open and call reportActualWorkDuration(J) with real per-frame durations
on each frame and only call close() when the game thread is finished; locate the
session usage around PerformanceHintManager$Session (v3), the
reportActualWorkDuration invocation and the paired close() calls and update
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24865053-5d9e-43da-bfa9-cbd7950e021e
📒 Files selected for processing (9)
.github/workflows/build-debug.ymlextension/BhFrameGenDialog.javaextension/BhFrameGenSettings.javaextension/BhFrameGenWiring.javaextension/BhFrameGenWriter.javaextension/BhVulkanIcdWriter.javapatch.shpatches/diffs/res/values/strings.xml.patchpatches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch
💤 Files with no reviewable changes (1)
- extension/BhFrameGenSettings.java
CodeRabbit (BhFrameGenDialog.java:274-276): already fixed in fa0e29a (Locale.ROOT in formatFloat) — no further change. CodeRabbit (WineActivity.smali.patch:261-279): thermal-headroom block already removed in 470b163 — no further change. CodeRabbit (WineActivity.smali.patch:299-311): the createHintSession + reportActualWorkDuration(1µs) + close block was a smoke test of the binder API, not a real per-frame perf hint. Holding the session for live frame durations would require hooking the frame loop and is out of scope here. Removed the entire block. Side effect: the patch loses the upstream :cond_0../:cond_1f rename cascade through onCreate that the block forced, dropping it from 39 hunks to 7 (-674 lines). Producdevity (BhVulkanIcdWriter.java:52, older imagefs): frame interpolation in libGameScopeVK.so was introduced in firmware 1.3.6. On older firmware where the .so is absent, the code already degraded safely (ICD JSON write short-circuits, control file is harmless, smali hooks swallow). Added BhVulkanIcdWriter.isVkAvailable(Context) (mirrors existing isV2Available) and gated BhFrameGenWiring.bind() on it so the entire @id/frame_gen_container row is hidden on legacy firmware instead of showing a non-functional switch. Producdevity (BhFrameGenWiring.java:28): removed the "Idempotent — onResume can call repeatedly." comment. Producdevity (BhFrameGenWiring.java:60-61): reworded the comment on invokeSetSwitch to a single line that just states the why (no compile classpath access to the patched app's SidebarSwitchItemView). Dropped the "not a CompoundButton" framing. Producdevity (BhFrameGenSettings.java:12, resource strings): moved preset labels and descriptions out of code. Preset now stores labelResName/descResName (bh_framegen_preset_<preset>_label/_desc) and exposes getLabel(Context)/getDescription(Context) resolvers that use the same getIdentifier + English-fallback pattern already in BhFrameGenDialog.getStringByName. Added 12 entries to res/values/strings.xml.
|
Pushed
To go one better than "silent no-op," I added
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch (1)
67-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the pref write consistent with bridge availability.
Lines 67-90 write
InputControlsManager.h(false, profileId)even whenWineActivity.his null, but the equivalent startup path at Lines 208-232 skips that write unlessk0(false)can actually run. That can leave the saved trackpad state out of sync with the real bridge state and break the later restore path.Also applies to: 208-232
🤖 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 `@patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch` around lines 67 - 90, The code writes InputControlsManager.h(false, profileId) even when the WinUIBridge isn't available, causing pref state to diverge; change the flow so the InputControlsManager.h(false, profileId) call only happens when the WinUIBridge instance exists and you've invoked WinUIBridge.k0(false) (i.e., guard the pref write behind the same bridge-availability check), and apply the same guard/fix to the other startup path (the block around the earlier 208-232 area); reference WinUIBridge.k0(Z)V, WineActivity.r / WineActivityData.e(), and InputControlsManager.h(ZLjava/lang/String;)V to locate the affected calls.
🧹 Nitpick comments (1)
extension/BhFrameGenSettings.java (1)
56-65: ⚡ Quick winConsider clamping
flowScale/modelon load.If
SharedPreferencesever contains an out-of-range orNaNvalue (manual edits, downgrade/upgrade, parallel writer bug), it will be passed straight through to the writer and UI (whereflowScaleToProgresswould clamp the slider but the persistedsettings.flowScalewould still be bad). A defensive clamp on load (e.g.,0.2f..1.0fforflowScale,0..1formodel) keeps the in-memory model trustworthy regardless of source.♻️ Suggested clamp
- s.flowScale = sp.getFloat("flowScale", s.preset.flowScale); - s.model = sp.getInt("model", s.preset.model); + float fs = sp.getFloat("flowScale", s.preset.flowScale); + if (Float.isNaN(fs) || fs < 0.2f) fs = 0.2f; + else if (fs > 1.0f) fs = 1.0f; + s.flowScale = fs; + int m = sp.getInt("model", s.preset.model); + s.model = (m < 0) ? 0 : (m > 1 ? 1 : m); return s;🤖 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 `@extension/BhFrameGenSettings.java` around lines 56 - 65, BhFrameGenSettings.load currently trusts stored flowScale and model values; after reading s.flowScale and s.model (in BhFrameGenSettings.load), defensively sanitize them: if Float.isNaN(s.flowScale) set to s.preset.flowScale, then clamp s.flowScale to the allowed range (e.g., Math.max(0.2f, Math.min(1.0f, s.flowScale))); for s.model validate it’s within allowed integer bounds (e.g., 0..1) and if out of range reset to s.preset.model or clamp to 0/1; ensure you reference the fields s.flowScale and s.model and perform these checks immediately after they are loaded so the in-memory BhFrameGenSettings is always valid.
🤖 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.
Outside diff comments:
In `@patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch`:
- Around line 67-90: The code writes InputControlsManager.h(false, profileId)
even when the WinUIBridge isn't available, causing pref state to diverge; change
the flow so the InputControlsManager.h(false, profileId) call only happens when
the WinUIBridge instance exists and you've invoked WinUIBridge.k0(false) (i.e.,
guard the pref write behind the same bridge-availability check), and apply the
same guard/fix to the other startup path (the block around the earlier 208-232
area); reference WinUIBridge.k0(Z)V, WineActivity.r / WineActivityData.e(), and
InputControlsManager.h(ZLjava/lang/String;)V to locate the affected calls.
---
Nitpick comments:
In `@extension/BhFrameGenSettings.java`:
- Around line 56-65: BhFrameGenSettings.load currently trusts stored flowScale
and model values; after reading s.flowScale and s.model (in
BhFrameGenSettings.load), defensively sanitize them: if Float.isNaN(s.flowScale)
set to s.preset.flowScale, then clamp s.flowScale to the allowed range (e.g.,
Math.max(0.2f, Math.min(1.0f, s.flowScale))); for s.model validate it’s within
allowed integer bounds (e.g., 0..1) and if out of range reset to s.preset.model
or clamp to 0/1; ensure you reference the fields s.flowScale and s.model and
perform these checks immediately after they are loaded so the in-memory
BhFrameGenSettings is always valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bcd7ea7-da19-402a-abc9-f9f3951e2254
📒 Files selected for processing (6)
extension/BhFrameGenDialog.javaextension/BhFrameGenSettings.javaextension/BhFrameGenWiring.javaextension/BhVulkanIcdWriter.javapatches/diffs/res/values/strings.xml.patchpatches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch
|
Close PerformanceHintManager$Session after reporting work duration. The created hint session (v3) is never closed. PerformanceHintManager.Session implements AutoCloseable and holds native system resources that must be explicitly released via close(). Failing to close leaks binder objects and system memory, particularly problematic across activity recreations. Add proper cleanup in a try-finally or exception handler after reportActualWorkDuration. is this still an issue? |
|
patches/diffs/smali_classes4/com/xj/winemu/WineActivity.smali.patch (1) Keep the pref write consistent with bridge availability. Lines 67-90 write InputControlsManager.h(false, profileId) even when WineActivity.h is null, but the equivalent startup path at Lines 208-232 skips that write unless k0(false) can actually run. That can leave the saved trackpad state out of sync with the real bridge state and break the later restore path. Also applies to: 208-232 |
2 participants
Summary
Adds an AI Frame Generation switch + settings dialog to the in-game sidebar (
SidebarPerformanceFragment/winemu_sidebar_hub_type_fragment). Toggling the switch writes byte 2 of the 10-byte mmap'dgamescope.controlfile thatlibGameScopeVK.sopolls. Gear button opens a dialog with 6 presets (Eco / Flow / Bal / Boost / Clear / Max) and a flow-scale slider; settings persist to SharedPreferences and re-apply on everyWineActivity.onCreate/onResumeso they survive game restarts and resumes.Origin
This is a port of work I originally added to my Bannerhub-Lite fork, with additional fixes contributed by @teldommm (Seldom) on top — see The412Banner/Bannerhub-Lite#6 for the upstream fix series:
fix: apply FrameGen settings on WineActivity.onResume + hide gear when disabledAdd blue close button(dialog can only be dismissed via the explicit Close button)feat: migrate FrameGen to Performance tab + add GameScope V2 driver supportAll of those refinements are folded into this single squashed commit.
Why it works against stock imagefs
gamehub-lite-api'simagefs.zstv1.4.1 already shipslibGameScopeVK.sowith frame interpolation compiled in (strings:Frame interpolation backend created,GAMESCOPE_CONTROL_PATH,GameScope control enabled). The IPC is env-var-driven, so the runtime is fully in place — only the UI + mmap writer + Vulkan ICD JSON writer were missing.No .so bundling, no imagefs change, no API endpoint change.
What's added
extension/: 5 Java sources (BhFrameGenSettings,BhFrameGenWriter,BhFrameGenWiring,BhFrameGenDialog,BhVulkanIcdWriter); all paths usectx.getPackageName()so they auto-adapt across the 5 package variantsSidebarPerformanceFragment.onResume()override callingBhFrameGenWiring.bindFromFragment(this);WineActivity.onCreateinsertion ofBhFrameGenWriter.applyFromPrefs(p0)after invoke-supercrt_switchbuild-debug.ymladds Android SDK setup + javac + d8 step gated onhashFiles('extension/*.java') != ''so it's a no-op for branches without the extension;patch.shgainsinject_extension_dex()that probes for the next-freeclassesN.dexslot in the rebuilt APK and zips the compiled dex in before align/sign (no-op ifEXTENSION_DEX_PATHunset)Verified
build-debug.ymlBh*classes inclasses11.dexTest plan
Summary by CodeRabbit
New Features
UI/UX Updates
Stability