Fix Graphite/Metal GradientBlend on the iOS simulator + shader-error diagnostic API (#4555) - #4586
ramezgerges wants to merge 2 commits into
Conversation
📦 Try the packages from this PRWarning Do not run these scripts without first reviewing the code in this PR. Step 1 — Download the packages bash / macOS / Linux: curl -fsSL https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.sh | bash -s -- 4586PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4586"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4586/packages --name skiasharp-pr-4586More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4586 |
#4555 was broadened to cover all three Metal failures because they share a root cause: no CI host has a real Metal device. That was wrong for tracking. #4586 carries 'Fixes #4555' and addresses only the iOS simulator's gradient pipeline, so merging it would have silently closed the macOS shutdown hang and the Mac Catalyst GPU-family gap along with it. #4555 is back to its original scope. The other two are now #4598 (macOS virtualized Metal hangs the test host on shutdown, killing the whole leg) and #4599 (Mac Catalyst reports no MTLGPUFamily that Ganesh or Graphite accepts). Each has its own cause and its own likely fix -- an Apple Silicon pool for the first, possibly a probe change for the second -- so each closes on its own. Repointed the opt-out comments and the policy doc at whichever issue explains that leg, and updated the #4591 umbrella table. The iOS leg cites both #4598 and #4555, because the simulator has the shutdown hang *and* the gradient bug.
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
1 similar comment
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
0b92317 to
9ab7523
Compare
|
Rebased onto the current |
9ab7523 to
a12b2b3
Compare
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR fixes a Graphite/Metal iOS-simulator rendering failure (null Recorder.Snap() in the GradientBlend scene) and introduces a managed shader-error diagnostic hook by wiring a new native fShaderErrorHandler option through to SKGraphiteContext creation.
Changes:
- Adds
SKGraphiteShaderErrorHandlerDelegateand newCreateDawn/CreateMetal/CreateVulkanoverloads that accept a shader-error handler, with lifetime management via pinned delegates and a native bridge handle. - Removes the simulator-specific null-
Snap()special-casing in the Graphite/Metal visual renderer so regressions fail consistently across hosts. - Adds a Vulkan smoke test ensuring installing a shader-error handler does not break context teardown and basic submission.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/VulkanTests/GraphiteVkBackendContextTest.cs | Adds a Vulkan smoke test for the new shader-error handler overload and lifetime/teardown behavior. |
| tests/Tests/SkiaSharp/Visual/Renderers/GraphiteMetalRenderer.cs | Makes Recorder.Snap() null a hard failure on all hosts (including simulator) now that the underlying issue is fixed. |
| binding/SkiaSharp/SkiaApi.generated.cs | Adds new P/Invoke surface and struct field for sk_graphite_shader_error_handler_t and SKGraphiteContextOptions.fShaderErrorHandler. |
| binding/SkiaSharp/Gpu/Graphite/SKGraphiteContext.cs | Adds handler-overload factories and pins/tears down managed + native handler bridge resources. |
| binding/SkiaSharp/DelegateProxies.cs | Adds the managed callback proxy implementation for shader-error handler invocations. |
| binding/SkiaSharp.Skottie/SkottieApi.generated.cs | Regenerates typedefs to include the new Graphite shader-error handler type alias. |
| binding/SkiaSharp.SceneGraph/SceneGraphApi.generated.cs | Regenerates typedefs to include the new Graphite shader-error handler type alias. |
| binding/SkiaSharp.Resources/ResourcesApi.generated.cs | Regenerates typedefs to include the new Graphite shader-error handler type alias. |
mattleibow
left a comment
There was a problem hiding this comment.
Requesting changes: the shader-handler test does not verify callback delivery, and the new backend-specific test setup should be consolidated with the existing Graphite backend harness so future Graphite scenarios can run consistently across Vulkan, Metal, and Dawn.
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
5 similar comments
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
📦 Artifact size reportPackages from this PR (build Total Packages
+38 package(s) unchanged (< 50.0 KB). Per-file changes
|
| File | Size |
|---|---|
🧬 wasm 5.0.6/mt · libSkiaSharp.a (buildTransitive/netstandard1.0/libSkiaSharp.a/5.0.6/mt/libSkiaSharp.a) |
17.9 MB → 17.9 MB (🔴 +1.5 KB) |
🧬 wasm 5.0.6/st,simd · libSkiaSharp.a (buildTransitive/netstandard1.0/libSkiaSharp.a/5.0.6/st,simd/libSkiaSharp.a) |
18.2 MB → 18.2 MB (🔴 +1.5 KB) |
🧬 wasm 5.0.6/mt,simd · libSkiaSharp.a (buildTransitive/netstandard1.0/libSkiaSharp.a/5.0.6/mt,simd/libSkiaSharp.a) |
18.2 MB → 18.2 MB (🔴 +1.5 KB) |
🧬 wasm 5.0.6/st · libSkiaSharp.a (buildTransitive/netstandard1.0/libSkiaSharp.a/5.0.6/st/libSkiaSharp.a) |
17.9 MB → 17.9 MB (🔴 +1.5 KB) |
🧬 wasm 3.1.56/mt · libSkiaSharp.a (buildTransitive/netstandard1.0/libSkiaSharp.a/3.1.56/mt/libSkiaSharp.a) |
17.7 MB → 17.7 MB (🔴 +1.5 KB) |
🧬 wasm 3.1.56/st · libSkiaSharp.a (buildTransitive/netstandard1.0/libSkiaSharp.a/3.1.56/st/libSkiaSharp.a) |
17.8 MB → 17.8 MB (🔴 +1.5 KB) |
🧬 wasm 3.1.56/mt,simd · libSkiaSharp.a (buildTransitive/netstandard1.0/libSkiaSharp.a/3.1.56/mt,simd/libSkiaSharp.a) |
18.0 MB → 18.0 MB (🔴 +1.5 KB) |
🧬 wasm 3.1.56/st,simd · libSkiaSharp.a (buildTransitive/netstandard1.0/libSkiaSharp.a/3.1.56/st,simd/libSkiaSharp.a) |
18.0 MB → 18.0 MB (🔴 +1.5 KB) |
SkiaSharp.NativeAssets.WinUI
| File | Size |
|---|---|
runtimes/win-x64/native/libGLESv2.pdb |
66.6 MB → 66.5 MB (🟢 −144.0 KB) |
runtimes/win-arm64/native/libGLESv2.pdb |
64.5 MB → 64.4 MB (🟢 −80.0 KB) |
runtimes/win-x86/native/libGLESv2.pdb |
67.3 MB → 67.2 MB (🟢 −80.0 KB) |
Informational only — this never blocks the PR. Native binaries are labelled by os/arch.
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
5 similar comments
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
2 similar comments
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
a12b2b3 to
077354b
Compare
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
1 similar comment
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
Wraps the new sk_graphite_context_options.fShaderErrorHandler C API so managed callers can capture Graphite shader compile failures. The motivating case is the Metal iOS simulator gradient scene where Recorder.Snap() silently returns null with no diagnostic — Skia's default handler prints via SkDebugf + asserts, which is invisible in shipping apps on Apple platforms. * new CreateDawn/CreateMetal/CreateVulkan overloads taking SKGraphiteShaderErrorHandlerDelegate (backwards-compatible additions, no signature changes) * SKGraphiteShaderErrorHandlerDelegate proxy in DelegateProxies never throws across the FFI boundary — a bad handler cannot crash Skia's shader-compile path * GCHandle pinned for the Context's lifetime, freed in DisposeNative after the native context tears down its internal handler pointer Includes GraphiteVkContextAcceptsShaderErrorHandler smoke test that proves the wiring survives Context create + trivial draw + dispose on Vulkan (Lavapipe compiles cleanly so the callback does not fire — the important assertion is no double-free / no EntryPointNotFound on teardown). The Metal simulator failure path requires an iOS host to reproduce and is out of scope for CI. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Root cause (diagnosed with the fShaderErrorHandler hook from this branch, plus PSO-failure forwarding added to it): Graphite's MtlCaps enabled framebuffer fetch for every Apple-family GPU and Caps::getDstReadStrategy() then always used it for dst reads. The iOS simulator advertises an Apple GPU family but cannot read a render target from a fragment shader — the MSL compiles, but newRenderPipelineStateWithDescriptor fails with CompilerError Code=2 "reading from a rendertarget is not supported" — so the Multiply-blend draw in GradientBlend (the only matrix scene needing a dst read) nulled its pipeline and Recorder.Snap() returned null. Ganesh/Metal never uses framebuffer fetch and real devices/macOS support it, which is why only this one cell failed. Submodule changes (mono/skia dev/issue-4555-graphite-shader-error-handler): * Metal PSO-creation failures are now reported through the ShaderErrorHandler (previously SkDebugf-only, invisible on iOS) * framebuffer fetch is disabled under TARGET_OS_SIMULATOR; dst reads fall back to DstReadStrategy::kTextureCopy there Test harness: * remove the simulator-only null-Snap() skip seam from GraphiteMetalRenderer — a null Snap() is a hard failure everywhere again, so regressions can no longer hide behind the skip * seed the graphite-metal.ios/GradientBlend golden (rendered on an iPhone 16 / iOS 26.1 simulator; pixel-identical to the graphite-metal.macos golden) Verified on the iOS simulator: full SkiaSharp.Tests.Devices run is green (1219 total, 0 failed, 1138 succeeded, 81 skipped), with the GradientBlend cell rendering and matching its golden. Fixes mono#4555. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
077354b to
71c6d22
Compare
Description
Fixes #4555 (Graphite/Metal
GradientBlendcell returning nullRecorder.Snap()only on the iOS simulator) and adds the C API surface used to diagnose it.Two commits, two roles:
SKGraphiteContextshader-error-handler overloads — wraps the newsk_graphite_context_options.fShaderErrorHandlerC API. New backwards-compatibleCreateDawn/CreateMetal/CreateVulkanoverloads take anSKGraphiteShaderErrorHandlerDelegate; delegate is pinned for the Context's lifetime and freed inDisposeNativeafter the native context tears down its handler pointer. The proxy never throws across the FFI boundary — a bad handler cannot crash Skia's shader-compile path.Fix Graphite/Metal
GradientBlendon the iOS simulator — bumps the submodule to pick up the two Skia-side fixes: PSO-creation failures now routed throughShaderErrorHandler, and framebuffer fetch disabled underTARGET_OS_SIMULATOR. Drops the simulator-only null-Snap()skip seam fromGraphiteMetalRenderer(a null Snap is again a hard failure everywhere, so regressions can't hide behind the skip) and adds the newly-passinggraphite-metal.ios/GradientBlend.pnggolden.Related issues
Fixes #4555
Required skia PR
Requires mono/skia#308
Areas affected
binding/)externals/skia/src/c,include/c)Changes
Public API
ABI-stable additive changes only.
Behavior
GradientBlend(and any other draw needing a dst read with a non-coefficient blend — Multiply, Screen, Overlay, …) now renders correctly instead of returning null fromRecorder.Snap(). Real Apple devices and macOS unchanged.Snap()skip inGraphiteMetalRendereris removed — regressions to this class of bug now fail loudly.SkDebugf).Testing
Full
SkiaSharp.Tests.Devicesrun on iPhone 16 / iOS 26.1 simulator (arm64): 1219 total, 0 failed, 1138 succeeded, 81 skipped.GradientBlendcell rendering and matching its committedgraphite-metal.iosgolden.GraphiteVkContextAcceptsShaderErrorHandlersmoke test on Linux/Lavapipe (Vulkan): passes — proves no double-free / noEntryPointNotFoundExceptionon Context teardown after installing a handler. The Metal simulator path itself needs an Apple host to reproduce, which is what the Devices-tests run above covered.Checklist
Changesabove lists all public API and behavioral changes (or "None.")mono/skiaPR linked above and bindings regenerated