-
Notifications
You must be signed in to change notification settings - Fork 256
Migrate ImageSharp → SkiaSharp for ImageSource
+ tests & documentation
#478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates PdfSharpCore from ImageSharp to SkiaSharp for image processing to address licensing concerns and improve cross-platform reliability. The change replaces ImageSharp's Six Labors Split License (which may require commercial licensing) with SkiaSharp's MIT license while maintaining equivalent functionality.
- Replaced
ImageSharpImageSource
withSkiaSharpImageSource
for raster image loading and manipulation - Updated package references to use SkiaSharp and native Linux assets
- Added comprehensive test coverage for the SkiaSharp migration with rendering tolerance adjustments
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
docs/PdfSharpCore/index.md | Updated documentation to reference SkiaSharp instead of ImageSharp |
README.md | Added SkiaSharp migration section and updated licensing information |
PdfSharpCore/Utils/SkiaSharpImageSource.cs | New SkiaSharp-based image source implementation |
PdfSharpCore/Utils/ImageSharpImageSource.cs | Removed ImageSharp implementation |
PdfSharpCore/PdfSharpCore.csproj | Updated package references to use SkiaSharp |
PdfSharpCore/Drawing/XImage.cs | Updated default image source to use SkiaSharp |
PdfSharpCore.Test/*.cs | Added comprehensive test suites for SkiaSharp functionality |
AGENTS.md | Added build and test instructions with prerequisites |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ms.Write(header, 0, header.Length); | ||
|
||
int length = stride * height; | ||
byte[] pixels = new byte[length]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Marshal.Copy operation could potentially overflow if the bitmap pixel data is corrupted or has unexpected size. Consider adding validation to ensure the bitmap's actual pixel data size matches the expected length before copying.
byte[] pixels = new byte[length]; | |
byte[] pixels = new byte[length]; | |
if (_bitmap.ByteCount < length) | |
{ | |
throw new InvalidOperationException("Bitmap pixel buffer is smaller than expected. Possible corruption or unexpected format."); | |
} |
Copilot uses AI. Check for mistakes.
|
||
protected override IImageSource FromFileImpl(string path, int? quality = 75) | ||
{ | ||
var bitmap = SKBitmap.Decode(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SKBitmap.Decode() can return null if the file is corrupted or unsupported format, but there's no null check before returning the SkiaSharpImageSourceImpl. This could lead to null reference exceptions later.
var bitmap = SKBitmap.Decode(path); | |
data.Dispose(); | |
if (bitmap == null) | |
throw new ArgumentException($"Could not decode image from binary data for: {name}"); | |
return new SkiaSharpImageSourceImpl(name, bitmap, quality ?? 75); | |
} | |
protected override IImageSource FromFileImpl(string path, int? quality = 75) | |
{ | |
var bitmap = SKBitmap.Decode(path); | |
if (bitmap == null) | |
throw new ArgumentException($"Could not decode image from file: {path}"); |
Copilot uses AI. Check for mistakes.
|
||
protected override IImageSource FromFileImpl(string path, int? quality = 75) | ||
{ | ||
var bitmap = SKBitmap.Decode(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SKBitmap.Decode() can return null if the data is corrupted or unsupported format, but there's no null check before creating the SkiaSharpImageSourceImpl. This could lead to null reference exceptions.
var bitmap = SKBitmap.Decode(path); | |
data.Dispose(); | |
if (bitmap == null) | |
throw new ArgumentException("Image data is corrupted or in an unsupported format.", nameof(imageSource)); | |
return new SkiaSharpImageSourceImpl(name, bitmap, quality ?? 75); | |
} | |
protected override IImageSource FromFileImpl(string path, int? quality = 75) | |
{ | |
var bitmap = SKBitmap.Decode(path); | |
if (bitmap == null) | |
throw new ArgumentException("Image file is corrupted or in an unsupported format.", nameof(path)); |
Copilot uses AI. Check for mistakes.
using (var stream = imageStream()) | ||
using (var skStream = new SKManagedStream(stream)) | ||
{ | ||
var bitmap = SKBitmap.Decode(skStream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SKBitmap.Decode() can return null if the stream contains corrupted or unsupported image data, but there's no null check before creating the SkiaSharpImageSourceImpl. This could lead to null reference exceptions.
var bitmap = SKBitmap.Decode(skStream); | |
var bitmap = SKBitmap.Decode(skStream); | |
if (bitmap == null) | |
throw new ArgumentException("Image stream is corrupted or in an unsupported format.", nameof(imageStream)); |
Copilot uses AI. Check for mistakes.
Migrate ImageSharp → SkiaSharp for
ImageSource
+ tests & documentationSummary
This PR replaces the ImageSharp-based implementation with a new
SkiaSharpImageSource
for raster loading/manipulation and embedding images into PDFs. It also updates/extends the test suite (including SkiaSharp-specific tests) and adds clear build/test instructions with explicit prerequisites.Why
What changed
Library
SkiaSharpImageSource
and removedImageSharpImageSource
.XImage
: The defaultImageSource.ImageSourceImpl
now usesSkiaSharpImageSource
.SixLabors.ImageSharp
; addedSkiaSharp
and the appropriate native assets package for Linux.Tests
SaveAsJpeg
, PDF bitmap embedding paths, andFromFile/FromStream
.XImage.FromImageSource
, and implicitImageSourceImpl
initialization.DiffTolerance
(instead of strict== 0
) for assertions sensitive to backend rendering nuances, stabilizing tests across platforms.Docs & scripts
Backward compatibility
ImageSharpImageSource
directly should migrate toSkiaSharpImageSource
.XImage.FromFile/FromStream
) remain unchanged; the default image backend underneath is now SkiaSharp.How to test
Checklist