Fix SkiaSharp obsolete warnings in generated output#526
Conversation
![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: 496678a558
ℹ️ 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".
| public void DrawImage(SKImage image, SKRect source, SKRect dest, SKSamplingOptions sampling, SKPaint? paint = null) | ||
| { | ||
| AddCommand(new DrawImageCanvasCommand(image, source, dest, paint, sampling)); |
There was a problem hiding this comment.
When a caller records an image with the new DrawImage(..., SKSamplingOptions, ...) overload, the retained model stores Sampling, but SkiaModel.Draw still renders every DrawImageCanvasCommand via the old skCanvas.DrawImage(image, source, dest, paint) path and never reads drawImageCanvasCommand.Sampling (checked src/Svg.Skia/SkiaModel.cs lines 1845-1849). As a result, models that request cubic/linear sampling display with whatever the paint/default legacy path chooses, while codegen emits the requested sampling, so runtime rendering and generated output diverge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 568a1081a9bbc4720aecaa0b3507ef49330aa46f.
SkiaModel.Draw now reads DrawImageCanvasCommand.Sampling during retained replay. Because Svg.Skia currently compiles against a SkiaSharp runtime surface where SKCanvas.DrawImage(..., SKSamplingOptions, ...) is not available, the replay path translates the retained shim SKSamplingOptions to the closest runtime SKFilterQuality and applies it to a scoped cloned SKPaint. This keeps cached paints immutable and makes retained runtime rendering honor explicit recorded sampling instead of falling back to the legacy paint/default choice.
I also added SKSvgRebuildFromModelTests.ToSKPicture_UsesRecordedImageSampling, which renders a retained two-color image with explicit linear sampling and verifies blended pixels are produced.
Validation run after the fix:
dotnet format Svg.Skia.slnx --no-restoredotnet build Svg.Skia.slnx -c Release --no-restore -v:minimal /nologodotnet test Svg.Skia.slnx -c Release --no-restore -v:minimal /nologo
The full build passed with existing unrelated warnings only, and the full test run passed. Svg.Skia.UnitTests now reports 1848 passed and 533 skipped.
There was a problem hiding this comment.
Additional refinement landed in 7b535163dcaa3bdb0a97d85bd62937798f1ba36a.
The initial fix in 568a1081a9bbc4720aecaa0b3507ef49330aa46f made retained replay consume DrawImageCanvasCommand.Sampling. This follow-up removes the remaining lossy behavior for newer SkiaSharp runtimes: when SKSamplingOptions and the matching SKCanvas.DrawImage overload are present at runtime, replay now constructs the native sampling options and invokes that exact overload. The SKFilterQuality mapping remains only as the SkiaSharp 2.88 fallback path.
This was validated with the focused retained replay tests plus the full release build and full release test suite after the follow-up commit.
| public void DrawText(string text, float x, float y, SKTextAlign textAlign, SKFont font, SKPaint paint) | ||
| { | ||
| AddCommand(new DrawTextCanvasCommand(text, x, y, paint, textAlign, font)); |
There was a problem hiding this comment.
The new overload records TextAlign and SKFont, but the renderer does not consume those fields: SkiaModel.Draw still calls TryDrawShapedText(..., paint) / skCanvas.DrawText(text, x, y, paint) for DrawTextCanvasCommand (checked src/Svg.Skia/SkiaModel.cs lines 1922-1924), and the same applies to the old path-based draw call. Any retained model created through this overload with a font size/typeface/scale or alignment that differs from the paint will render incorrectly even though codegen preserves the font.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 568a1081a9bbc4720aecaa0b3507ef49330aa46f.
Retained text replay now consumes the recorded font and alignment state instead of relying only on the paint. The fix adds runtime SKFont conversion for shim SKPaint and shim SKFont, including typeface resolution, size, scale, skew, edging, subpixel, and embolden state. TryDrawShapedText now shapes with the selected SKFont and uses the selected SKTextAlign for shaped-text alignment.
For fallback/runtime APIs that still draw text through SKPaint, SkiaModel.Draw creates a scoped cloned paint when the retained command provides an explicit font or alignment, applies those fields to that clone, and leaves cached render paints untouched. The same pattern is applied to DrawTextOnPathCanvasCommand. Positioned text replay now also keys its blob cache by SKFont signature and uses SKTextBlob.Font when the retained blob carries an explicit font.
I added SKSvgRebuildFromModelTests.ToSKPicture_UsesRecordedTextFontAndAlignment, which renders a retained text command whose recorded font/alignment intentionally differs from the paint and verifies the rendered output lands on the expected side with the larger font.
Validation run after the fix:
dotnet format Svg.Skia.slnx --no-restoredotnet build Svg.Skia.slnx -c Release --no-restore -v:minimal /nologodotnet test Svg.Skia.slnx -c Release --no-restore -v:minimal /nologo
The full build passed with existing unrelated warnings only, and the full test run passed. Svg.Skia.UnitTests now reports 1848 passed and 533 skipped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 568a1081a9
ℹ️ 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".
| if (samplingOptions.UseCubic) | ||
| { | ||
| return SkiaSharp.SKFilterQuality.High; |
There was a problem hiding this comment.
Fresh evidence in this revision is that the replay path now reads Sampling, but this helper collapses every cubic/custom SKSamplingOptions to legacy SKFilterQuality.High (and similarly loses mipmap details for some filter modes). When a retained model records DrawImage with SKCubicResampler.CatmullRom or another non-default sampling option, SkiaModel.Draw still replays a different sampler while generated code emits the exact SKSamplingOptions, so runtime playback can diverge from generated output; use the SKCanvas.DrawImage overload that accepts SKSamplingOptions or convert the options losslessly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 7b535163dcaa3bdb0a97d85bd62937798f1ba36a.
Svg.Skia still compiles against SkiaSharp 2.88.9, so the exact SKSamplingOptions runtime types are not available at compile time. I added a runtime reflection bridge that detects SkiaSharp.SKSamplingOptions, SKFilterMode, SKMipmapMode, SKCubicResampler, and the SKCanvas.DrawImage(SKImage, SKRect, SKRect, SKSamplingOptions, SKPaint) overload when a newer SkiaSharp runtime provides them.
When that API is present, retained replay now constructs native sampling options from the shim values, including custom cubic B and C, and invokes the exact native DrawImage overload. The previous SKFilterQuality conversion is now ToLegacySKFilterQuality and is only used as the SkiaSharp 2.88 fallback path. I also added a retained image null guard and conditioned the linker annotations so net461 and netstandard2.0 keep compiling while modern targets remain trim-analysis clean.
Validation:
dotnet build src/Svg.Skia/Svg.Skia.csproj -c Release --no-restore -v:minimal /nologo-> passed, 0 warnings, 0 errorsdotnet build tests/Svg.Skia.UnitTests/Svg.Skia.UnitTests.csproj -c Release --no-restore -v:minimal /nologo-> passed, 0 warnings, 0 errorsdotnet test tests/Svg.Skia.UnitTests/Svg.Skia.UnitTests.csproj -c Release --no-restore --filter "FullyQualifiedName~SKSvgRebuildFromModelTests.ToSKPicture_UsesRecorded" -v:minimal /nologo-> passed, 2 passed, 0 faileddotnet format Svg.Skia.slnx --no-restore-> completed; unrelatedexternals/SVGformatter churn was restoreddotnet build Svg.Skia.slnx -c Release --no-restore -v:minimal /nologo-> passed, 13 existing package advisory warnings, 0 errorsdotnet test Svg.Skia.slnx -c Release --no-restore -v:minimal /nologo-> passed;Svg.Skia.UnitTestsreported 1848 passed and 533 skipped
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b535163dc
ℹ️ 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 = ResolveSKTypeface(paint.Typeface); | ||
| var skFont = new SkiaSharp.SKFont(typefaceResolution.Typeface, paint.TextSize) | ||
| { | ||
| Edging = paint.LcdRenderText ? SkiaSharp.SKFontEdging.SubpixelAntialias : SkiaSharp.SKFontEdging.Antialias, |
There was a problem hiding this comment.
When a text command is replayed or generated from an SKPaint with IsAntialias=false (for example SVG rendering hints that request crisp text), this conversion still creates an SKFont with Edging=Antialias unless LCD rendering is enabled. The new SkiaSharp text APIs draw with the SKFont's edging, so paint-only text that used to render aliased through SKPaint.IsAntialias=false is now replayed/generated as antialiased; map non-antialiased paints to SKFontEdging.Alias here (and in the codegen helper) to keep output consistent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in d52959951bac927cf3504ae7259ec58bef4036ce.
SkiaModel.ToSKFont(SKPaint) now maps paint-derived fonts with IsAntialias = false to SKFontEdging.Alias. LCD/subpixel edging is only selected when antialiasing remains enabled, so paint-only replay keeps the old aliased text behavior after moving to the newer SKFont text APIs.
I also updated the C# codegen helper for paint-derived fonts to emit SKFontEdging.Alias when the retained SKPaint has antialiasing disabled. That keeps generated output aligned with runtime replay for crisp/aliased text hints.
Regression coverage added:
SKSvgRebuildFromModelTests.ToSKFont_MapsAliasedPaintToAliasEdgingSkiaCSharpCodeGenTests.Generate_UsesAliasFontEdgingForAliasedTextPaint
Validation:
dotnet test tests/Svg.Skia.UnitTests/Svg.Skia.UnitTests.csproj -c Release --no-restore --filter "FullyQualifiedName~ToSKFont_MapsAliasedPaintToAliasEdging|FullyQualifiedName~Generate_UsesAliasFontEdgingForAliasedTextPaint" -v:minimal /nologo-> passed, 2 passed, 0 faileddotnet format Svg.Skia.slnx --no-restore-> completed; unrelatedexternals/SVGformatter churn was restoreddotnet build Svg.Skia.slnx -c Release --no-restore -v:minimal /nologo-> passed, 135 existing unrelated warnings, 0 errorsdotnet test Svg.Skia.slnx -c Release --no-restore -v:minimal /nologo-> passed;Svg.Skia.UnitTestsreported 1850 passed and 533 skipped
1 participant
Fix SkiaSharp Obsolete Warnings in Generated Output
Summary
This PR fixes the SkiaSharp obsolete-warning area reported in issue #293 by migrating generated SkiaSharp 3 output away from deprecated text, font, and image-filter APIs.
The main change is that generated C# now emits
SKFontandSKSamplingOptionsfor SkiaSharp 3 drawing calls instead of placing font and sampling state onSKPaint.What Changed
SKFontandSKFontEdgingSKSamplingOptions,SKFilterMode,SKMipmapMode, andSKCubicResamplerSKCanvas.DrawImage(..., SKSamplingOptions, SKPaint?)SKCanvas.DrawText(..., SKTextAlign, SKFont, SKPaint)SKCanvas.DrawTextOnPath(..., SKTextAlign, SKFont, SKPaint)SKTextBlob.CreatePositioned(string?, SKFont, SKPoint[]?)Svg.CodeGen.Skiaso generated output no longer emits obsolete SkiaSharp 3 APIs such as:SKPaint.TypefaceSKPaint.TextSizeSKPaint.LcdRenderTextSKPaint.SubpixelTextSKPaint.TextEncodingSKPaint.FilterQualitySKPaint.ToFont()SKCanvas.DrawText(string, float, float, SKPaint)SKPaint.FilterQualityvalues toSKSamplingOptionswhen generating code from older command shapes.SKSamplingOptionswhen the new image overload is used.Root Cause
Projects that consume
build/SkiaSharp.v3.propscompile generated C# against SkiaSharp 3. The generated source still used SkiaSharp 2-style APIs where font state and filter quality lived onSKPaint, so clean Release builds emitted obsolete warnings from generated files.SkiaSharp 3 moved text/font state to
SKFont, moved image sampling toSKSamplingOptions, and added drawing overloads that take explicitSKTextAlign,SKFont, and sampling options.Upstream API Notes
The migration was checked against upstream SkiaSharp sources:
binding/SkiaSharp/SKCanvas.csbinding/SkiaSharp/SKPaint.csbinding/SkiaSharp/SKFont.csbinding/SkiaSharp/Definitions.csImportant mappings used:
SKPaint.TextSize->new SKFont(typeface, size)SKPaint.Typeface->new SKFont(typeface, size)SKPaint.LcdRenderText->SKFont.Edging = SKFontEdging.SubpixelAntialiasSKPaint.SubpixelText->SKFont.SubpixelSKFilterQuality.None->new SKSamplingOptions(SKFilterMode.Nearest, SKMipmapMode.None)SKFilterQuality.Low->new SKSamplingOptions(SKFilterMode.Linear, SKMipmapMode.None)SKFilterQuality.Medium->new SKSamplingOptions(SKFilterMode.Linear, SKMipmapMode.Linear)SKFilterQuality.High->new SKSamplingOptions(SKCubicResampler.Mitchell)Validation
dotnet format Svg.Skia.slnx --no-restoredotnet clean Svg.Skia.slnx -c Releasedotnet build Svg.Skia.slnx -c Releasedotnet test Svg.Skia.slnx -c ReleaseValidation result:
Remaining warnings are existing non-scope warnings such as NuGet advisory warnings, nullable warnings in other projects, and unrelated
SvgDeferredPaintServerobsolete warnings.