Cache SKPath to PathF.PlatformPath in Microsoft.Maui.Graphics.Skia

[lindexi]

Description of Change

We can observe that in implementations such as Microsoft.Maui.Graphics.Win2D, methods like PlatformDrawPath and FillPath rely on the PathF.PlatformPath property as a cache when obtaining platform-specific Path or Geometry from PathF. This approach enhances the performance of repeated drawing and reduces the frequency of creating platform-specific objects.

However, in Microsoft.Maui.Graphics.Skia, a new SKPath object is created each time. This means that repeated drawing of a complex PathF will not achieve optimal performance in Microsoft.Maui.Graphics.Skia.

The purpose of this change is to allow Microsoft.Maui.Graphics.Skia to utilize the PathF.PlatformPath property as a cache, thereby improving the performance of Microsoft.Maui.Graphics.Skia when repeatedly drawing complex PathF.

// Cc: @mattleibow

[jsuarezruiz]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[bronteq]

@mattleibow can this help with performance?

[kubaflo]

/azp run maui-pr-uitests maui-pr-devicetests

[azure-pipelines]

No pipelines are associated with this pull request.

[kubaflo]

@mattleibow what do you think?

[kubaflo]

/review -b feature/refactor-copilot-yml

[kubaflo]

/review -b feature/enhanced-reviewer -p windows

[kubaflo]

@mattleibow

[kubaflo]

/review rerun

[kubaflo]

/review -b feature/enhanced-reviewer -p windows

[MauiBot]

AI Review Summary

@lindexi — new AI review results are available based on this last commit: ff9d1b7. To request a fresh review after new comments or commits, comment /review rerun.

---

🚀 Next Steps — alternative fix proposed (try-fix-1)

Automated review — alternative fix proposed

The expert-reviewer evaluation compared the PR fix against automatically generated candidates and selected try-fix-1 as the strongest fix.

Why: try-fix-1 wins because it preserves the SKPath PlatformPath cache while rebuilding disposed native handles and preventing operation-specific FillType state from leaking through the shared cached path. It passed the targeted Graphics.Skia build and avoids the compatibility risk of replacing PlatformPath with a wrapper.

Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.

Candidate diff (try-fix-1)

diff --git a/src/Graphics/src/Graphics.Skia/SkiaCanvas.cs b/src/Graphics/src/Graphics.Skia/SkiaCanvas.cs
index 382cfd4bb4..1c2760b4c7 100644
--- a/src/Graphics/src/Graphics.Skia/SkiaCanvas.cs
+++ b/src/Graphics/src/Graphics.Skia/SkiaCanvas.cs
@@ -642,29 +642,63 @@ namespace Microsoft.Maui.Graphics.Skia
 			_canvas.ClipRect(rect, SKClipOperation.Difference);
 		}
 
