diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index c05d988cd..1800fed72 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -224,3 +224,9 @@ jobs: - name: Pin-bump gate unit tests run: python3 scripts/tests/test_downstream_pin_bump.py + + - name: App-linter INV-4.6 colour-swapchain unit tests + # Hermetic (synthetic app trees). Pins the per-leg _SRGB-first check that + # would have caught the gauss demo's Android leg preferring UNORM after + # #1589 made UNORM swapchains linear. + run: python3 scripts/tests/test_check_displayxr_app_color.py diff --git a/docs/guides/displayxr-app-rules.md b/docs/guides/displayxr-app-rules.md index 7cad58d05..4e3485bb0 100644 --- a/docs/guides/displayxr-app-rules.md +++ b/docs/guides/displayxr-app-rules.md @@ -480,9 +480,11 @@ re-implementing — see [INV-8.1](#8-app-folder-layout--what-to-include)). `eyeCount = 1`, render at full window resolution, center-eye = average of the located eyes. The runtime accepts `viewCount == 1` only in a 2D/non-3D mode. Ref: `main.cpp:415,632-644,707-711`. -- **INV-4.6 — Color space: request an sRGB swapchain.** The display panel is sRGB, and an - in-process native compositor is a **byte passthrough to the panel** — it does *not* color-manage - a linear-format swapchain. So the encoding of the bytes you store is what reaches the display. +- **INV-4.6 — Color space: request an sRGB swapchain.** The display panel is sRGB, and the + runtime reads a swapchain the way OpenXR says to: an `*_SRGB` format holds **encoded** colour and + reaches the panel as stored; every other format "will be treated as linear values" and is + **sRGB-encoded** on the way to the panel (#1589). So what you store must match the format you + asked for. - **Do this — request an sRGB swapchain format:** `..._UNORM_SRGB` (D3D11/D3D12), `GL_SRGB8_ALPHA8` (GL), `..._SRGB` (Vulkan), `MTLPixelFormat*sRGB` (Metal). Then you are correct **either way you render into it**: @@ -499,15 +501,22 @@ re-implementing — see [INV-8.1](#8-app-folder-layout--what-to-include)). scratch image of the swapchain's UNORM sibling, then `vkCmdCopyImage` into the swapchain (a copy never converts). This is byte-exact on any runtime; - clear with the linearized colour (sRGB EOTF), not the display-referred one. - - **A linear / UNORM swapchain (`R8G8B8A8_UNORM`, `GL_RGBA8`, …) is NOT color-managed** on the - in-process path: the compositor passes the bytes straight through with no linear→sRGB encode. - It's only correct if you write display-referred bytes into it; writing genuinely-linear values - and expecting the runtime to encode → **too bright**. ("Submit a linear-format swapchain and - the runtime color-manages it" is a future runtime follow-up, not current behavior.) **Subject to - change — see [#409](https://github.com/DisplayXR/displayxr-runtime/issues/409):** if the color - model lands on linear-compose ("Model B"), a linear-format swapchain *would* be honored as - linear (the runtime would encode at the DP boundary). This bullet describes current ("Model A") - behavior; the recommendation above (request an sRGB swapchain) stays correct either way. + - **A linear / UNORM swapchain (`R8G8B8A8_UNORM`, `GL_RGBA8`, …) holds LINEAR values** and the + runtime encodes it. Display-referred (already gamma-encoded) bytes stored in one are encoded a + **second** time → **washed out**: lifted blacks, desaturated colour (a splat demo's authored + `(229,182,127)` reaches the atlas as `(243,220,187)`). This is the #1589/#1610 format-honest + colour model, shipped per backend — D3D11 in v2.21.0, D3D12 v2.21.1, GL v2.21.2, **Vulkan + (`vk_native`, incl. Android) v2.21.7**; Metal still passes UNORM bytes through. Before those + releases a UNORM swapchain *was* a byte passthrough, which is why an app that picked UNORM and + wrote display-referred bytes looked right until it met a newer runtime — including a leg that + lagged its siblings (an Android leg that still preferred `{R8G8B8A8_UNORM, B8G8R8A8_UNORM}` + after the desktop legs had moved to `_SRGB`). **Every leg of a multi-platform app must make the + same `_SRGB`-first choice** — route it through one shared helper rather than a per-leg + preference list; `scripts/check_displayxr_app.py` flags a UNORM-first preference per leg. + - **A/B on a runtime:** `DXR_COLOR_LEGACY_UNORM_ENCODED=1` restores the old passthrough reading + of UNORM (on Android: `adb shell setprop debug.xrt.DXR_COLOR_LEGACY_UNORM_ENCODED 1`, then + relaunch the app). If that makes a washed-out app look right, the app is storing + display-referred bytes in UNORM — fix the app, the knob is a diagnostic, not a setting. - **Data textures (normal, AO, roughness, metalness) are ALWAYS linear — never sRGB.** Only albedo/color is sRGB; sample those through an `_SRGB` texture view (or decode in-shader) so lighting runs in linear, then write your result into the sRGB swapchain per above. @@ -1047,7 +1056,7 @@ macOS app (no manifest → no Android findings). - [ ] View configuration chosen at startup: N-view app begins `PRIMARY_MULTIVIEW_DXR` (when enumerated), stereo-fixed app stays on `PRIMARY_STEREO`; `xrLocateViews` into an 8-wide buffer; **render** `eyeCount` from the active mode, not 2 (INV-3.1) - [ ] Projection layer **submits the LOCATED count**, with the inactive tail aliased via `DxrAliasInactiveViews()` — including every 3D zone layer (INV-3.4) - [ ] App swapchain sized once to worst-case atlas (INV-4.2); per-tile = window/canvas × scaleXY, never display (INV-4.3) -- [ ] Color space: request an **sRGB swapchain** and write a correctly-encoded image (linear render + GPU sRGB-write, or display-referred bytes — not both); linear/UNORM swapchain is not color-managed; data textures always linear (INV-4.6) +- [ ] Color space: request an **sRGB swapchain** and write a correctly-encoded image (linear render + GPU sRGB-write, or display-referred bytes — not both); a linear/UNORM swapchain is read as LINEAR and encoded by the runtime (display-referred bytes in it wash out); same `_SRGB`-first choice on every leg; data textures always linear (INV-4.6) - [ ] Whole declared `imageRect` is written — partial-tile renders clear the full tile to `(0,0,0,0)` first (or shrink the rect); no undefined pixels reach the atlas, esp. transparent-bg (INV-4.7) - [ ] (if `XR_EXT_view_configuration_views_change` is enabled) the handler re-enumerates and **moves `subImage.imageRect`** — no `xrCreateSwapchain` from the event; filters on `systemId` **and** `viewConfigurationType` (INV-4.9) - [ ] (texture) regions declared via display-zones — 3D zones + Local2D zones, not output-rect/surround (INV-5.1/5.3) diff --git a/scripts/check_displayxr_app.py b/scripts/check_displayxr_app.py index 1b41de641..923c8f8c6 100755 --- a/scripts/check_displayxr_app.py +++ b/scripts/check_displayxr_app.py @@ -42,7 +42,7 @@ # Directories never linted: shared reference code, vendored headers, build output. EXCLUDE_DIRS = { "build", ".git", "third_party", "openxr_includes", "common", - "_package", "__pycache__", "node_modules", ".vs", "out", + "_package", "__pycache__", "node_modules", ".vs", "out", ".claude", } SOURCE_EXTS = {".c", ".cc", ".cpp", ".cxx", ".h", ".hh", ".hpp", ".m", ".mm"} # JVM sources are linted for the Android rules only (§11) — the C/C++ checks @@ -56,7 +56,7 @@ "INV-3.1": "An N-view app BEGINS XR_VIEW_CONFIGURATION_TYPE_PRIMARY_MULTIVIEW_DXR (when enumerated; needs XR_DXR_display_info), locates into an XRT_MAX_VIEWS (8)-wide buffer and submits the active mode's viewCount. A stereo-fixed app stays on PRIMARY_STEREO and receives exactly 2. Deriving eyeCount from the rendering mode's viewCount without the opt-in is an error: PRIMARY_STEREO reports 2 and rejects viewCount>2.", "INV-3.4": "A projection layer carries the LOCATED view count (what xrLocateViews returned), for every view configuration type. Render only the active mode's views, then alias the inactive tail [active, located) onto view 0's subImage keeping each view's own located pose/fov (DxrAliasInactiveViews in dxr_view_config.h: test_apps/common in-tree, displayxr-common outside). Checked per leg: every platform directory of a multi-leg app must call it itself. Submitting the ACTIVE count is under-submit and xrEndFrame now refuses it (ADR-041). A 3D zone layer is a projection layer and obeys the same rule.", "INV-4.3": "Per-tile render size = window/canvas x scaleXY, never display size.", - "INV-4.6": "Request an sRGB swapchain (and store a correctly-encoded image); don't double-encode.", + "INV-4.6": "Request an sRGB swapchain (and store a correctly-encoded image); don't double-encode. A UNORM swapchain is read as LINEAR and encoded by the runtime (#1589), so display-referred bytes in it wash out. Checked per leg: every platform directory must choose _SRGB first.", "INV-4.7": "Write every pixel of the imageRect you declare — clear partial-tile renders to (0,0,0,0) first (or shrink the rect); undefined pixels read as opaque magenta on MoltenVK and break transparent-bg.", "INV-4.9": "An app that enables XR_EXT_view_configuration_views_change must not call xrCreateSwapchain from its event handler — move subImage.imageRect instead (an app sized at maxImageRect* per ADR-010 never needs to reallocate).", "INV-5.9": "VK apps MUST use XR_KHR_vulkan_enable2 (the runtime creates the VkDevice via xrCreateVulkanDeviceKHR); an app-side vkCreateDevice = enable1, which forfeits the #868 weave-rate decoupling and the late-weave pacing.", @@ -156,6 +156,27 @@ def __init__(self, level, rule, path, line, msg, fix): re.IGNORECASE, ) CREATES_SWAPCHAIN = re.compile(r"\bxrCreateSwapchain\b") +ENUMERATES_SWAPCHAIN_FORMATS = re.compile(r"\bxrEnumerateSwapchainFormats\b") +# INV-4.6, the per-leg shapes of "this leg prefers UNORM" (#1589). Since the +# format-honest colour model (D3D11 v2.21.0 ... vk_native v2.21.7) an UNORM +# swapchain holds LINEAR values and is sRGB-encoded on the way to the panel, so +# an app that picks UNORM and stores display-referred bytes is encoded twice — +# washed out. Every leg used to get away with it because UNORM was a byte +# passthrough; the one that did not get migrated (an Android leg preferring +# {R8G8B8A8_UNORM, B8G8R8A8_UNORM} after its desktop siblings moved to _SRGB) +# is what these catch. Only checked in files that enumerate swapchain formats. +_UNORM8 = r"(?:VK_FORMAT|DXGI_FORMAT)_[RB]8G8[RB]8A8_UNORM\b" +# (a) a preference list naming only 8-bit UNORM colour formats (2+ entries): +# const int64_t preferred[] = {VK_FORMAT_R8G8B8A8_UNORM, VK_FORMAT_B8G8R8A8_UNORM}; +UNORM_ONLY_PREFERENCE_LIST = re.compile( + r"\{\s*(?:\(\s*\w+\s*\)\s*)?" + _UNORM8 + + r"(?:\s*,\s*(?:\(\s*\w+\s*\)\s*)?" + _UNORM8 + r")+\s*,?\s*\}" +) +# (b) a scan loop that STOPS at the first UNORM it meets: +# if (f == VK_FORMAT_B8G8R8A8_UNORM || ...) { selected = f; break; } +UNORM_FIRST_BREAK = re.compile( + r"==\s*" + _UNORM8 + r"[^;{}]*\)\s*\{[^{}]*\bbreak\b" +) # INV-3.1 (#1486). Two different questions, two different markers: # # MULTIVIEW_OPT_IN — does the app BEGIN the N-view view configuration? Only a @@ -291,7 +312,6 @@ def scan_sources(root: Path, findings: list): stereo_fixed_marked = True stereo_fixed_legs.add(leg_of(path, root)) is_multiview_app = any(MULTIVIEW_OPT_IN.search(t) for _, t in files) - any_srgb = any(SRGB_TOKENS.search(t) for _, t in files) # INV-3.1 (#1486): mode-derived view counts WITHOUT the PRIMARY_MULTIVIEW_DXR # opt-in. The app reads the active mode's viewCount (so it can reach 4) but @@ -375,7 +395,6 @@ def scan_sources(root: Path, findings: list): "3D zone layers are projection layers and need the same treatment, per zone.", )) - swapchain_loc = None for path, text in files: for regex, level, rule, msg, fix, multiview_only in SRC_PATTERNS: if multiview_only and not is_multiview_app: @@ -383,10 +402,6 @@ def scan_sources(root: Path, findings: list): for m in regex.finditer(text): line_no = text.count("\n", 0, m.start()) + 1 findings.append(Finding(level, rule, rel(path, root), line_no, msg, fix)) - if swapchain_loc is None: - m = CREATES_SWAPCHAIN.search(text) - if m: - swapchain_loc = (rel(path, root), text.count("\n", 0, m.start()) + 1) # INV-5.9 (enforced): a VK app that creates its own VkDevice is using # XR_KHR_vulkan_enable (enable1). enable1 forfeits the runtime-owned VkQueue @@ -411,14 +426,76 @@ def scan_sources(root: Path, findings: list): "app-side vkCreateInstance / vkCreateDevice). Reference: test_apps/handle/cube_handle_vk_win.", )) - # INV-4.6 advisory: creates a swapchain but no sRGB format appears anywhere. - if swapchain_loc and not any_srgb: - p, ln = swapchain_loc + check_color_swapchain_per_leg(files, root, findings) + + +def check_color_swapchain_per_leg(files, root: Path, findings: list): + """INV-4.6, per leg. A multi-leg app is only as right as its worst leg: the + shared/single group ("") counts toward every leg, but one leg's _SRGB choice + does not cover another's (the Android leg of a splat demo kept preferring + UNORM for months after its desktop legs moved to _SRGB, and an app-wide + "any sRGB token anywhere" check was satisfied by the desktop legs).""" + fix_unorm = ( + "Choose the first _SRGB format the runtime enumerates (fall back to UNORM only if none), " + "the same way on every leg — ideally through one shared helper. Store display-referred " + "bytes in it by a raw copy (blit into an UNORM-sibling scratch, then vkCmdCopyImage) or " + "render linear and let the GPU encode; linearize clear colours. Since #1589 an UNORM " + "swapchain is read as LINEAR and encoded by the runtime, so display-referred bytes in it " + "come out washed out. A/B: DXR_COLOR_LEGACY_UNORM_ENCODED=1 (Android: setprop " + "debug.xrt.DXR_COLOR_LEGACY_UNORM_ENCODED 1) restores the old passthrough." + ) + by_leg = {} + for path, text in files: + by_leg.setdefault(leg_of(path, root), []).append((path, text)) + shared = by_leg.get("", []) + shared_srgb = any(SRGB_TOKENS.search(t) for _, t in shared) + app_srgb = any(SRGB_TOKENS.search(t) for _, t in files) + + for leg, leg_files in sorted(by_leg.items()): + # (a)/(b): a UNORM-first choice, wherever the leg enumerates formats. + for path, text in leg_files: + if not ENUMERATES_SWAPCHAIN_FORMATS.search(text): + continue + for regex, what in ( + (UNORM_ONLY_PREFERENCE_LIST, "a swapchain-format preference list naming only UNORM formats"), + (UNORM_FIRST_BREAK, "a swapchain-format scan that stops at the first UNORM format"), + ): + for m in regex.finditer(text): + findings.append(Finding( + WARN, "INV-4.6", rel(path, root), text.count("\n", 0, m.start()) + 1, + f"UNORM-first colour swapchain choice ({what}). An UNORM swapchain is " + "read as LINEAR and encoded by the runtime (#1589): display-referred " + "bytes stored in it are encoded twice and look washed out.", + fix_unorm, + )) + # The leg creates a swapchain but neither it nor the shared code ever + # names an sRGB format. Judged per leg only where the leg CHOOSES the + # format (enumerates it); a leg that creates swapchains in a format + # negotiated elsewhere (displayxr-common's session code, "reuse the + # main format") falls back to the app-wide test the check always had. + if leg == "" and len(by_leg) > 1: + continue # shared code alone is not a leg + loc = None + for path, text in leg_files: + m = CREATES_SWAPCHAIN.search(text) + if m: + loc = (rel(path, root), text.count("\n", 0, m.start()) + 1) + break + if loc is None: + continue + leg_chooses = any(ENUMERATES_SWAPCHAIN_FORMATS.search(t) for _, t in leg_files) + if leg_chooses: + if any(SRGB_TOKENS.search(t) for _, t in leg_files) or shared_srgb: + continue + elif app_srgb: + continue findings.append(Finding( - WARN, "INV-4.6", p, ln, - "No sRGB swapchain format detected — INV-4.6 recommends an sRGB swapchain.", + WARN, "INV-4.6", loc[0], loc[1], + "No sRGB swapchain format detected" + (f" in the {leg}/ leg" if leg else "") + + " — INV-4.6 recommends an sRGB swapchain.", "Request an sRGB swapchain (_UNORM_SRGB / GL_SRGB8_ALPHA8 / _SRGB / MTLPixelFormat*sRGB). " - "A UNORM swapchain is valid ONLY if you store display-referred (already-encoded) bytes.", + "A UNORM swapchain is read as LINEAR and encoded by the runtime (#1589); it is valid " + "only if you store scene-linear values in it.", )) diff --git a/scripts/tests/test_check_displayxr_app_color.py b/scripts/tests/test_check_displayxr_app_color.py new file mode 100644 index 000000000..cd6254512 --- /dev/null +++ b/scripts/tests/test_check_displayxr_app_color.py @@ -0,0 +1,138 @@ +#!/usr/bin/env python3 +"""Unit tests for the INV-4.6 colour-swapchain checks in scripts/check_displayxr_app.py. + +Hermetic: each case writes a tiny synthetic app tree to a temp dir and lints it. + +The shapes are taken from real demo legs. Since the #1589 format-honest colour +model (vk_native in v2.21.7) an UNORM swapchain is read as LINEAR and encoded by +the runtime, so a leg that picks UNORM and stores display-referred bytes is +encoded twice and looks washed out. The Android leg of the Gaussian-splat demo +did exactly that through v1.29.0 while its desktop legs had moved to _SRGB, and +the old app-wide "any sRGB token anywhere" check was satisfied by the desktop +legs, so nothing flagged it. + + python3 scripts/tests/test_check_displayxr_app_color.py +""" +from __future__ import annotations + +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) +import check_displayxr_app as lint # noqa: E402 + +ENUM = "xrEnumerateSwapchainFormats(s, n, &n, formats);\nxrCreateSwapchain(s, &ci, &sc);\n" + +# The Android leg of displayxr-demo-gaussiansplat v1.29.0 (android/src/main/cpp/main.cpp:923). +ANDROID_UNORM_LIST = ENUM + """ +const int64_t preferred[] = {VK_FORMAT_R8G8B8A8_UNORM, VK_FORMAT_B8G8R8A8_UNORM}; +for (int64_t pref : preferred) { for (uint32_t i = 0; i < n; ++i) if (formats[i] == pref) fmt = pref; } +""" + +# A desktop leg that stops at the first UNORM (displayxr-demo-avatar v0.14.0 macos/main.mm). +UNORM_FIRST_BREAK = ENUM + """ +for (auto f : fmts) { + if (f == VK_FORMAT_B8G8R8A8_UNORM || f == VK_FORMAT_R8G8B8A8_UNORM) { selectedFmt = f; break; } + if (f == VK_FORMAT_B8G8R8A8_SRGB || f == VK_FORMAT_R8G8B8A8_SRGB) selectedFmt = f; +} +""" + +# The correct shape: stop at the first _SRGB, remember a UNORM only as a fallback. +SRGB_FIRST_BREAK = ENUM + """ +for (auto f : fmts) { + if (f == VK_FORMAT_B8G8R8A8_SRGB || f == VK_FORMAT_R8G8B8A8_SRGB) { selectedFmt = f; break; } + if (f == VK_FORMAT_B8G8R8A8_UNORM || f == VK_FORMAT_R8G8B8A8_UNORM) selectedFmt = f; +} +""" + +# Choosing through a shared helper (no format token in the leg itself). +VIA_HELPER = ENUM + "fmt = gsChooseSwapchainFormat(formats, n);\n" + +# A mutable-format list on an intermediate image, not a swapchain preference. +MUTABLE_LIST = ENUM + """ +const VkFormat view_formats[] = {VK_FORMAT_R8G8B8A8_UNORM, VK_FORMAT_R8G8B8A8_SRGB}; +""" + +SHARED_HELPER = """ +bool gsIsSrgbFormat(VkFormat f) { return f == VK_FORMAT_R8G8B8A8_SRGB || f == VK_FORMAT_B8G8R8A8_SRGB; } +""" + + +def lint_tree(files: dict[str, str]) -> list: + with tempfile.TemporaryDirectory() as d: + root = Path(d) + for rel, text in files.items(): + p = root / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(text) + findings: list = [] + lint.scan_sources(root, findings) + return [f for f in findings if f.rule == "INV-4.6"] + + +class Inv46PerLeg(unittest.TestCase): + def test_android_unorm_list_flagged_although_desktop_legs_are_srgb(self): + got = lint_tree({ + "android/src/main/cpp/main.cpp": ANDROID_UNORM_LIST, + "macos/main.mm": SRGB_FIRST_BREAK, + "linux/main.cpp": SRGB_FIRST_BREAK, + "3dgs_common/gs_vulkan_utils.cpp": SHARED_HELPER, + }) + self.assertEqual([(f.path, f.line) for f in got], [("android/src/main/cpp/main.cpp", 4)]) + self.assertIn("preference list naming only UNORM", got[0].msg) + + def test_unorm_first_break_flagged(self): + got = lint_tree({"macos/main.mm": UNORM_FIRST_BREAK}) + self.assertEqual(len(got), 1) + self.assertIn("stops at the first UNORM", got[0].msg) + + def test_fixed_tree_is_clean(self): + got = lint_tree({ + "android/src/main/cpp/main.cpp": VIA_HELPER, + "macos/main.mm": VIA_HELPER, + "linux/main.cpp": VIA_HELPER, + "3dgs_common/gs_vulkan_utils.cpp": SHARED_HELPER, + }) + self.assertEqual(got, []) + + def test_srgb_first_scan_is_clean(self): + self.assertEqual(lint_tree({"linux/main.cpp": SRGB_FIRST_BREAK}), []) + + def test_mutable_format_list_is_not_a_preference(self): + self.assertEqual(lint_tree({"linux/main.cpp": MUTABLE_LIST}), []) + + def test_leg_with_no_srgb_anywhere_is_named(self): + got = lint_tree({ + "android/src/main/cpp/main.cpp": ENUM, + "macos/main.mm": SRGB_FIRST_BREAK, + }) + self.assertEqual(len(got), 1) + self.assertEqual(got[0].path, "android/src/main/cpp/main.cpp") + self.assertIn("android/ leg", got[0].msg) + + def test_leg_reusing_a_negotiated_format_is_not_flagged(self): + # displayxr-demo-avatar windows/main.cpp: a zone swapchain created with + # `ci.format = xr->swapchain.format` (chosen by displayxr-common). + got = lint_tree({ + "windows/main.cpp": "ci.format = xr->swapchain.format;\nxrCreateSwapchain(s, &ci, &sc);\n", + "macos/main.mm": SRGB_FIRST_BREAK, + }) + self.assertEqual(got, []) + + def test_single_leg_app_without_srgb(self): + got = lint_tree({"main.cpp": ENUM}) + self.assertEqual(len(got), 1) + self.assertIn("No sRGB swapchain format detected", got[0].msg) + + def test_commented_out_unorm_list_is_ignored(self): + got = lint_tree({ + "linux/main.cpp": VIA_HELPER + "// was: {VK_FORMAT_R8G8B8A8_UNORM, VK_FORMAT_B8G8R8A8_UNORM}\n", + "shared/util.cpp": SHARED_HELPER, + }) + self.assertEqual(got, []) + + +if __name__ == "__main__": + unittest.main(verbosity=2)