Expose Convolution Api #2
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 file(s) reviewed, 22 comment(s)
Edit PR Review Bot Settings | Greptile
| public static IImageProcessingContext BoxBlur( | ||
| this IImageProcessingContext source, | ||
| int radius, | ||
| Rectangle rectangle, | ||
| BorderWrappingMode borderWrapModeX, | ||
| BorderWrappingMode borderWrapModeY) | ||
| => source.ApplyProcessor(new BoxBlurProcessor(radius, borderWrapModeX, borderWrapModeY), rectangle); |
There was a problem hiding this comment.
logic: Consider adding parameter validation for radius >= 0 to prevent invalid kernel sizes
| public static IImageProcessingContext Convolve(this IImageProcessingContext source, DenseMatrix<float> kernelXY) | ||
| => Convolve(source, kernelXY, false); |
There was a problem hiding this comment.
logic: No validation for null or empty kernelXY matrix. Consider adding Guard.NotNull and size validation.
| /// <param name="preserveAlpha">Whether the convolution filter is applied to alpha as well as the color channels.</param> | ||
| /// <returns>The <see cref="IImageProcessingContext"/>.</returns> | ||
| public static IImageProcessingContext Convolve(this IImageProcessingContext source, DenseMatrix<float> kernelXY, bool preserveAlpha) | ||
| => Convolve(source, kernelXY, preserveAlpha, BorderWrappingMode.Repeat, BorderWrappingMode.Repeat); |
There was a problem hiding this comment.
style: Default BorderWrappingMode.Repeat is used but not documented in XML comments
| /// Defines general convolution extensions to apply on an <see cref="Image"/> | ||
| /// using Mutate/Clone. | ||
| /// </summary> |
There was a problem hiding this comment.
style: XML comment should mention that these are extension methods for IImageProcessingContext, not just Image
|
|
||
| mapXY.BuildSamplingOffsetMap(this.Kernel.Length, this.Kernel.Length, interest, this.BorderWrapModeX, this.BorderWrapModeY); | ||
| mapXY.BuildSamplingOffsetMap(this.KernelX.Length, this.KernelX.Length, interest, this.BorderWrapModeX, this.BorderWrapModeY); |
There was a problem hiding this comment.
logic: Using KernelX.Length for both dimensions could be incorrect if kernels have different sizes
| Image<TPixel> source, | ||
| Rectangle sourceRectangle, | ||
| BorderWrappingMode borderWrapModeX, | ||
| BorderWrappingMode borderWrapModeY) | ||
| : base(configuration, source, sourceRectangle) | ||
| { | ||
| this.KernelXY = kernelXY; |
There was a problem hiding this comment.
logic: no validation that kernelXY is not empty or null
|
|
||
| // We need a clean copy for each pass to start from | ||
| using ImageFrame<TPixel> cleanCopy = source.Clone(); | ||
|
|
||
| using (var processor = new ConvolutionProcessor<TPixel>(this.Configuration, in this.kernels[0], true, this.Source, interest)) | ||
| using (ConvolutionProcessor<TPixel> processor = new(this.Configuration, in this.kernels[0], true, this.Source, interest)) |
There was a problem hiding this comment.
style: Consider adding BorderWrappingMode parameters here to match the new convolution API
| @@ -78,12 +78,12 @@ protected override void OnFrameApply(ImageFrame<TPixel> source) | |||
| { | |||
| using ImageFrame<TPixel> pass = cleanCopy.Clone(); | |||
|
|
|||
| using (var processor = new ConvolutionProcessor<TPixel>(this.Configuration, in this.kernels[i], true, this.Source, interest)) | |||
| using (ConvolutionProcessor<TPixel> processor = new(this.Configuration, in this.kernels[i], true, this.Source, interest)) | |||
There was a problem hiding this comment.
style: Consider adding BorderWrappingMode parameters here to match the new convolution API
| [GroupOutput("Convolution")] | ||
| public class ConvolutionTests | ||
| { | ||
| private static readonly ImageComparer ValidatorComparer = ImageComparer.TolerantPercentage(0.05F); |
There was a problem hiding this comment.
style: 5% tolerance seems high for convolution operations - could mask subtle errors in kernel application
| [Theory] | ||
| [WithFileCollection(nameof(InputImages), nameof(Values), PixelTypes.Rgba32)] | ||
| public void OnFullImage<TPixel>(TestImageProvider<TPixel> provider, DenseMatrix<float> value) | ||
| where TPixel : unmanaged, IPixel<TPixel> | ||
| => provider.RunValidatingProcessorTest( | ||
| x => x.Convolve(value), | ||
| string.Join('_', value.Data), | ||
| ValidatorComparer); |
There was a problem hiding this comment.
logic: Missing tests for preserveAlpha and border wrapping mode parameters that exist in the ConvolutionProcessor
Prerequisites
Description
See the roadmap discussion at SixLabors#2533
Adds a general convolution API that accepts a
DenseMatrix<float>to describe the convolution kernel.All this stuff has been the plumbing for various processors over the years and should be exposed publicly now it's solid.
Greptile Summary
This PR exposes a new public convolution API in ImageSharp, allowing users to apply custom convolution kernels to images while maintaining the library's existing performance optimizations and border handling capabilities.
Key changes:
ConvolutionExtensions.csexposing public methods to apply custom convolution kernels usingDenseMatrix<float>The changes look well-structured and maintain ImageSharp's existing patterns, though test coverage could be expanded to verify edge cases and different border handling modes.