Repository navigation
Tidy the Metal seams, name the renderer's literals and test the iOS mirrors - #41
Merged
Merged
Conversation
…e home in layout/ The slot count was declared three times (MetalVisibilityPass::kSlots, MetalFrameCompute::kSlots, MetalSplatRenderer::kFramesInFlight) and the radix sort owned the key width enum, so cull/ included sort/ and raster/ included cull/ only to read constants. kFramesInFlight, KeyBits and kDrawBatches/kDrawArgumentBytes now live in layout/MetalFrameSlots.h, MetalSortKeys.h and MetalDrawArguments.h. Every per-slot array derives from kFramesInFlight, the cull -> sort and raster -> cull includes are gone, and the test-local copies of the slot count in MetalSurfaceTest are dropped. MetalFrameLoopTest keeps its own count because it acquires exactly two slots by hand. Behaviour and shader text are unchanged.
…ure limits MetalSurface clamps the render scale with kMinRenderScale and kMaxRenderScale, the same names VulkanSurface uses. MetalSplatRenderer reports its residency window as kMinResidencyCapacity and kMaxResidencyCapacity (the Android names), the LOD error range as kMinLodErrorPixels and kMaxLodErrorPixels, and the texture ceiling as kAppleFamily7MaxTextureDimension. The values are unchanged.
Nothing the header declares comes from GpuLayout.h; the sources that need its records include it themselves.
The test writes a 3x2 BGRA image, decodes the file with ImageIO and checks its size and every channel. A zero size, a buffer one byte short or long, and an unwritable path all return false, and the first two leave no file.
…tructs Policy round trips both sort depths, and toCppPolicy rejects a sort depth of 24 and leaves the output untouched. Policy support, stats, capabilities, camera state, camera request and lens settings each check every field against distinct values.
… flight Fifty renderers each draw both frame slots on a 4096-leaf hierarchy, so a selection, a sort and a render buffer are in flight per slot, and are destroyed straight away. The test is a regression guard for the completion handlers that capture pointers into MetalFrameCompute; it needs an AddressSanitizer build to catch a late handler.
The selection and sort completion handlers wrote through raw pointers into MetalFrameCompute and MetalFrameLoop, and ~MetalSplatRenderer waits only on the frame slots that the render buffer's handler returns. Apple documents no order between one command buffer's completion handlers and another's, so a handler can still be running after the renderer has gone. In practice the driver ran them in commit order here: a 30 ms sleep in the sort handler under AddressSanitizer did not fail the regression test, which therefore passes before and after this change. The handlers now hold a share of a GpuReport that carries the counters, and of the frame loop's failure latch, so nothing they touch can be freed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Metal renderer's seams follow the folder rules, its literals have names, and the Swift-facing mirrors have tests.
MetalSurfacenames its render-scale clamp, andMetalSplatRenderernames the residency, LOD error and texture ceilings, matching the Android names where one exists.MetalSplatRenderer.hdrops the unusedGpuLayout.hinclude.SKTypeConversionsTest(every mirror, plustoCppPolicyrejecting a badsortDepthwithoutuntouched) andSKPngWriterTest(decodes the written PNG, rejects a zero size and a wrong-length buffer).MetalFrameCompute, and~MetalSplatRendererwaits only on the slots the render buffer's handler returns.Apple documents no order between one command buffer's completion handlers and another's (only that
waitUntilCompletedcovers its own), so a late handler was possible.The handlers now hold a share of a
GpuReportand of the frame loop's failure latch.Observed order here: a 30 ms sleep in the sort handler under AddressSanitizer did not fail the new destroy-in-flight test, so the driver runs the handlers in commit order and the test passes before and after the fix.
Evidence
cteston the macOS build passes 385 of 385 (2 fixture tests skipped for missing files); the ASan build passes the renderer, frame compute and LOD tests.scripts/lint-cpp.shclean;scripts/build-ios.shandscripts/package-ios.shsucceed; the embedded shader text hash is61ea372e20f1cd4f65eeb83e1a8da7f6327e9261, unchanged.sdk_harness.py,npm run checkandnpm run lintdid not run: nothing they cover changed.Merge Danger
Door: Two-way.
Pure renaming, includes and a handler-capture change; revert restores the old layout.
Blast Radius: iOS
Only
packages/splatkit-ioschanges; the iOS mirror export picks up the new tests and headers.