Avoid eager font manager initialization#532
Conversation
wieslawsoltes
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae60d91d6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var typefaceResolution = ResolveExplicitTypeface(paint.Typeface); | ||
| var skFont = new SkiaSharp.SKFont(typefaceResolution?.Typeface, paint.TextSize) |
There was a problem hiding this comment.
When SVG text has no explicit font-family, PaintingService.SetTypeface still creates a shim typeface with a null family, so this path now builds an SKFont with Typeface = null. In DrawTextCanvasCommand, that font is passed to TryDrawShapedText, whose TryShapeText immediately returns false when font.Typeface is null, so default-font Arabic/Indic/RTL text skips the HarfBuzz shaping path and falls back to SKCanvas.DrawText. This regresses text rendering/measurement for any complex-script text that does not specify a family, even though the change is intended to avoid eager initialization for non-text SVGs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 52d4096e6.
The review was right: using the non-text paint/typeface path for SKFont creation meant implicit-family text could reach TryDrawShapedText with font.Typeface == null, which disables HarfBuzz shaping.
The fix restores text-specific typeface resolution while keeping non-text paint conversion lazy:
ToSKFont(SKPaint)andToSKFont(SKFont?)now resolve throughResolveSKTypeface(...), so implicit/default text gets a real Skia typeface before shaping.- Added
ToSKTextPaint(...)for text-only paint operations. It starts from the lazyToSKPaint(...), then resolves the text typeface only for text metrics, paths, shaping, and text drawing. DrawTextCanvasCommand,DrawTextOnPathCanvasCommand,GetFontMetrics,MeasureText,GetTextPath, andTryShapeGlyphRunnow use the text-specific path.ToSKPaint, cached render paint, and wireframe paint still avoid resolving the default typeface for non-text rendering.
Regression coverage added: TryShapeGlyphRun_WithImplicitTypeface_UsesDefaultTextTypeface plus updated ToSKFont_*_ResolvesDefaultTypefaceForText tests.
Validation: dotnet format Svg.Skia.slnx --no-restore, dotnet build Svg.Skia.slnx -c Release --no-restore, focused Svg.Skia.UnitTests filter with 45 passed, full dotnet test Svg.Skia.slnx -c Release --no-restore --no-build with Svg.Skia.UnitTests at 1864 passed / 533 skipped / 2397 total, and git diff --check.
wieslawsoltes
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b35287e59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private TypefaceResolution? ResolveExplicitTypeface(SKTypeface? typeface) | ||
| { | ||
| return HasExplicitTypeface(typeface) ? ResolveSKTypeface(typeface) : null; |
There was a problem hiding this comment.
When an SVG sets font-style="italic" or font-style="oblique" but leaves font-family unspecified, PaintingService.SetTypeface still passes a shim typeface whose family is null and whose slant carries the requested style. This new helper treats that as no typeface at all, so ToSKPaint/ToSKFont pass null to Skia and ApplyTypefaceAdjustments only synthesizes bold; the slant is dropped and the text renders upright instead of using the default italic/oblique face as the previous ResolveSKTypeface path did.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 52d4096e6.
This was also valid. Treating a null/blank-family shim typeface as no typeface in ToSKFont dropped the style information that PaintingService.SetTypeface carried for implicit font-style="italic" / font-style="oblique" text.
The fix makes text font conversion resolve via ResolveSKTypeface(...) using the requested weight, width, and slant even when the family is implicit. That preserves the default styled face for text shaping and rendering. The lazy explicit-family-only behavior remains limited to non-text ToSKPaint / render-paint paths, where resolving a default font would be an unwanted side effect.
Regression coverage added: ToSKFont_WithImplicitItalicTypeface_PreservesSlantForText, plus updated implicit bold/default ToSKFont tests.
Validation: same fix commit 52d4096e6 passed dotnet format Svg.Skia.slnx --no-restore, dotnet build Svg.Skia.slnx -c Release --no-restore, the focused Svg.Skia.UnitTests review-fix subset with 45 passed, full dotnet test Svg.Skia.slnx -c Release --no-restore --no-build with Svg.Skia.UnitTests at 1864 passed / 533 skipped / 2397 total, and git diff --check.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f5e603de0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| private TypefaceResolution? ResolveExplicitTypeface(SKTypeface? typeface) | ||
| { | ||
| return HasExplicitTypeface(typeface) ? ResolveSKTypeface(typeface) : null; |
There was a problem hiding this comment.
When SVG text sets font-stretch/non-normal width but leaves font-family unset, the shim typeface carries the requested FontWidth with a null family. This path now treats that as no typeface, so ToSKPaint/ToSKFont pass a null native typeface and there is no other place to apply condensed/expanded width; previously ResolveSKTypeface selected SKTypeface.FromFamilyName(null, weight, width, slant), preserving the default-family width request when available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed by the text-resolution fix in 52d4096e6, with explicit regression coverage added in f922c0b06.
The code fix resolves text font conversion through ResolveSKTypeface(...), which carries the requested weight, width, and slant from the shim typeface even when FamilyName is null/blank. That means implicit font-stretch requests follow the same resolution path as the previous eager text conversion, while non-text ToSKPaint / render-paint conversion stays explicit-family-only to avoid loading the platform font manager as a side effect.
I added ToSKFont_WithImplicitCondensedTypeface_PreservesWidthForText in f922c0b06. The regression compares the ToSKFont text conversion result with the direct ToSKTypeface resolution for the same implicit condensed shim typeface, so the test verifies that width is no longer discarded by the ToSKFont path.
Validation after the follow-up: dotnet format Svg.Skia.slnx --no-restore, focused dotnet test tests/Svg.Skia.UnitTests/Svg.Skia.UnitTests.csproj -f net10.0 -c Release --no-restore --filter "FullyQualifiedName~SKSvgSettingsTests" with 38 passed, and git diff --check. The underlying code fix in 52d4096e6 had already passed the full release build and full release test run.
There was a problem hiding this comment.
💡 Codex Review
Resolve text paints before splitting fallback spans
When text has an implicit family but carries style information such as font-style="italic" or font-stretch="condensed", this asset-loader path still builds its working paint with ToSKPaint, which now intentionally leaves implicit typefaces as null. The fallback matching path then reaches MatchPlatformCharacter(null, ...), which ignores the requested width/slant and returns a platform face whose slant/width are copied into the span/run, so text layout paths that use FindTypefaces/FindRunTypeface can replace implicit italic/condensed text with upright/normal text despite the direct draw path being restored via ToSKTextPaint.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the latest Codex review, "Resolve text paints before splitting fallback spans", in The finding was valid. The fix keeps this scoped to actual text fallback resolution:
Regression coverage added in
Validation after the fix:
I re-read the PR discussion after pushing; this latest Codex item is present as a PR review body rather than an inline review thread in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5e098f1ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var paint = wireframe | ||
| ? ToWireframePaint(drawTextCanvasCommand.Paint) | ||
| : GetRenderPaint(drawTextCanvasCommand.Paint); | ||
| : ToSKTextPaint(drawTextCanvasCommand.Paint); |
There was a problem hiding this comment.
When rendering non-wireframe text, this now calls ToSKTextPaint, which allocates a fresh SKPaint via ToSKPaint instead of returning a model-owned cached paint. The local paint is never disposed here—the finally only disposes textPaint when it is a clone—so repeated DrawTextCanvasCommand rendering leaks unmanaged SKPaint instances; the text-on-path branch has the same ownership issue after the same replacement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a0d9383.
The review was correct: the text-specific paint path creates transient SKPaint instances with ToSKTextPaint/ToWireframePaint, while DisposeIfCloned only handles the optional CreateTextRenderPaint clone.
The fix changes both DrawTextCanvasCommand and DrawTextOnPathCanvasCommand to bind the transient paint with using var paint. When CreateTextRenderPaint returns a clone, DisposeIfCloned(textPaint, paint) still disposes the clone; then the using scope disposes the base transient paint. When no clone is created, textPaint is the same object as paint and DisposeIfCloned is a no-op, with the using scope disposing the paint once.
Validation after the fix:
- focused text/render subset: 153 tests passed
- dotnet format Svg.Skia.slnx --no-restore passed
- dotnet build Svg.Skia.slnx -c Release --no-restore passed with 0 errors and 375 warnings
- full dotnet test Svg.Skia.slnx -c Release --no-restore --no-build passed, including Svg.Skia.UnitTests 1867 passed, 533 skipped, 2400 total
- git diff --check passed
1 participant
Summary
Fixes eager font-manager side effects in Svg.Skia without trying to mask Alpine Linux native dependency problems.
Issue #531 reports an Alpine ARM64 crash inside SkiaSharp's native fontconfig path while resolving
SKFontManager.Default. The linked repro already installsfontconfigand fonts, but it usesSkiaSharp.NativeAssets.Linuxunder Alpine. The Alpine-specific deployment guidance in https://medo64.com/posts/skiasharp-under-alpine-linux points to usingSkiaSharp.NativeAssets.Linux.NoDependenciesplusfontconfigand installed fonts for that environment.This PR therefore keeps Svg.Skia from initializing the platform font manager as a side effect of settings construction, cache hashing, or non-text paint/font conversion. It does not suppress normal platform font fallback when text rendering actually needs SkiaSharp's font manager.
Changes
FontManagerTypefaceProviderlazy-loadSKFontManager.Defaultonly when font-manager-backed font resolution is requested.SkiaModelandSkiaSvgAssetLoader.FontManagerTypefaceProviderin the shared provider typeface cache until its font manager has already been initialized.FamilyNameas implicit SVG typefaces instead of resolving them toSKTypeface.Defaultduring paint/font conversion.SkiaSharp.NativeAssets.Linux.NoDependencies,fontconfig, and installed fonts in the README.FindTypefacesandFindRunTypefacepreserve implicit-family italic and condensed text styling.Tests
Added regressions covering:
FontManagerTypefaceProviderconstruction not loading the default font manager.ToSKPaint, cached render paint, and wireframe paint preserving null native typefaces for implicit/no-family source typefaces.ToSKPaintpreserving synthetic bold for implicit bold typefaces without resolving a platform typeface.ToSKFontresolving an implicit/default typeface for text rendering.ToSKFontpreserving implicit bold, italic, and condensed-width text styling.TryShapeGlyphRunshaping implicit-family text with the default text typeface.FindTypefacesandFindRunTypefacematching the resolved text typeface for implicit-family styled text.Validation
dotnet format Svg.Skia.slnx --no-restoredotnet build Svg.Skia.slnx -c Release --no-restoredotnet test Svg.Skia.slnx -c Release --no-restore --no-buildgit diff --checkThe release build completed with existing warnings and no errors. The full test run passed, including
Svg.Skia.UnitTestswith 1862 passed, 533 skipped, 2395 total.The README-only follow-up was validated with
git diff --check; no additional runtime test run was required for the documentation change.After addressing PR review feedback, the focused
SKSvgSettingsTests,Issue405Tests, andIssue462Testssubset passed with 45 tests. The full release test run passed again, includingSvg.Skia.UnitTestswith 1864 passed, 533 skipped, 2397 total. A follow-up focusedSKSvgSettingsTestsrun passed with 38 tests after adding explicit coverage for implicit condensed text width.After the follow-up review about fallback span splitting, the focused
SKSvgSettingsTests,Issue405Tests, andIssue462Testssubset passed with 48 tests.dotnet format Svg.Skia.slnx --no-restore,dotnet build Svg.Skia.slnx -c Release --no-restore, fulldotnet test Svg.Skia.slnx -c Release --no-restore --no-build, andgit diff --checkpassed. The final full release test run includedSvg.Skia.UnitTestswith 1867 passed, 533 skipped, 2400 total.After the transient text paint disposal review, the focused text/render subset passed with 153 tests.
dotnet format Svg.Skia.slnx --no-restore,dotnet build Svg.Skia.slnx -c Release --no-restore, fulldotnet test Svg.Skia.slnx -c Release --no-restore --no-build, andgit diff --checkpassed. The final full release test run includedSvg.Skia.UnitTestswith 1867 passed, 533 skipped, 2400 total.I reviewed the issue comment and the
JMS-1/pdf-svg-crash-reproproject. The repro's Dockerfile installsfontconfigand fonts, but the project referencesSkiaSharp.NativeAssets.Linux; for Alpine the likely deployment fix is to switch the consuming app toSkiaSharp.NativeAssets.Linux.NoDependenciesand keepfontconfigplus fonts installed. I could not execute the exact Alpine ARM64 Docker repro locally because Docker/Colima was not running andcolima startfailed in this environment.