Repository navigation
fix(agc): keep sRGB storage images in their UNORM twin on every driver - #10859
Conversation
StorageFormatOrUndefined kept an 8-bit sRGB storage texture in its sRGB format whenever the driver reported storage support for it, and fell back to the UNORM twin only otherwise. Shader stores to an sRGB storage image are sRGB-encoded, so a guest store of a raw code landed encoded. MoltenVK 1.4.2 reports storage support for these formats: agc_driver_image_srgb_sample_copy and its forced variant sampled the encoded value, and agc_driver_recover_srgb_allocation failed because CanSrgbExpandFrom refused a storage format that differs from the sampled UNORM format. The five 8-bit sRGB formats now always use their UNORM twin for storage. Co-authored-by: Ycaro Gabriel <280482597+ycarogabriel-lgtm@users.noreply.github.com> Co-authored-by: Himansh Raj <67262056+iamthehimansh@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 290f9eed01224dd071752c4ad0cd0657d772602b against current main.
The five 8-bit sRGB formats now select their UNORM twin before querying the sRGB format's storage support, while the existing packed-format branch remains first. This matches Vulkan's image-write rule: stores to sRGB formats apply linear-to-sRGB encoding, whereas the guest storage path needs the raw encoded byte values. Using the UNORM twin avoids the extra encode and also lets the sRGB expansion path recognize the source format. If the twin itself lacks storage support, returning VK_FORMAT_UNDEFINED is preferable to using an sRGB view with different write semantics. The Vulkan spec documents this in Texel Encode.
The PR reports same-composition MoltenVK controls with three runs per side: the three targeted regressions fail without the change and pass with it; the unrelated #9125 failure and the same validation VUID set remain on both sides. I did not independently run the GPU tests. Hosted eligibility, conventions, intake, labels and report checks pass; product jobs are skipped by repository policy. Linux and Windows are not reported as tested. I found no blocking issue in the format-selection change.
AI-assisted review: yes.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
Windows check on three non-Apple drivers.
The three are |
…0859 imellag's earlier boykopovar#10676 already makes PendingList::published build with Apple's libc++, and maximilianfv's earlier boykopovar#10859 keeps sRGB storage images in their UNORM twin on every driver, which covers MoltenVK. Drop both hunks here and depend on those PRs.
jotitadev
left a comment
There was a problem hiding this comment.
A second Apple GPU, on a main-based tree, with both MoltenVK builds.
Machine: Apple M2 Max, macOS 27.2, x86_64 Release build under Rosetta 2.
Tree: main b986c11a34 + the open macOS PRs needed to build and open a device (#10366, #7631, #10659, #10660, #11249, #11358, #10669, #10552, #7804, #10676, #11447, #11437, #11554, #11705) + this PR at 290f9eed01 (clean merge). Control: the same tree without this PR. ANYPS5_NO_SHADER_CACHE=1, -j1 --timeout 30.
Drivers: MoltenVK 1.4.2 (LunarG SDK 1.4.363.0) and MoltenVK main + SPIRV-Cross #2704.
ctest -R srgb (7 tests) |
Without | With (3 runs) |
|---|---|---|
| MoltenVK 1.4.2 | image_srgb_sample_copy and _forced fail |
7/7 pass (1 skip, see below), 3/3 |
| MoltenVK main + #2704 | same 2 fail | same, 3/3 |
- Full
^agcsuite, 1037 tests, MoltenVK 1.4.2: 41 failures without, 39 with. The two sRGB tests are the only difference, so nothing new fails. - Validation layer (
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, SDK 1.4.363.0, loader output shows it inserted) on the same 7 tests: the same single VUID on both sides,VUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605×2 fromagc_driver_srgb8_color_target, which this PR does not touch. No newVUID-. agc_driver_recover_srgb_allocationcannot be checked on amain-based macOS tree yet: it runs fromagc_driver_sampled_cache_budget_tests, which loadslibvulkan.so.1, so it reports "skipped, no usable Vulkan device: AGC graphics: cannot load Vulkan" (and fails with that message underANYPS5_REQUIRE_VULKAN=1) with and without this PR. Your 64→67 result for it came from the series tree with your extra test fixes; that file is one of the ones #10920 touches.
GPUs tested: Apple M2 Max (MoltenVK 1.4.2 and MoltenVK main + #2704)
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
✅ NVIDIA GPU test: passedPR head
Failing on main too (3, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (4 IDs in 397 tests)Skipped (40)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
What
StorageFormatOrUndefined(libSceAgcDriver/Graphics/src/Texture.cpp) kept an 8-bit sRGB texture in its sRGB format for storage whenever the driver reportedVK_FORMAT_FEATURE_STORAGE_IMAGE_BITfor it, and used the UNORM twin only as a fallback. The Vulkan spec applies a linear-to-sRGB encode when a shader stores to an sRGB storage image (Texel Encode), so a guest store of raw code 8 lands as 49:agc_driver_image_srgb_sample_copyand its forced variant sample the decode of 49.agc_driver_recover_srgb_allocationfails differently:CanSrgbExpandFromrequires the storage format to equal the sampled UNORM format, so it refuses the sRGB storage surface as the source of an expansion copy.Drivers that report no storage support for these formats took the UNORM fallback and never hit this. MoltenVK 1.4.2 reports it for
R8_SRGB,R8G8_SRGB,R8G8B8A8_SRGB,B8G8R8A8_SRGBandA8B8G8R8_SRGB_PACK32, and is conformant in doing so; the bug is that the code relied on the driver lacking sRGB storage.The five 8-bit sRGB formats now always use their UNORM twin for storage, before the sRGB format itself is checked. Packed formats keep their branch first. With no platform guard, drivers without sRGB storage take the same path as before. A driver that supports sRGB storage but not storage for the UNORM twin now gets
VK_FORMAT_UNDEFINEDinstead of the sRGB format.Prior art, credited with
Co-authored-bytrailers:#ifdef __APPLE__, keeps sRGB-first on other platforms, and falls back to the sRGB format when the UNORM twin lacks storage support. It predates the packed-format branch.Tested
maindoes not build on macOS by itself. I tested on mugurc's macOS series (ci/macos-series-2, 27e9689) merged withmainat 522e32e, plus the other open macOS fixes (#10419 build parts, #10540, #10562, #10641) and some unopened test fixes. Only this change differed between the two sets of runs.Setup: Apple M1 Pro, macOS 27.0.1, x86_64 build under Rosetta 2, Vulkan through MoltenVK from the LunarG Vulkan SDK 1.4.363. Every run used
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationwithctest -R '^(agc_driver_.*srgb.*|agc_driver_image_.*)$' -E 'audio_in|voice' -V(68 tests), three runs with and three without this change.agc_driver_image_srgb_sample_copy,agc_driver_image_srgb_sample_copy_forcedandagc_driver_recover_srgb_allocationfail.agc_driver_image_plane_bias_on_single_level_volumealso fails, which is unrelated and covered by test(agc): skip plane-bias reduction cases without min/max sampler reduction #9125.agc_driver_image_plane_bias_on_single_level_volumefails.Validation Errorlines without and 123 with, with the same VUID set (VUID-VkDeviceCreateInfo-pProperties-04451and others, already there before this change). Neither log containswas not found but was requested by env var VK_INSTANCE_LAYERS.MultisampleTarget.cppstorage format andsameStorageForm, andStorageSrgbTwininMultisampleRetirement.cppno longer skips). Same setup,ctest -R '^(agc_.*multisample.*|agc_driver_.*msaa.*|image_msaa_load|agc_driver_srgb8_color_target|agc_driver_emulated_color_compare_srgb_decode|agc_driver_.*srgb.*)$'(36 tests), three runs each way: 30/36 without, 33/36 with. The three sRGB tests above are the only difference.agc_driver_multisample_retirement,agc_driver_native_multisample_depthandagc_driver_sample_mask_multisamplefail both ways with the same message.agc_driver_multisample_retirementstops beforeStorageSrgbTwin("the device cannot write the multisampled color target's format as a storage image at its sample count", covered by test(agc): skip the MSAA retirement storage promotion without shaderStorageImageMultisample #8604), so that check is still not exercised on this host. 63Validation Errorlines both ways, same VUID set.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code