+		private SKPath GetPath(PathF path)
+		{
+			var skPath = path.PlatformPath as SKPath;
+
+			if (skPath is null || skPath.Handle == IntPtr.Zero)
+			{
+				skPath = path.AsSkiaPath();
+				path.PlatformPath = skPath;
+			}
+
+			return skPath;
+		}
+
+		static SKPathFillType GetFillType(WindingMode windingMode) =>
+			windingMode == WindingMode.NonZero ? SKPathFillType.Winding : SKPathFillType.EvenOdd;
+
 		protected override void PlatformDrawPath(
 			PathF path)
 		{
-			var platformPath = path.AsSkiaPath();
+			var platformPath = GetPath(path);
 			_canvas.DrawPath(platformPath, CurrentState.StrokePaintWithAlpha);
-			platformPath.Dispose();
 		}
 
 		public override void ClipPath(PathF path,
 			WindingMode windingMode = WindingMode.NonZero)
 		{
-			var platformPath = path.AsSkiaPath();
-			platformPath.FillType = windingMode == WindingMode.NonZero ? SKPathFillType.Winding : SKPathFillType.EvenOdd;
-			_canvas.ClipPath(platformPath);
+			var platformPath = GetPath(path);
+			var previousFillType = platformPath.FillType;
+
+			try
+			{
+				platformPath.FillType = GetFillType(windingMode);
+				_canvas.ClipPath(platformPath);
+			}
+			finally
+			{
+				if (platformPath.Handle != IntPtr.Zero)
+					platformPath.FillType = previousFillType;
+			}
 		}
 
 		public override void FillPath(PathF path,
 			WindingMode windingMode)
 		{
-			var platformPath = path.AsSkiaPath();
-			platformPath.FillType = windingMode == WindingMode.NonZero ? SKPathFillType.Winding : SKPathFillType.EvenOdd;
-			_canvas.DrawPath(platformPath, CurrentState.FillPaintWithAlpha);
-			platformPath.Dispose();
+			var platformPath = GetPath(path);
+			var previousFillType = platformPath.FillType;
+
+			try
+			{
+				platformPath.FillType = GetFillType(windingMode);
+				_canvas.DrawPath(platformPath, CurrentState.FillPaintWithAlpha);
+			}
+			finally
+			{
+				if (platformPath.Handle != IntPtr.Zero)
+					platformPath.FillType = previousFillType;
+			}
 		}
 
 		public override void DrawString(

🗂️ Review Sessions — click to expand

🧪 Gate — Test Before & After Fix

Gate Result: ⚠️ INCONCLUSIVE

Platform: WINDOWS

⚠️ verify-tests-fail.ps1 exited before writing a verification report. Diagnostics below.

Exit code: 3

Likely cause:

• Test detection failed — no runnable tests were found in the PR diff.

Gate output log (last 60 lines)

📁 Output directory: CustomAgentLogsTmp/PRState/21169/PRAgent/gate/verify-tests-fail
🔍 Detecting base branch and merge point...
No PR detected, scanning remote branches for closest base...
✅ Base branch: main (via closest-merge-base)
✅ Merge base commit: 4567a055
   (1 commits ahead of main)
╔═══════════════════════════════════════════════════════════╗
║         FULL VERIFICATION MODE                            ║
╠═══════════════════════════════════════════════════════════╣
║  Fix files detected - will verify:                        ║
║  1. Tests FAIL without fix                                ║
║  2. Tests PASS with fix                                   ║
╚═══════════════════════════════════════════════════════════╝
✅ Fix files (1):
   - src/Graphics/src/Graphics.Skia/SkiaCanvas.cs
🔍 Auto-detecting test filter from changed test files...
⚠️ No tests detected in this PR.
   Searched for: UI tests, unit tests, XAML tests, device tests
   Consider adding tests via write-tests-agent.

---

🛫 Pre-Flight — Context & Validation

Issue: N/A - No linked issue found in PR metadata
PR: #21169 - Cache SKPath to PathF.PlatformPath in Microsoft.Maui.Graphics.Skia
Platforms Affected: Skia graphics backend; validation platform: windows
Files Changed: 1 implementation, 0 test

Key Findings

• PR caches PathF.AsSkiaPath() results in PathF.PlatformPath for SkiaCanvas.DrawPath, ClipPath, and FillPath to reduce repeated native SKPath allocation.
• The PR matches the broad caching pattern used by Win2D and Mac/iOS graphics canvases, but the current Skia helper reuses any non-null cached SKPath without checking whether the native handle was disposed.
• The current PR helper also combines cache lookup with fill-rule mutation, so stroke drawing calls GetPath(path) and resets the cached path's FillType to Winding even though stroking does not require fill-rule state.
• GitHub CLI was unauthenticated in this environment; public PR metadata and diff were fetched through the GitHub REST API. Required CI status could not be queried.

Code Review Summary

Verdict: NEEDS_CHANGES
Confidence: low
Errors: 1 | Warnings: 1 | Suggestions: 1

Key code review findings:

• ❌ src/Graphics/src/Graphics.Skia/SkiaCanvas.cs:647 — cached disposed SKPath can be reused because GetPath() only checks for null and does not check Handle == IntPtr.Zero.
• ⚠️ src/Graphics/src/Graphics.Skia/SkiaCanvas.cs:655 — PlatformDrawPath() resets cached FillType to Winding; stroke drawing should not need to mutate fill-rule state.
• 💡 Add Skia graphics regression coverage for cache reuse, mutation invalidation, disposed cached path recreation, and fill-type behavior across repeated draw/fill/clip calls.

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #21169	Cache one SKPath in PathF.PlatformPath and set FillType per draw/fill/clip operation	⚠️ INCONCLUSIVE (Gate pre-run could not build/run)	src/Graphics/src/Graphics.Skia/SkiaCanvas.cs	Original PR improves allocation behavior but leaves disposed-handle and shared fill-state concerns.

---

🔬 Code Review — Deep Analysis

Code Review — PR #21169

Independent Assessment

What this changes: Caches PathF.AsSkiaPath() results in PathF.PlatformPath for Skia DrawPath, FillPath, and ClipPath, avoiding per-call SKPath allocation/disposal.
Inferred motivation: Improve repeated rendering performance for complex PathF instances, matching other platform canvas cache patterns.

Reconciliation with PR Narrative

Author claims: Skia should use PathF.PlatformPath as a platform-path cache like Win2D, improving repeated complex path drawing performance.
Agreement/disagreement: Agreed on the goal and general direction. However, the implementation leaves cached native lifetime and cached FillType state issues unresolved.

Prior Review Reconciliation

Prior ❌ Error Finding	Source	Status	Evidence
Cached disposed SKPath can be reused because GetPath() only checks null	MauiBot reviews on 2026-05-05 / 2026-06-07	❌ Unresolved	Current src/Graphics/src/Graphics.Skia/SkiaCanvas.cs:647-655 still reuses path.PlatformPath as SKPath without checking skPath.Handle == IntPtr.Zero.

Prior warning about PlatformDrawPath() resetting cached FillType is also still present.

Blast Radius Assessment

• Runs for all instances: Yes — every Skia DrawPath, FillPath, and ClipPath now uses the cache.
• Startup impact: No.
• Static/shared state: No, but it stores mutable native state on public PathF.PlatformPath.

CI Status

• Required-check result: undetermined
• Classification: tool-unavailable; gh pr checks 21169 --repo dotnet/maui --required failed because gh is unauthenticated.
• Action taken: capped confidence; not LGTM.

Findings

❌ Error — Disposed cached SKPath can be reused

src/Graphics/src/Graphics.Skia/SkiaCanvas.cs:647

GetPath() treats any non-null PathF.PlatformPath as SKPath as reusable. Since PlatformPath is public and stores a disposable native object, callers can observe/dispose it independently, leaving an SKPath whose native handle is invalid. The Mac/iOS cache explicitly guards Handle == IntPtr.Zero; Skia should do the same before returning the cached path.

⚠️ Warning — Stroke drawing mutates cached fill state

src/Graphics/src/Graphics.Skia/SkiaCanvas.cs:655

PlatformDrawPath() calls GetPath(path) with the default Winding fill type, so stroke-only drawing rewrites cached SKPath.FillType. Fill/clip operations have explicit winding modes; stroke drawing does not need to alter fill state. Prefer setting FillType only for FillPath/ClipPath.

💡 Suggestion — Add regression coverage

Add Skia graphics tests for cache reuse, mutation invalidation, disposed cached path recreation, and fill-type behavior across repeated draw/fill/clip calls.

Failure-Mode Probing

• Cached path externally disposed: current code reuses the disposed SKPath; handle guard is missing.
• PathF mutated after caching: normal mutations call Invalidate() and release the cache, so this path is mostly safe.
• Draw/fill/clip interleaving: fill/clip reset FillType, but stroke draws still unnecessarily mutate cached state.

Verdict: NEEDS_CHANGES

Confidence: low
Summary: The optimization direction is sound, but the unresolved disposed-handle issue is a concrete correctness bug. CI status is also unavailable from this environment, so this cannot be LGTM.

---

🛠️ Fix — Analysis & Comparison

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
1	maui-expert-reviewer	Separate cached geometry from fill-rule state. GetPath only returns a valid cached SKPath and checks Handle == IntPtr.Zero; FillPath and ClipPath temporarily set the requested fill type and restore the previous value in finally; PlatformDrawPath no longer mutates FillType.	✅ Pass (targeted build)	1 file	Most robust alternative: fixes disposed cached native handles and prevents operation-specific fill-rule state from leaking into the shared cached path. Extra try/finally complexity is acceptable because Skia draw/clip APIs are synchronous.
2	maui-expert-reviewer	Cache only the canonical Winding path. NonZero draw/fill/clip use the cached path with a disposed-handle guard; EvenOdd fill/clip create temporary paths so the cached path is never changed to EvenOdd.	✅ Pass (targeted build)	1 file	Lower-risk and simpler than candidate 1, but performance gains are narrower because repeated EvenOdd operations still allocate. It also preserves the old no-dispose behavior for EvenOdd ClipPath.
3	maui-expert-reviewer	Cache a private disposable Skia path-cache wrapper in PathF.PlatformPath, with separate lazily-created SKPath instances for Winding and EvenOdd fill types.	✅ Pass (targeted build)	1 file	Compiles and avoids fill-state mutation while caching both modes, but is not selected because PathF.PlatformPath would no longer be directly an SKPath, creating compatibility risk for consumers that inspect the platform path.
PR	PR #21169	Cache one mutable SKPath in PathF.PlatformPath and set FillType in the shared helper	⚠️ INCONCLUSIVE (prior gate could not build/run)	1 file	Original PR improves repeated-path allocation but does not guard disposed cached handles and mutates shared fill state from stroke calls.

Cross-Pollination

Model	Round	New Ideas?	Details
gpt-5.5 / maui-expert-reviewer	1	Yes	Candidate 1: separate cache lookup from scoped fill-rule mutation/restoration.
gpt-5.5 / maui-expert-reviewer	2	Yes	Candidate 2: cache only Winding paths and keep EvenOdd paths temporary.
gpt-5.5 / maui-expert-reviewer	3	Yes	Candidate 3: cache a wrapper containing separate Winding and EvenOdd SKPath instances; rejected for compatibility risk.

Exhausted: Yes - remaining variants are trivial combinations of these tradeoffs: shared path with scoped state, Winding-only cache plus temporary EvenOdd paths, or a multi-path cache wrapper.
Selected Fix: Candidate #1 - It is the best balance of correctness and compatibility: PathF.PlatformPath remains an SKPath, disposed native handles are rebuilt, and operation-specific fill-rule state is not left on the cached path.

Validation Notes

All three candidates passed the targeted Windows build for src\Graphics\src\Graphics.Skia\Graphics.Skia.csproj targeting netstandard2.0. The prior gate result remains inconclusive and was not rerun, per instruction.

---

📝 Recommended PR Title & Description

Assessment: ✏️ Recommend updating — the current metadata accurately explains the caching goal, but the winning fix also adds disposed-handle recovery and scoped SKPath.FillType handling that should be documented.

Recommended title

[Skia] Graphics: Cache SKPath in PathF.PlatformPath

Recommended description

### Description of Change

We can observe that in implementations such as Microsoft.Maui.Graphics.Win2D, methods like PlatformDrawPath and FillPath rely on the PathF.PlatformPath property as a cache when obtaining platform-specific Path or Geometry from PathF. This approach enhances the performance of repeated drawing and reduces the frequency of creating platform-specific objects.

However, in Microsoft.Maui.Graphics.Skia, a new SKPath object is created each time. This means that repeated drawing of a complex PathF will not achieve optimal performance in Microsoft.Maui.Graphics.Skia.

The purpose of this change is to allow Microsoft.Maui.Graphics.Skia to utilize the PathF.PlatformPath property as a cache, thereby improving the performance of Microsoft.Maui.Graphics.Skia when repeatedly drawing complex PathF.

The final implementation keeps PathF.PlatformPath as an SKPath for compatibility, recreates the cached path if the existing SKPath has already been disposed, and scopes temporary SKPath.FillType changes for FillPath and ClipPath so operation-specific winding mode state does not leak through the shared cached path.

// Cc: @mattleibow

---

🏁 Report — Final Recommendation

Comparative Analysis — PR #21169

Candidate Ranking

Rank	Candidate	Test result	Assessment
1	try-fix-1	✅ Pass — targeted Graphics.Skia netstandard2.0 build	Best balance of correctness, compatibility, and performance. It keeps PathF.PlatformPath as an SKPath, recreates disposed cached native paths, and scopes temporary fill-rule changes so draw/fill/clip operations do not leak FillType state through the shared cached object.
2	pr-plus-reviewer	✅ Pass — targeted sandbox build after restore	Fixes the expert reviewer's major correctness issue by rebuilding disposed cached paths, but still mutates the shared cached SKPath.FillType for each operation and leaves the last fill rule on the cached path.
3	try-fix-2	✅ Pass — targeted build	Correctly guards disposed Winding-path reuse and avoids changing the cached path for EvenOdd operations, but it narrows the performance win by continuing to allocate temporary EvenOdd paths and preserves old ClipPath temporary-path lifetime behavior.
4	try-fix-3	✅ Pass — targeted build	Caches both Winding and EvenOdd paths without shared fill-state mutation, but stores a private wrapper in PathF.PlatformPath instead of an SKPath, creating compatibility risk for consumers expecting the platform path to be the actual native path.
5	pr	⚠️ Inconclusive gate; no targeted pass recorded for the raw PR in this phase	Improves repeated-path allocation, but reuses disposed cached SKPath instances and mutates shared fill-rule state from draw/fill/clip operations.

Regression-Test Rule

No candidate had a recorded regression-test failure. The original gate was inconclusive because no runnable tests were detected, and per instruction this was not treated as a failed fix. All try-fix candidates and pr-plus-reviewer passed a targeted Graphics.Skia build; candidates were therefore ranked by code correctness, compatibility, and performance tradeoffs.

Winner

Winner: try-fix-1

try-fix-1 should replace the raw PR fix. It addresses both substantive risks identified during review: disposed cached SKPath reuse and operation-specific FillType leakage on the shared cached path. Unlike try-fix-3, it preserves PathF.PlatformPath as an SKPath, and unlike try-fix-2, it keeps caching effective for both fill modes while avoiding persistent state mutation.

[kubaflo]

Closing this as a stale one - please feel free to create a new one if you think it is still needed given the latest version of SkiaSharp

[lindexi]

@kubaflo This is a so bad development experience. But I know this is not your problem.

[kubaflo]

@lindexi Sorry, but we reviewed this PR with suggested changes a couple of times, and no action was taken afterward, so I closed it.

If you still want to pursue this change, please feel free to create a new PR and we’ll review it promptly.
I don’t think it’s worth keeping a PR open for two years when no updates have been made, especially after
recommendations were already provided.

[jfversluis]

@lindexi just chiming in here as well! Sorry about the bad experience, of course that is not what we want to provide or what our intention was here from Jakub or anyone else. In the first place it seems that this PR has been open a long time without any movement, that is totally on us, sorry for that.

However, we did get to it now and we ran our reviews on it that came up with some suggestions and that wasn't acted on. But seeing your response now that wasn't because you lost interest, but rather that maybe you didn't see that any action on your side was requested?

Let's see if we can work together to get this across the finish line if this is still something that you think is relevant and you're happy to contribute, thank you!

Is there a good way to be in touch a bit more directly? Maybe the MAUIverse is an option for that? Please let us know!