feat: add per-game backdrop images for letterboxing#1002
feat: add per-game backdrop images for letterboxing#1002xXJSONDeruloXx wants to merge 9 commits intoutkarshdalal:masterfrom
Conversation
|
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 a container-level backdrop image feature: dialog picker with persistable-URI handling and cleanup, persistence in ContainerData/ContainerUtils, prioritized image resolution and Coil loading in XServerScreen, and OpenGL backdrop rendering plus viewport shadowing in the renderer. Changes
Sequence DiagramsequenceDiagram
participant User
participant Dialog
participant State
participant ContainerData
participant XServerScreen
participant Coil
participant Renderer
User->>Dialog: open dialog / pick image
Dialog->>Dialog: takePersistableUriPermission(...)
Dialog->>State: set config.backdropImageUri
State->>ContainerData: persist backdropImageUri (ContainerUtils)
Note over XServerScreen,Coil: LaunchedEffect resolves and loads candidates
XServerScreen->>XServerScreen: resolveBackdropImageModels()
XServerScreen->>Coil: request bitmap for candidate URLs
Coil-->>XServerScreen: bitmap or failure
alt bitmap loaded
XServerScreen->>Renderer: setBackdropBitmap(bitmap)
Renderer->>Renderer: ensureBackdropTexture()/upload
Renderer->>Renderer: renderBackdrop() and renderViewportShadow()
else all fail
XServerScreen->>Renderer: clearBackdrop()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
1 issue found across 10 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/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:332">
P2: Catching `Throwable` in suspend backdrop loading swallows coroutine cancellation and can allow stale `LaunchedEffect` work to update renderer state.</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
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/dialog/ContainerConfigDialog.kt`:
- Around line 836-849: The backdropImagePickerLauncher callback currently takes
a new persistable URI permission but never revokes any previous grant; update
the launcher (rememberLauncherForActivityResult) to parse the existing
config.backdropImageUri, and if non-null, call
context.contentResolver.releasePersistableUriPermission(Uri.parse(oldUri),
Intent.FLAG_GRANT_READ_URI_PERMISSION) inside a try/catch for SecurityException
before taking the new permission and setting config =
config.copy(backdropImageUri = uri.toString()); likewise, when the image is
cleared (e.g., in GeneralTab's clear button handler that updates
config.backdropImageUri to null/empty), add similar logic to
releasePersistableUriPermission for the old URI before clearing the config to
avoid accumulating stale grants.
In `@app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt`:
- Around line 327-333: The delete IconButton currently sets the Icon's
contentDescription to null, making the clear action inaccessible; update the
Icon (inside IconButton) to provide an appropriate, localized contentDescription
(e.g., via stringResource like a new R.string.clear_backdrop_image or an
existing descriptive string) so screen readers announce the button's purpose,
while keeping the onClick that calls state.config.value =
config.copy(backdropImageUri = "") unchanged.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 268-270: The Steam branch currently adds steamAppInfo.headerUrl
before steamAppInfo.getHeroUrl(), which makes the hero art fallback unreachable;
change the order so getHeroUrl() is evaluated and added to models first (call
steamAppInfo?.getHeroUrl()?.takeIf { it.isNotEmpty() }?.let(models::add) before
steamAppInfo?.headerUrl?.takeIf { it.isNotEmpty() }?.let(models::add)) so hero
art is prioritized, keeping the existing null/empty guards intact.
- Around line 314-318: The ImageRequest currently decodes images at full
resolution (ImageRequest.Builder with .data(imageModel)), which can cause
OOM/GL_MAX_TEXTURE_SIZE issues; update the request to cap decoding to the
renderer surface dimensions by passing the surfaceWidth and surfaceHeight into
the request (e.g., ImageRequest.Builder(...).data(imageModel).size(surfaceWidth,
surfaceHeight) or use scale/resolution APIs available) so the decoded bitmap is
bounded to the target surface before upload; keep allowHardware(false) as-is and
apply this change where ImageLoader and ImageRequest.Builder are used in
XServerScreen (reference: imageModel, ImageLoader, ImageRequest.Builder,
surfaceWidth, surfaceHeight).
- Around line 332-334: In XServerScreen.kt replace the broad "catch (e:
Throwable)" used when loading the backdrop image model with a handler that
preserves coroutine cancellation: catch only Exception and explicitly re-throw
CancellationException (e.g., if (e is CancellationException) throw e) before
logging with Timber, so the LaunchedEffect/ coroutine scope for the backdrop
loader is allowed to cancel promptly when appId changes; update the catch in the
backdrop image-loading block accordingly.
In `@app/src/main/java/com/winlator/renderer/GLRenderer.java`:
- Around line 410-426: setBackdropBitmap and clearBackdrop are mutating GL state
and shared fields (backdropBitmap, backdropTextureId, backdropTextureDirty) off
the render thread and calling GLES20.glDeleteTextures via
deleteBackdropTexture(), which causes GL context violations and data races; fix
by moving all GL calls and mutations of backdropTextureId/backdropTextureDirty
into the GL render thread: have setBackdropBitmap/clearBackdrop only update the
Java-side bitmap reference (backdropBitmap) and post a runnable to the renderer
thread (e.g., via xServerView.queueEvent or the renderer's task queue) that
calls deleteBackdropTexture(), sets backdropTextureDirty, and updates
backdropTextureId as needed, or alternatively enqueue a small command like
renderer.markBackdropForUpdate(bitmap) that the render thread will consume
inside ensureBackdropTexture()/renderBackdrop(); ensure deleteBackdropTexture()
is only ever invoked on the render thread.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2d0ce32-e9b0-44bf-9a46-fe58697afd7f
📒 Files selected for processing (10)
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.ktapp/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigState.ktapp/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/com/winlator/container/ContainerData.ktapp/src/main/java/com/winlator/renderer/GLRenderer.javaapp/src/main/java/com/winlator/renderer/material/ShaderMaterial.javaapp/src/main/java/com/winlator/renderer/material/ShadowMaterial.javaapp/src/main/res/values/strings.xml
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
1 issue found across 6 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/component/dialog/ContainerConfigDialog.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt:938">
P2: Old backdrop URI permission is revoked before save completion is confirmed, which can break existing backdrop access if save fails/asynchronously errors.</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/component/dialog/ContainerConfigDialog.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
449-465: Consider deferring the decode until the surface has a real size.This effect usually starts as soon as
xServerViewis assigned, which is earlier than the first non-zero renderer/view dimensions. Falling back todisplayMetricsis fine as a fallback, but waiting for the measured surface would keep the decode target aligned with post-attach and resize paths too.🤖 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 449 - 465, The effect should wait for a real surface size before decoding: in the LaunchedEffect that uses xServerView and appId, replace the immediate decode logic with a size-wait — capture currentXServerView and renderer, then suspend until renderer.surfaceWidth and renderer.surfaceHeight (or currentXServerView.width/height) become > 0 (for example using snapshotFlow { Pair(renderer.surfaceWidth, renderer.surfaceHeight) } and filter/first { it.first > 0 && it.second > 0 }), then compute targetWidth/targetHeight and call loadBackdropBitmap, and finally call renderer.setBackdropBitmap or renderer.clearBackdrop; keep the same function names (LaunchedEffect, xServerView, renderer, loadBackdropBitmap, setBackdropBitmap, clearBackdrop) so the decode happens only after a real measured surface exists.
🤖 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/renderer/GLRenderer.java`:
- Around line 410-429: setBackdropBitmap and clearBackdrop currently recycle
incoming Bitmap instances even though loadBackdropBitmap can return either
cached (borrowed) bitmaps from Coil or newly created (owned) bitmaps, which
causes cache corruption or leaks. Fix by choosing one of two consistent
ownership strategies: (A) Make loadBackdropBitmap always return an owned copy
(use Bitmap.createBitmap on the drawable.bitmap) so the renderer can safely
recycle in setBackdropBitmap and clearBackdrop (update loadBackdropBitmap, then
keep recycle calls in setBackdropBitmap/clearBackdrop); or (B) Treat bitmaps as
borrowed everywhere: remove the recycle calls from setBackdropBitmap and
clearBackdrop, and ensure all callers that create owned bitmaps (e.g., any
Bitmap.createBitmap usage) are responsible for recycling them and document
ownership in loadBackdropBitmap and the renderer APIs. Apply one strategy
consistently and update comments/docs accordingly for loadBackdropBitmap,
setBackdropBitmap, and clearBackdrop.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 449-465: The effect should wait for a real surface size before
decoding: in the LaunchedEffect that uses xServerView and appId, replace the
immediate decode logic with a size-wait — capture currentXServerView and
renderer, then suspend until renderer.surfaceWidth and renderer.surfaceHeight
(or currentXServerView.width/height) become > 0 (for example using snapshotFlow
{ Pair(renderer.surfaceWidth, renderer.surfaceHeight) } and filter/first {
it.first > 0 && it.second > 0 }), then compute targetWidth/targetHeight and call
loadBackdropBitmap, and finally call renderer.setBackdropBitmap or
renderer.clearBackdrop; keep the same function names (LaunchedEffect,
xServerView, renderer, loadBackdropBitmap, setBackdropBitmap, clearBackdrop) so
the decode happens only after a real measured surface exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4efe209f-3321-464a-b82d-0131904bdd5d
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.ktapp/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigState.ktapp/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/renderer/GLRenderer.javaapp/src/main/res/values/strings.xml
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/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 252-308: resolveBackdropImageModels currently always runs store
metadata fallbacks even when a user-selected backdrop
(container.getExtra("backdropImageUri", "")) is present, delaying the chosen
image; change the logic to add the saved URI to models first and attempt to
validate/decode it (e.g., a quick local/file/URL reachability or ImageDecoder
check) and if the validation succeeds return that as the result immediately,
otherwise proceed to the GameSource switch (Steam/GOG/EPIC/AMAZON/CUSTOM_GAME)
to collect fallback URLs into models; update resolveBackdropImageModels to
short-circuit to models when the saved backdrop is valid and only perform the
store lookups on decode/fetch failure.
- Around line 450-455: LaunchedEffect is decoding using fallback displayMetrics
before the real render size is available; update the effect that references
xServerView/renderer to wait for a real surface/view size before resolving
images: inside the LaunchedEffect (the block that computes
targetWidth/targetHeight and calls resolveBackdropImageModels), if
renderer.surfaceWidth/renderer.surfaceHeight and
currentXServerView.width/currentXServerView.height are all zero (i.e. you would
fall back to context.resources.displayMetrics), suspend until a non-zero size is
observed (e.g., use snapshotFlow watching
renderer.surfaceWidth/renderer.surfaceHeight/currentXServerView.width/currentXServerView.height
and awaitFirst non-zero) and only then compute targetWidth/targetHeight and call
resolveBackdropImageModels so the backdrop is decoded at the real surface size.
🪄 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: 7d640add-9b6a-42d4-98df-810954a64a2c
📒 Files selected for processing (5)
app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/com/winlator/container/ContainerData.ktapp/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/app/gamenative/utils/ContainerUtils.kt
- app/src/main/java/com/winlator/container/ContainerData.kt
- app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (2)
261-264:⚠️ Potential issue | 🟠 MajorKeep store-art fallbacks available when the saved backdrop URI is stale.
Lines 261-264 return as soon as
backdropImageUriexists, so a deleted file or lost SAF grant leaves the game with no backdrop instead of falling back to hero/cover art. Try the saved URI first, but only short-circuit after it decodes successfully; otherwise continue to the source-specific candidates.🤖 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 261 - 264, The current logic short-circuits and returns models as soon as container.getExtra("backdropImageUri", "") yields a non-empty string, which prevents falling back to store art when the saved URI is stale; update the block around container.getExtra/backdropImageUri inside the withContext so you attempt to resolve/decode/validate the saved userUri (e.g., try opening/decoding the URI or checking SAF permission) and only return models.toList() if that validation succeeds, otherwise do not return early and allow the subsequent source-specific candidates (hero/cover art) to be added to models and returned at the end.
453-469:⚠️ Potential issue | 🟡 MinorWait for a real surface size before decoding.
Lines 456-457 fall back to
context.resources.displayMetricswhen the renderer/view is still0x0, and this effect is keyed only byxServerView/appId. In external-display or swap flows that can decode once at the wrong size and never refresh. Suspend until a non-zero renderer/view size is available, or include size changes in the effect key.🤖 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 453 - 469, The LaunchedEffect currently decodes/backdrops using fallback display metrics when renderer or view sizes are zero, causing one-time wrong-size decoding; update the effect to wait for a real non-zero surface size or to re-run on size changes by including the renderer/view size in the effect key (e.g., add renderer.surfaceWidth, renderer.surfaceHeight, currentXServerView.width/height) or suspend/collect until those sizes are >0 (using snapshotFlow or a small retry loop) before calling resolveBackdropImageModels/loadBackdropBitmap and renderer.setBackdropBitmap/clearBackdrop so decoding only happens once a valid targetWidth/targetHeight are available.
🤖 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 328-330: The loaded BitmapDrawable.bitmap is being returned
directly from the image load (via loader.execute(request) and SuccessResult) and
later recycled by GLRenderer.setBackdropBitmap()/clearBackdrop(), which may
recycle a cached Coil bitmap; change the loader result handling to detect
BitmapDrawable and return a copy of drawable.bitmap (e.g.,
bitmap.copy(bitmap.config ?: Bitmap.Config.ARGB_8888, true)) so the renderer
owns and can recycle the copy without poisoning Coil's cache; keep the rest of
the flow (loader.execute, SuccessResult) unchanged.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 261-264: The current logic short-circuits and returns models as
soon as container.getExtra("backdropImageUri", "") yields a non-empty string,
which prevents falling back to store art when the saved URI is stale; update the
block around container.getExtra/backdropImageUri inside the withContext so you
attempt to resolve/decode/validate the saved userUri (e.g., try opening/decoding
the URI or checking SAF permission) and only return models.toList() if that
validation succeeds, otherwise do not return early and allow the subsequent
source-specific candidates (hero/cover art) to be added to models and returned
at the end.
- Around line 453-469: The LaunchedEffect currently decodes/backdrops using
fallback display metrics when renderer or view sizes are zero, causing one-time
wrong-size decoding; update the effect to wait for a real non-zero surface size
or to re-run on size changes by including the renderer/view size in the effect
key (e.g., add renderer.surfaceWidth, renderer.surfaceHeight,
currentXServerView.width/height) or suspend/collect until those sizes are >0
(using snapshotFlow or a small retry loop) before calling
resolveBackdropImageModels/loadBackdropBitmap and
renderer.setBackdropBitmap/clearBackdrop so decoding only happens once a valid
targetWidth/targetHeight are available.
🪄 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: 4a2a5deb-7817-416c-94ca-f4ab5123891d
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
There was a problem hiding this comment.
1 issue found across 6 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/container/ContainerData.kt">
<violation number="1" location="app/src/main/java/com/winlator/container/ContainerData.kt:223">
P2: Restoring missing `enableBackdropImage` as false breaks older saved configs that already have a backdrop URI, causing backdrops to stop rendering after upgrade until the user manually re-enables them.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| unpackFiles = (savedMap["unpackFiles"] as? Boolean) ?: false, | ||
| suspendPolicy = (savedMap["suspendPolicy"] as? String) ?: Container.SUSPEND_POLICY_MANUAL, | ||
| portraitMode = (savedMap["portraitMode"] as? Boolean) ?: false, | ||
| enableBackdropImage = (savedMap["enableBackdropImage"] as? Boolean) ?: false, |
There was a problem hiding this comment.
P2: Restoring missing enableBackdropImage as false breaks older saved configs that already have a backdrop URI, causing backdrops to stop rendering after upgrade until the user manually re-enables them.
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/container/ContainerData.kt, line 223:
<comment>Restoring missing `enableBackdropImage` as false breaks older saved configs that already have a backdrop URI, causing backdrops to stop rendering after upgrade until the user manually re-enables them.</comment>
<file context>
@@ -218,6 +220,7 @@ data class ContainerData(
unpackFiles = (savedMap["unpackFiles"] as? Boolean) ?: false,
suspendPolicy = (savedMap["suspendPolicy"] as? String) ?: Container.SUSPEND_POLICY_MANUAL,
portraitMode = (savedMap["portraitMode"] as? Boolean) ?: false,
+ enableBackdropImage = (savedMap["enableBackdropImage"] as? Boolean) ?: false,
backdropImageUri = (savedMap["backdropImageUri"] as? String) ?: "",
sharpnessEffect = (savedMap["sharpnessEffect"] as? String) ?: "None",
</file context>
| enableBackdropImage = (savedMap["enableBackdropImage"] as? Boolean) ?: false, | |
| enableBackdropImage = (savedMap["enableBackdropImage"] as? Boolean) | |
| ?: ((savedMap["backdropImageUri"] as? String)?.isNotBlank() == true), |
https://discord.com/channels/1378308569287622737/1485707057108881519
Especially for 21:9 devices, allow using hero image or user provided image in background of renderer to mitigate black bars without stretching:

This example I'm making the game 4:3 so you can still see the effect in 16:9 example
Summary by cubic
Adds per-game backdrop images behind letterboxed content with hero/cover art fallback and a soft viewport shadow. Opt-in per container.
New Features
backdropImageUri, else falls back to game art (Steam hero/header, Custom grid hero, GOG image/icon, Epic portrait/cover/square/logo, Amazon hero/art). Images load viacoil.Bug Fixes
Written for commit 6a707b6. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes