Conversation
Co-authored-by: stephentoub <[email protected]>
Co-authored-by: stephentoub <[email protected]>
There was a problem hiding this comment.
Pull request overview
Updates Microsoft.ML.ImageAnalytics to be compatible with SkiaSharp 3.x to prevent runtime MissingMethodException when users bring newer SkiaSharp versions into their apps.
Changes:
- Bumps SkiaSharp to
3.119.2viaeng/Versions.props. - Adds an explicit
SkiaSharppackage reference toMicrosoft.ML.ImageAnalytics.csproj. - Migrates removed/obsolete APIs in
MLImage(replacesGetPixelSpan()withGetPixels()pointer span; replacesSKFilterQualitywithSKSamplingOptions/DrawImage).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Microsoft.ML.ImageAnalytics/Microsoft.ML.ImageAnalytics.csproj | Adds explicit managed SkiaSharp reference (3.x no longer comes transitively via native assets). |
| src/Microsoft.ML.ImageAnalytics/MLImage.cs | Updates pixel access and resize/grayscale code paths to SkiaSharp 3.x APIs. |
| eng/Versions.props | Bumps SkiaSharpVersion to 3.119.2. |
Comments suppressed due to low confidence (1)
src/Microsoft.ML.ImageAnalytics/MLImage.cs:397
- Same as above:
SKImage.FromBitmap(_image)in the crop path adds an extra allocation/native resource per resize. Prefer drawing fromSKBitmapdirectly withSKSamplingOptionsif the SkiaSharp 3.x API supports it, to reduce per-image overhead in pipelines.
using SKCanvas canvas = new SKCanvas(dst);
using SKImage image = SKImage.FromBitmap(_image);
canvas.DrawImage(image, srcRect, destRect, new SKSamplingOptions(SKCubicResampler.Mitchell));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| <PackageReference Include="SkiaSharp" /> | ||
|
|
||
| <!-- SkiaSharp reference Windows, UWP, and MacOS native dependencies packages. No need to explicity reference it here. --> |
There was a problem hiding this comment.
The comment above the native assets reference is now inaccurate: this project explicitly references the managed SkiaSharp package, so “No need to explicitly reference it here” (and the current spelling “explicity”) should be updated to avoid confusing future maintainers.
| <!-- SkiaSharp reference Windows, UWP, and MacOS native dependencies packages. No need to explicity reference it here. --> | |
| <!-- SkiaSharp already includes Windows, UWP, and macOS native dependency packages; only the Linux native assets package needs to be referenced explicitly here. --> |
There was a problem hiding this comment.
Fixed in 00d95ab — updated the comment to accurately describe that SkiaSharp includes Windows/UWP/macOS native assets, and only Linux needs explicit reference.
| Debug.Assert(_image.Info.BytesPerPixel == 4); | ||
|
|
||
| return _image.GetPixelSpan(); | ||
| return new ReadOnlySpan<byte>(_image.GetPixels().ToPointer(), _image.ByteCount); |
There was a problem hiding this comment.
Pixels creates a span directly from _image.GetPixels() without verifying the pointer is non-zero. If GetPixels() returns IntPtr.Zero (e.g., an unallocated/failed bitmap), the returned span will reference invalid memory. Consider validating the pointer (and expected length) and throwing a clear InvalidOperationException if pixels are unavailable.
| return new ReadOnlySpan<byte>(_image.GetPixels().ToPointer(), _image.ByteCount); | |
| var pixelsPtr = _image.GetPixels(); | |
| if (pixelsPtr == IntPtr.Zero || _image.ByteCount <= 0) | |
| throw new InvalidOperationException("Pixel data is unavailable."); | |
| return new ReadOnlySpan<byte>(pixelsPtr.ToPointer(), _image.ByteCount); |
There was a problem hiding this comment.
Added IntPtr.Zero and ByteCount validation in 00d95ab.
| using SKCanvas canvas = new SKCanvas(destBitmap); | ||
| using SKPaint paint = new SKPaint() { FilterQuality = SKFilterQuality.High }; | ||
| using SKImage image = SKImage.FromBitmap(_image); | ||
|
|
||
| canvas.DrawBitmap(_image, srcRect, destRect, paint); | ||
| canvas.DrawImage(image, srcRect, destRect, new SKSamplingOptions(SKCubicResampler.Mitchell)); | ||
|
|
There was a problem hiding this comment.
SKImage.FromBitmap(_image) introduces an extra allocation/native resource for every resize-with-padding call. Since this code can run per-row in pipelines, consider using an API that draws directly from SKBitmap with SKSamplingOptions (if available in the target SkiaSharp version) to avoid creating an intermediate SKImage each time.
This issue also appears on line 393 of the same file.
There was a problem hiding this comment.
In SkiaSharp 3.x, SKCanvas.DrawBitmap doesn't have an overload that accepts SKSamplingOptions — only DrawImage does. So SKImage.FromBitmap is required to use high-quality resampling. The old code's SKPaint.FilterQuality property is obsolete/removed in 3.x.
|
@copilot address the feedback as is necessary / valuable |
…idation Co-authored-by: stephentoub <[email protected]>
Head branch was pushed to by a user without write access
Addressed in 00d95ab:
The Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
eng/Versions.propsMLImage.csto fix breaking API changes:_image.GetPixelSpan()withGetPixels()pointer-based span with IntPtr.Zero validationSKFilterQuality.Nonewithnew SKSamplingOptions(SKFilterMode.Nearest)inResizeFull()SKPaint.FilterQuality = SKFilterQuality.HighwithDrawImage+SKSamplingOptions(SKCubicResampler.Mitchell)inResizeWithPadding()andResizeWithCrop()SKPaint.FilterQualityfromCloneWithGrayscale()SkiaSharppackage reference in csproj (no longer transitively pulled by native assets in 3.x)Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.