Repository navigation
SurfaceFlinger renderer: RGBA_8888 scan-out slots under the daemon's layout rule - #2325
Conversation
…layout rule The SurfaceFlinger display renderer hands each X window's CPU swapchain slot to surfaceflinger as a BGRA_8888 AHardwareBuffer. Android 16 on a Skia GLES render engine cannot wrap one and aborts surfaceflinger (RedMagic Astra: "Unable to generate SkImage ... colorType 6" 30-40 ms after the first window maps, whole UI restarts). The gamescope daemon already answers this with an RGBA_8888 pool chosen by a device rule (sdk >= 36 and a GLES render engine, GNDRM_SCANOUT_LAYOUT override); the X11 path had no equivalent. The renderer's slots now follow the same rule, read through the daemon's layout query before the first drawable: RGBA_8888 slots get the X server's B,G,R,A bytes swapped on the CPU copy and go to surfaceflinger as they are, skipping the BGRA-to-RGBA blit (which would swap an RGBA-declared source again). The front buffer the X server writes into stays as it was. BGRA devices are unchanged. libahbimage.so rebuilt with NDK 27.3.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Resolve scan-out layout app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt |
The X11 setup resolves the container layout override, updates the process environment, and enables CPU RGBA scan-out when the resolved layout is "rgba". Gamescope sessions skip this setup. |
Propagate buffer format app/src/main/java/com/winlator/renderer/AHBImage.java, app/src/main/cpp/asurfacerenderer/ahbimage.c |
AHBImage captures the CPU scan-out format and passes it to swapchain buffer creation. Native creation selects RGBA or B8G8R8A8 format. |
Copy pixels with format-aware conversion app/src/main/java/com/winlator/renderer/ASurfaceRenderer.java, app/src/main/java/com/winlator/renderer/AHBImage.java, app/src/main/cpp/asurfacerenderer/ahbimage.c |
The renderer enables red-blue conversion only when compatibility mode is active and the scan-out image is not RGBA. Native copying swaps channels using contiguous or row-by-row processing. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant XServerScreen
participant AHBImage
participant ASurfaceRenderer
participant NativeAHBImage
XServerScreen->>AHBImage: Set CPU scan-out format from resolved layout
AHBImage->>NativeAHBImage: Create buffers with RGBA format flag
ASurfaceRenderer->>AHBImage: Copy CPU image with conversion flag
AHBImage->>NativeAHBImage: Copy pixels with optional red-blue swap
Possibly related PRs
- utkarshdalal/GameNative#2276: Passes the
GNDRM_SCANOUT_LAYOUTvalue into the Gamescope session; this change resolves the layout and uses it to select scan-out buffers. - utkarshdalal/GameNative#2324: Selects Vulkan for certain X11 Gamescope sessions; this change handles scan-out format and channel conversion for GLES compatibility.
Suggested reviewers: butilly
Merge Risk: 
43631
-
The change makes the renderer's scan-out slots follow the daemon's layout rule on Android 16 devices with a Skia GLES render engine. No concrete defect was established. The PR notes that the fix is not yet confirmed on the affected device. It is worth checking on a device that the layout query returns "rgba" at the point where it is called.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly identifies the main change: enabling RGBA_8888 scan-out slots for the SurfaceFlinger renderer under the daemon's layout rule. Description check The description explains the problem, implementation, affected paths, testing status, change type, and checklist items. The recording requirement is not fulfilled, but the author documents why a recor… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description explains the problem, implementation, affected paths, testing status, change type, and checklist items. The recording requirement is not fulfilled, but the author documents why a recording is unavailable, so the description is mostly complete.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 8 functions across 4 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. 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:482">
P1: With `convert` false for RGBA slots, the frame goes to the native else branch, which hands the same acquire fence fd to both `returnSourceFence` and `ASurfaceTransaction_setBuffer`. Give the fd to one owner only: either skip the `returnSourceFence` call there or `dup` the fd for the transaction. This path becomes the default on affected devices with this change.</violation>
</file>
<file name="app/src/main/cpp/asurfacerenderer/ahbimage.c">
<violation number="1" location="app/src/main/cpp/asurfacerenderer/ahbimage.c:14">
P3: This comment states an unverified universal claim as fact. Describe the layout as the one this renderer uses for CPU scan-out slots, and drop the claim about every render engine.</violation>
</file>
<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:2136">
P2: The X11 path resolves the layout via `nativeScanoutLayout()` before any `nativeStart`, which the function's KDoc says is only valid after start. Confirm the native side returns the device rule when no session is running. If it does not, the RGBA selection never takes effect on X11.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| // Disable swap R/B in cpu path as it is handled with drawable. | ||
| // An RGBA_8888 slot already holds R,G,B,A bytes: SurfaceFlinger takes it as is, and the | ||
| // BGRA-to-RGBA blit (which swaps an RGBA-declared source) must not touch it. | ||
| boolean convert = sfCompatMode && !g.isScanoutRgba(); |
There was a problem hiding this comment.
P1: With convert false for RGBA slots, the frame goes to the native else branch, which hands the same acquire fence fd to both returnSourceFence and ASurfaceTransaction_setBuffer. Give the fd to one owner only: either skip the returnSourceFence call there or dup the fd for the transaction. This path becomes the default on affected devices with this change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/main/java/com/winlator/renderer/ASurfaceRenderer.java, line 482:
<comment>With `convert` false for RGBA slots, the frame goes to the native else branch, which hands the same acquire fence fd to both `returnSourceFence` and `ASurfaceTransaction_setBuffer`. Give the fd to one owner only: either skip the `returnSourceFence` call there or `dup` the fd for the transaction. This path becomes the default on affected devices with this change.</comment>
<file context>
@@ -476,8 +476,11 @@ private void pushCpuImageToNative(int windowId, Drawable drawable) {
+ // Disable swap R/B in cpu path as it is handled with drawable.
+ // An RGBA_8888 slot already holds R,G,B,A bytes: SurfaceFlinger takes it as is, and the
+ // BGRA-to-RGBA blit (which swaps an RGBA-declared source) must not touch it.
+ boolean convert = sfCompatMode && !g.isScanoutRgba();
+ nativeSetWindowBuffer(windowId, ahbPtr, acquireFence, 0, 0, g, g.getLastUsedSlot(), convert);
if (hudRef != null && skipFPSCount.get() >= 1) {
</file context>
| val useGLRenderer = isX11Display && (container.graphicsDriver == "virgl" || container.displayRenderer.equals("gl", true)) | ||
| // Before the first drawable: the SurfaceFlinger display renderer hands its window buffers to | ||
| // surfaceflinger, so they follow the same scan-out layout rule as the gamescope daemon's pool. | ||
| if (isX11Display) AHBImage.setCpuScanoutRgba(resolveScanoutLayout(container) == "rgba") |
There was a problem hiding this comment.
P2: The X11 path resolves the layout via nativeScanoutLayout() before any nativeStart, which the function's KDoc says is only valid after start. Confirm the native side returns the device rule when no session is running. If it does not, the RGBA selection never takes effect on X11.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt, line 2136:
<comment>The X11 path resolves the layout via `nativeScanoutLayout()` before any `nativeStart`, which the function's KDoc says is only valid after start. Confirm the native side returns the device rule when no session is running. If it does not, the RGBA selection never takes effect on X11.</comment>
<file context>
@@ -2128,6 +2131,9 @@ fun XServerScreen(
val useGLRenderer = isX11Display && (container.graphicsDriver == "virgl" || container.displayRenderer.equals("gl", true))
+ // Before the first drawable: the SurfaceFlinger display renderer hands its window buffers to
+ // surfaceflinger, so they follow the same scan-out layout rule as the gamescope daemon's pool.
+ if (isX11Display) AHBImage.setCpuScanoutRgba(resolveScanoutLayout(container) == "rgba")
val xServerViewInstance: XServerRendererView = if (useGLRenderer) {
XServerViewGL(context, xServerToUse)
</file context>
| /* HAL_PIXEL_FORMAT_RGBA_8888: the only 32-bit layout every SurfaceFlinger render engine can | ||
| * wrap; AHARDWAREBUFFER_FORMAT_R8G8B8A8_UNORM in the NDK header. */ |
There was a problem hiding this comment.
P3: This comment states an unverified universal claim as fact. Describe the layout as the one this renderer uses for CPU scan-out slots, and drop the claim about every render engine.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/main/cpp/asurfacerenderer/ahbimage.c, line 14:
<comment>This comment states an unverified universal claim as fact. Describe the layout as the one this renderer uses for CPU scan-out slots, and drop the claim about every render engine.</comment>
<file context>
@@ -11,6 +11,9 @@
#define LOGD(...) __android_log_print(ANDROID_LOG_DEBUG, LOG_TAG, __VA_ARGS__)
#define AHARDWAREBUFFER_FORMAT_B8G8R8A8_UNORM 5
+/* HAL_PIXEL_FORMAT_RGBA_8888: the only 32-bit layout every SurfaceFlinger render engine can
+ * wrap; AHARDWAREBUFFER_FORMAT_R8G8B8A8_UNORM in the NDK header. */
+#define AHB_FORMAT_RGBA_8888 1
</file context>
| /* HAL_PIXEL_FORMAT_RGBA_8888: the only 32-bit layout every SurfaceFlinger render engine can | |
| * wrap; AHARDWAREBUFFER_FORMAT_R8G8B8A8_UNORM in the NDK header. */ | |
| /* HAL_PIXEL_FORMAT_RGBA_8888: the layout this renderer uses for CPU scan-out slots. Rendering | |
| * with BGRA_8888 is unsupported on some devices (Android 16 with Skia GLES). */ |
|
Verified on the Odin 3 (Adreno 830, Android 15, rule says bgra) with this build, Celeste container set to X11 + SurfaceFlinger renderer + compat off, Open container (wfm.exe on the 1280x720 desktop):
Screenshots pixel-identical at the sampled points (desktop blue 2,119,189; taskbar 96,125,139; window background 245,245,245 in both), so the R/B swap on the copy is right. Container configs restored afterwards. The surfaceflinger abort itself can only be confirmed on the Astra. |
Description
The SurfaceFlinger display renderer hands each X window's CPU swapchain slot to surfaceflinger as a BGRA_8888 AHardwareBuffer. Android 16 on a Skia GLES render engine cannot wrap one and aborts surfaceflinger. The gamescope daemon already answers this with an RGBA_8888 pool chosen by a device rule (SDK 36 and a GLES render engine,
GNDRM_SCANOUT_LAYOUToverride); the X11 path had no equivalent.Report: RedMagic Astra (nubia NP05J, Android 16), GameNative 1.4.0 (24). Play works since the daemon's RGBA layout; "Open container" (an X11 session with the SurfaceFlinger renderer) restarts the whole UI:
Unable to generate SkImage ... GrGLTextureInfo fFormat 32856 colorType 6on a 1280x720 buffer, 30-40 ms after the wfm.exe window maps.The renderer's slots now follow the daemon's rule, read through
GameNativeDrm.nativeScanoutLayout()(valid without a session) before the first drawable, with the container'sGNDRM_SCANOUT_LAYOUToverride honoured the same way:AHBImage: the three scan-out slots are RGBA_8888 under the rule; the X server's B,G,R,A bytes are swapped on the CPU copy into the slot. The front buffer the X server writes into stays BGRA (never handed to surfaceflinger).ASurfaceRenderer: an RGBA slot goes to surfaceflinger as is, skipping the BGRA-to-RGBA blit, which swaps an RGBA-declared source and would undo the copy.XServerScreen: resolves the layout for X11 sessions and logs it (X11 scan-out layout rgba (device rule)).libahbimage.sorebuilt from the changedahbimage.cwith NDK 27.3.13750724 (the asurfacerenderer libs ship prebuilt).BGRA devices are unchanged: same formats, same copy, same blit.
Recording
Not applicable: the reporter's device is the only one that reproduces it (Android 16 + Skia GLES). Not yet confirmed on it.
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
Fixes a SurfaceFlinger abort when the X11 path hands BGRA_8888 buffers to surfaceflinger on Android 16 devices with a Skia GLES render engine. The renderer's CPU swapchain slots now follow the same RGBA_8888 scan-out layout rule the gamescope daemon applies, resolved before the first drawable from the container's
GNDRM_SCANOUT_LAYOUToverride or the device rule.libahbimage.sorebuilt from the modified C source with NDK 27.3.Written for commit 4363185. Summary will update on new commits.
Summary by CodeRabbit