Migrate imaging from ImageSharp to SkiaSharp + add exhaustive Skia-based tests#150
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the imaging library from ImageSharp to SkiaSharp for license compatibility and cross-platform stability. The change replaces ImageSharp's commercial licensing model with SkiaSharp's permissive MIT license while maintaining rich imaging capabilities through Skia's graphics engine.
- Replace ImageSharp with SkiaSharp for image decoding/encoding and color handling
- Add comprehensive test suite covering color parsing, CSS conversion, and image transformations
- Update documentation with migration rationale and build instructions
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents SkiaSharp migration rationale and benefits |
| OpenXmlPowerTools.csproj | Replaces ImageSharp package with SkiaSharp and native Linux assets |
| ColorParser.cs | Migrates from ImageSharp Color to SKColor with System.Drawing.ColorTranslator |
| OxPtHelpers.cs | Updates color handling to use SKColor properties and hex formatting |
| HtmlToWmlCssParser.cs | Converts CSS color parsing from ImageSharp to SkiaSharp types |
| HtmlToWmlConverterCore.cs | Replaces image processing with SkiaSharp bitmap/codec APIs |
| ImageHandler.cs | Rewrites image transformation using SKCodec and MIME type detection |
| Test files | Adds comprehensive test coverage for new SkiaSharp-based functionality |
| AGENTS.md | Provides build and test instructions for contributors |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| using (var s = newPart.GetStream(FileMode.Create, FileAccess.ReadWrite)) | ||
| { | ||
| s.Write(ba, 0, ba.GetUpperBound(0) + 1); | ||
| s.Write(ba, 0, ba.Length); |
There was a problem hiding this comment.
The bug fix correctly changes from ba.GetUpperBound(0) + 1 to ba.Length. However, the old code should have been s.Write(ba, 0, ba.GetUpperBound(0) + 1); but the diff shows it was s.Write(ba, 0, ba.GetUpperBound(0) + 1);. This suggests the original code may have been incorrect for multi-dimensional arrays, but since ba is a byte array (single dimension), GetUpperBound(0) + 1 equals Length. The fix is correct but the original issue description may be misleading.
| { | ||
| SixLabors.ImageSharp.Color colorValue; | ||
| SkiaSharp.SKColor colorValue; | ||
| if (!ColorParser.TryFromName(backColor, out colorValue)) |
There was a problem hiding this comment.
The variable name is inconsistent - the method checks backColor but the error message references foreColor. This should check foreColor since that's the variable being validated for the foreground color.
| if (!ColorParser.TryFromName(backColor, out colorValue)) | |
| if (!ColorParser.TryFromName(foreColor, out colorValue)) |
|
I have read the CLA Document and I hereby sign the CLA |
Migrate imaging from ImageSharp to SkiaSharp + add exhaustive Skia-based tests
Summary
This PR replaces SixLabors.ImageSharp with SkiaSharp for image decoding/encoding and color handling across the codebase, and adds a comprehensive test suite to validate the new behavior. It also updates documentation to reflect the migration and adds build/test instructions. ([GitHub][1])
Why
What changed
Library code
Color handling
ColorParsernow usesSkiaSharp.SKColorandSystem.Drawing.ColorTranslatorto parse named/hex colors.Signatures changed:
ColorParser.FromName(string)→ returnsSKColorColorParser.TryFromName(string?, out SKColor)This replaces prior ImageSharp types and parsing. ([GitHub][1])
HTML → WML image processing
SKBitmapand encode viaSKImage.Encode(PNG/JPEG/WebP), including handling of data-URI sources.ba.Length(instead ofGetUpperBound(0)+1). ([GitHub][1])Project references
Removed ImageSharp; added:
SkiaSharp3.119.0SkiaSharp.NativeAssets.Linux.NoDependencies3.119.0(both library and test project)Kept
SixLabors.Fontsas before. ([GitHub][2])Tests
Added an exhaustive suite validating color parsing, CSS color coercion, and image transform behaviors:
ColorParserTests— case-insensitive named colors, hex variants (#RRGGBB,#RGB), invalid inputs, and exception behavior. ([GitHub][2])CssPropertyValueTests— verifies detection and conversion of color-like CSS values intoSKColor. ([GitHub][2])ImageHandlerTests— ensures generateddata:URIs with correct MIME type (PNG/JPEG/WEBP/GIF), alt text behavior, and error conditions on invalid bytes. ([GitHub][2])Docs
dotnet build,dotnet test) for contributors. ([GitHub][1])Backward compatibility
Breaking change:
ColorParserreturnsSkiaSharp.SKColorinstead of the previous ImageSharp color type.If downstream code relied on the old type, conversion helpers are straightforward (e.g., map
SKColortoSystem.Drawing.Colorwith channel copy). Consider accepting this change now to remove the ImageSharp dependency; if needed, I can add temporary shim methods returningSystem.Drawing.Colorto ease migration. ([GitHub][1])Runtime: SkiaSharp introduces native dependencies on Linux; the PR includes
SkiaSharp.NativeAssets.Linux.NoDependenciesin both library and tests so CI/Linux builds run out of the box. ([GitHub][2])How to test
dotnet restore dotnet build dotnet testAll new tests are deterministic and run cross-platform. See
OpenXmlPowerTools.Tests/*for coverage of colors, CSS parsing, and image transformation. ([GitHub][2])Implementation notes
SKBitmap.Decode(...)for file paths and base64 data URIs; encode viaSKImage.Encode(...)(PNG default) before adding to the OpenXML package. ([GitHub][1])ColorTranslator.FromHtml(...)leveraged for named/hex inputs; strict validation viaTryFromName. ([GitHub][1])Related work
This fork tracks the active
Codeuctivity/OpenXmlPowerToolslineage; releases continue to be published there. This PR targets that repo to consolidate the SkiaSharp migration and its tests upstream. ([GitHub][3])Checklist