Skip to content
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

Expose Convolution Api #2

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,11 @@ public static IImageProcessingContext BoxBlur(this IImageProcessingContext sourc
/// The <see cref="BorderWrappingMode"/> to use when mapping the pixels outside of the border, in Y direction.
/// </param>
/// <returns>The <see cref="IImageProcessingContext"/>.</returns>
public static IImageProcessingContext BoxBlur(this IImageProcessingContext source, int radius, Rectangle rectangle, BorderWrappingMode borderWrapModeX, BorderWrappingMode borderWrapModeY)
{
var processor = new BoxBlurProcessor(radius, borderWrapModeX, borderWrapModeY);
return source.ApplyProcessor(processor, rectangle);
}
public static IImageProcessingContext BoxBlur(
this IImageProcessingContext source,
int radius,
Rectangle rectangle,
BorderWrappingMode borderWrapModeX,
BorderWrappingMode borderWrapModeY)
=> source.ApplyProcessor(new BoxBlurProcessor(radius, borderWrapModeX, borderWrapModeY), rectangle);
Comment on lines +58 to +64
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Consider adding parameter validation for radius >= 0 to prevent invalid kernel sizes

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using SixLabors.ImageSharp.Processing.Processors.Convolution;

namespace SixLabors.ImageSharp.Processing.Extensions.Convolution;

/// <summary>
/// Defines general convolution extensions to apply on an <see cref="Image"/>
/// using Mutate/Clone.
/// </summary>
Comment on lines +9 to +11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: XML comment should mention that these are extension methods for IImageProcessingContext, not just Image

public static class ConvolutionExtensions
{
/// <summary>
/// Applies a convolution filter to the image.
/// </summary>
/// <param name="source">The current image processing context.</param>
/// <param name="kernelXY">The convolution kernel to apply.</param>
/// <returns>The <see cref="IImageProcessingContext"/>.</returns>
public static IImageProcessingContext Convolve(this IImageProcessingContext source, DenseMatrix<float> kernelXY)
=> Convolve(source, kernelXY, false);
Comment on lines +20 to +21
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: No validation for null or empty kernelXY matrix. Consider adding Guard.NotNull and size validation.


/// <summary>
/// Applies a convolution filter to the image.
/// </summary>
/// <param name="source">The current image processing context.</param>
/// <param name="kernelXY">The convolution kernel to apply.</param>
/// <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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Default BorderWrappingMode.Repeat is used but not documented in XML comments


/// <summary>
/// Applies a convolution filter to the image.
/// </summary>
/// <param name="source">The current image processing context.</param>
/// <param name="kernelXY">The convolution kernel to apply.</param>
/// <param name="preserveAlpha">Whether the convolution filter is applied to alpha as well as the color channels.</param>
/// <param name="borderWrapModeX">The <see cref="BorderWrappingMode"/> to use when mapping the pixels outside of the border, in X direction.</param>
/// <param name="borderWrapModeY">The <see cref="BorderWrappingMode"/> to use when mapping the pixels outside of the border, in Y direction.</param>
/// <returns>The <see cref="IImageProcessingContext"/>.</returns>
public static IImageProcessingContext Convolve(
this IImageProcessingContext source,
DenseMatrix<float> kernelXY,
bool preserveAlpha,
BorderWrappingMode borderWrapModeX,
BorderWrappingMode borderWrapModeY)
=> source.ApplyProcessor(new ConvolutionProcessor(kernelXY, preserveAlpha, borderWrapModeX, borderWrapModeY));

/// <summary>
/// Applies a convolution filter to the image.
/// </summary>
/// <param name="source">The current image processing context.</param>
/// <param name="rectangle">The rectangle structure that specifies the portion of the image object to alter.</param>
/// <param name="kernelXY">The convolution kernel to apply.</param>
/// <returns>The <see cref="IImageProcessingContext"/>.</returns>
public static IImageProcessingContext Convolve(this IImageProcessingContext source, Rectangle rectangle, DenseMatrix<float> kernelXY)
=> Convolve(source, rectangle, kernelXY, false);

/// <summary>
/// Applies a convolution filter to the image.
/// </summary>
/// <param name="source">The current image processing context.</param>
/// <param name="rectangle">The rectangle structure that specifies the portion of the image object to alter.</param>
/// <param name="kernelXY">The convolution kernel to apply.</param>
/// <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, Rectangle rectangle, DenseMatrix<float> kernelXY, bool preserveAlpha)
=> Convolve(source, rectangle, kernelXY, preserveAlpha, BorderWrappingMode.Repeat, BorderWrappingMode.Repeat);

/// <summary>
/// Applies a convolution filter to the image.
/// </summary>
/// <param name="source">The current image processing context.</param>
/// <param name="rectangle">The rectangle structure that specifies the portion of the image object to alter.</param>
/// <param name="kernelXY">The convolution kernel to apply.</param>
/// <param name="preserveAlpha">Whether the convolution filter is applied to alpha as well as the color channels.</param>
/// <param name="borderWrapModeX">The <see cref="BorderWrappingMode"/> to use when mapping the pixels outside of the border, in X direction.</param>
/// <param name="borderWrapModeY">The <see cref="BorderWrappingMode"/> to use when mapping the pixels outside of the border, in Y direction.</param>
/// <returns>The <see cref="IImageProcessingContext"/>.</returns>
public static IImageProcessingContext Convolve(
this IImageProcessingContext source,
Rectangle rectangle,
DenseMatrix<float> kernelXY,
bool preserveAlpha,
BorderWrappingMode borderWrapModeX,
BorderWrappingMode borderWrapModeY)
=> source.ApplyProcessor(new ConvolutionProcessor(kernelXY, preserveAlpha, borderWrapModeX, borderWrapModeY), rectangle);
}
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public static IImageProcessingContext GaussianBlur(this IImageProcessingContext
/// <returns>The <see cref="IImageProcessingContext"/>.</returns>
public static IImageProcessingContext GaussianBlur(this IImageProcessingContext source, float sigma, Rectangle rectangle, BorderWrappingMode borderWrapModeX, BorderWrappingMode borderWrapModeY)
{
var processor = new GaussianBlurProcessor(sigma, borderWrapModeX, borderWrapModeY);
GaussianBlurProcessor processor = new(sigma, borderWrapModeX, borderWrapModeY);
return source.ApplyProcessor(processor, rectangle);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,14 @@ protected override void OnFrameApply(ImageFrame<TPixel> source)

source.CopyTo(targetPixels);

var interest = Rectangle.Intersect(this.SourceRectangle, source.Bounds());
Rectangle interest = Rectangle.Intersect(this.SourceRectangle, source.Bounds());

using (var map = new KernelSamplingMap(allocator))
using (KernelSamplingMap map = new(allocator))
{
// Since the kernel sizes are identical we can use a single map.
map.BuildSamplingOffsetMap(this.KernelY, interest);

var operation = new Convolution2DRowOperation<TPixel>(
Convolution2DRowOperation<TPixel> operation = new(
interest,
targetPixels,
source.PixelBuffer,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,48 @@ public Convolution2PassProcessor(
Rectangle sourceRectangle,
BorderWrappingMode borderWrapModeX,
BorderWrappingMode borderWrapModeY)
: this(configuration, kernel, kernel, preserveAlpha, source, sourceRectangle, borderWrapModeX, borderWrapModeY)
{
}

/// <summary>
/// Initializes a new instance of the <see cref="Convolution2PassProcessor{TPixel}"/> class.
/// </summary>
/// <param name="configuration">The configuration which allows altering default behaviour or extending the library.</param>
/// <param name="kernelX">The 1D convolution kernel. X Direction</param>
/// <param name="kernelY">The 1D convolution kernel. Y Direction</param>
/// <param name="preserveAlpha">Whether the convolution filter is applied to alpha as well as the color channels.</param>
/// <param name="source">The source <see cref="Image{TPixel}"/> for the current processor instance.</param>
/// <param name="sourceRectangle">The source area to process for the current processor instance.</param>
/// <param name="borderWrapModeX">The <see cref="BorderWrappingMode"/> to use when mapping the pixels outside of the border, in X direction.</param>
/// <param name="borderWrapModeY">The <see cref="BorderWrappingMode"/> to use when mapping the pixels outside of the border, in Y direction.</param>
public Convolution2PassProcessor(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: No validation that kernelX and kernelY are non-null or have valid lengths

Configuration configuration,
float[] kernelX,
float[] kernelY,
bool preserveAlpha,
Image<TPixel> source,
Rectangle sourceRectangle,
BorderWrappingMode borderWrapModeX,
BorderWrappingMode borderWrapModeY)
: base(configuration, source, sourceRectangle)
{
this.Kernel = kernel;
this.KernelX = kernelX;
this.KernelY = kernelY;
this.PreserveAlpha = preserveAlpha;
this.BorderWrapModeX = borderWrapModeX;
this.BorderWrapModeY = borderWrapModeY;
}

/// <summary>
/// Gets the convolution kernel.
/// Gets the convolution kernel. X direction.
/// </summary>
public float[] KernelX { get; }

/// <summary>
/// Gets the convolution kernel. Y direction.
/// </summary>
public float[] Kernel { get; }
public float[] KernelY { get; }

/// <summary>
/// Gets a value indicating whether the convolution filter is applied to alpha as well as the color channels.
Expand All @@ -68,21 +98,21 @@ protected override void OnFrameApply(ImageFrame<TPixel> source)
{
using Buffer2D<TPixel> firstPassPixels = this.Configuration.MemoryAllocator.Allocate2D<TPixel>(source.Size);

var interest = Rectangle.Intersect(this.SourceRectangle, source.Bounds());
Rectangle interest = Rectangle.Intersect(this.SourceRectangle, source.Bounds());

// We can create a single sampling map with the size as if we were using the non separated 2D kernel
// the two 1D kernels represent, and reuse it across both convolution steps, like in the bokeh blur.
using var mapXY = new KernelSamplingMap(this.Configuration.MemoryAllocator);
using KernelSamplingMap mapXY = new(this.Configuration.MemoryAllocator);

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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Using KernelX.Length for both dimensions could be incorrect if kernels have different sizes


// Horizontal convolution
var horizontalOperation = new HorizontalConvolutionRowOperation(
HorizontalConvolutionRowOperation horizontalOperation = new(
interest,
firstPassPixels,
source.PixelBuffer,
mapXY,
this.Kernel,
this.KernelX,
this.Configuration,
this.PreserveAlpha);

Expand All @@ -92,12 +122,12 @@ protected override void OnFrameApply(ImageFrame<TPixel> source)
in horizontalOperation);

// Vertical convolution
var verticalOperation = new VerticalConvolutionRowOperation(
VerticalConvolutionRowOperation verticalOperation = new(
interest,
source.PixelBuffer,
firstPassPixels,
mapXY,
this.Kernel,
this.KernelY,
this.Configuration,
this.PreserveAlpha);

Expand Down Expand Up @@ -140,7 +170,7 @@ public HorizontalConvolutionRowOperation(
}

/// <inheritdoc/>
[MethodImpl(InliningOptions.ShortMethod)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public int GetRequiredBufferLength(Rectangle bounds)
=> 2 * bounds.Width;

Expand Down Expand Up @@ -306,7 +336,7 @@ public VerticalConvolutionRowOperation(
}

/// <inheritdoc/>
[MethodImpl(InliningOptions.ShortMethod)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public int GetRequiredBufferLength(Rectangle bounds)
=> 2 * bounds.Width;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using SixLabors.ImageSharp.PixelFormats;

namespace SixLabors.ImageSharp.Processing.Processors.Convolution;

/// <summary>
/// Defines a processor that uses a 2 dimensional matrix to perform convolution against an image.
/// </summary>
public class ConvolutionProcessor : IImageProcessor
{
/// <summary>
/// Initializes a new instance of the <see cref="ConvolutionProcessor"/> class.
/// </summary>
/// <param name="kernelXY">The 2d gradient operator.</param>
/// <param name="preserveAlpha">Whether the convolution filter is applied to alpha as well as the color channels.</param>
/// <param name="borderWrapModeX">The <see cref="BorderWrappingMode"/> to use when mapping the pixels outside of the border, in X direction.</param>
/// <param name="borderWrapModeY">The <see cref="BorderWrappingMode"/> to use when mapping the pixels outside of the border, in Y direction.</param>
public ConvolutionProcessor(
in DenseMatrix<float> kernelXY,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: kernelXY parameter should be validated for null and dimensions > 0

bool preserveAlpha,
BorderWrappingMode borderWrapModeX,
BorderWrappingMode borderWrapModeY)
{
this.KernelXY = kernelXY;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: consider making a defensive copy of kernelXY to prevent external modification

this.PreserveAlpha = preserveAlpha;
this.BorderWrapModeX = borderWrapModeX;
this.BorderWrapModeY = borderWrapModeY;
}

/// <summary>
/// Gets the 2d convolution kernel.
/// </summary>
public DenseMatrix<float> KernelXY { get; }

/// <summary>
/// Gets a value indicating whether the convolution filter is applied to alpha as well as the color channels.
/// </summary>
public bool PreserveAlpha { get; }

/// <summary>
/// Gets the <see cref="BorderWrappingMode"/> to use when mapping the pixels outside of the border, in X direction.
/// </summary>
public BorderWrappingMode BorderWrapModeX { get; }

/// <summary>
/// Gets the <see cref="BorderWrappingMode"/> to use when mapping the pixels outside of the border, in Y direction.
/// </summary>
public BorderWrappingMode BorderWrapModeY { get; }

/// <inheritdoc/>
public IImageProcessor<TPixel> CreatePixelSpecificProcessor<TPixel>(Configuration configuration, Image<TPixel> source, Rectangle sourceRectangle)
where TPixel : unmanaged,
IPixel<TPixel>
{
if (this.KernelXY.TryGetLinearlySeparableComponents(out float[]? kernelX, out float[]? kernelY))
{
return new Convolution2PassProcessor<TPixel>(
configuration,
kernelX,
kernelY,
this.PreserveAlpha,
source,
sourceRectangle,
this.BorderWrapModeX,
this.BorderWrapModeY);
}
Comment on lines +57 to +68
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: check that kernelX and kernelY are not null before using them, even though they are nullable


return new ConvolutionProcessor<TPixel>(
configuration,
this.KernelXY,
this.PreserveAlpha,
source,
sourceRectangle,
this.BorderWrapModeX,
this.BorderWrapModeY);
}
}
Loading
Loading