Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
@@ -1,6 +1,8 @@
// Copyright (c) Six Labors.
// Licensed under the Apache License, Version 2.0.

using System;

namespace SixLabors.ImageSharp.Processing
{
/// <summary>
Expand Down Expand Up @@ -29,9 +31,11 @@ public static IImageProcessingContext Pad(this IImageProcessingContext source, i
/// <returns>The <see cref="IImageProcessingContext"/> to allow chaining of operations.</returns>
public static IImageProcessingContext Pad(this IImageProcessingContext source, int width, int height, Color color)
{
Size size = source.GetCurrentSize();
var options = new ResizeOptions
{
Size = new Size(width, height),
// Prevent downsizing.
Size = new Size(Math.Max(width, size.Width), Math.Max(height, size.Height)),
Mode = ResizeMode.BoxPad,
Sampler = KnownResamplers.NearestNeighbor,
PadColor = color
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ protected virtual void Dispose(bool disposing)
/// Returns a <see cref="ResizeKernel"/> for an index value between 0 and DestinationSize - 1.
/// </summary>
[MethodImpl(InliningOptions.ShortMethod)]
internal ref ResizeKernel GetKernel(int destIdx) => ref this.kernels[destIdx];
internal ref ResizeKernel GetKernel(nint destIdx) => ref this.kernels[destIdx];
Copy link
Contributor

Choose a reason for hiding this comment

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

For the plain array access nint doesn't bring any advantage.
Here the JIT misses a optimization (a bug is tracked, but can't find it at the moment).

If you want best machine code here, look at Sharplab method GetKernelC.
It's more C# code and I doubt it's needed here. I'd just go with int (or nint as you have it now) and wait for the JIT fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

No MemoryMarshal.GetArrayDataReference for us yet unfortunately..... One day!


/// <summary>
/// Computes the weights to apply at each pixel when resizing.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ public void ApplyTransform<TResampler>(in TResampler sampler)
Rectangle destinationRectangle = this.destinationRectangle;
bool compand = this.options.Compand;
bool premultiplyAlpha = this.options.PremultiplyAlpha;
TPixel fillColor = this.options.PadColor.ToPixel<TPixel>();
bool shouldFill = (this.options.Mode == ResizeMode.BoxPad || this.options.Mode == ResizeMode.Pad)
&& this.options.PadColor != default;
TPixel fillColor = this.options.PadColor.ToPixel<TPixel>();

// Handle resize dimensions identical to the original
if (source.Width == destination.Width
Expand Down Expand Up @@ -209,21 +209,18 @@ private static void ApplyResizeFrameTransform(

// To reintroduce parallel processing, we would launch multiple workers
// for different row intervals of the image.
using (var worker = new ResizeWorker<TPixel>(
using var worker = new ResizeWorker<TPixel>(
configuration,
sourceRegion,
conversionModifiers,
horizontalKernelMap,
verticalKernelMap,
destination.Width,
interest,
destinationRectangle.Location))
{
worker.Initialize();
destinationRectangle.Location);
worker.Initialize();

var workingInterval = new RowInterval(interest.Top, interest.Bottom);
worker.FillDestinationPixels(workingInterval, destination.PixelBuffer);
}
var workingInterval = new RowInterval(interest.Top, interest.Bottom);
worker.FillDestinationPixels(workingInterval, destination.PixelBuffer);
}

private readonly struct NNRowOperation : IRowOperation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ internal sealed class ResizeWorker<TPixel> : IDisposable

private readonly ResizeKernelMap verticalKernelMap;

private readonly int destWidth;

private readonly Rectangle targetWorkingRect;

private readonly Point targetOrigin;
Expand All @@ -57,7 +55,6 @@ public ResizeWorker(
PixelConversionModifiers conversionModifiers,
ResizeKernelMap horizontalKernelMap,
ResizeKernelMap verticalKernelMap,
int destWidth,
Rectangle targetWorkingRect,
Point targetOrigin)
{
Expand All @@ -67,7 +64,6 @@ public ResizeWorker(
this.conversionModifiers = conversionModifiers;
this.horizontalKernelMap = horizontalKernelMap;
this.verticalKernelMap = verticalKernelMap;
this.destWidth = destWidth;
this.targetWorkingRect = targetWorkingRect;
this.targetOrigin = targetOrigin;

Expand All @@ -80,19 +76,19 @@ public ResizeWorker(

int numberOfWindowBands = ResizeHelper.CalculateResizeWorkerHeightInWindowBands(
this.windowBandHeight,
destWidth,
targetWorkingRect.Width,
workingBufferLimitHintInBytes);

this.workerHeight = Math.Min(this.sourceRectangle.Height, numberOfWindowBands * this.windowBandHeight);

this.transposedFirstPassBuffer = configuration.MemoryAllocator.Allocate2D<Vector4>(
this.workerHeight,
destWidth,
targetWorkingRect.Width,
preferContiguosImageBuffers: true,
options: AllocationOptions.Clean);

this.tempRowBuffer = configuration.MemoryAllocator.Allocate<Vector4>(this.sourceRectangle.Width);
this.tempColumnBuffer = configuration.MemoryAllocator.Allocate<Vector4>(destWidth);
this.tempColumnBuffer = configuration.MemoryAllocator.Allocate<Vector4>(targetWorkingRect.Width);

this.currentWindow = new RowInterval(0, this.workerHeight);
}
Expand All @@ -118,6 +114,9 @@ public void FillDestinationPixels(RowInterval rowInterval, Buffer2D<TPixel> dest
// When creating transposedFirstPassBuffer, we made sure it's contiguous:
Span<Vector4> transposedFirstPassBufferSpan = this.transposedFirstPassBuffer.DangerousGetSingleSpan();

int left = this.targetWorkingRect.Left;
int right = this.targetWorkingRect.Right;
int width = this.targetWorkingRect.Width;
for (int y = rowInterval.Min; y < rowInterval.Max; y++)
{
// Ensure offsets are normalized for cropping and padding.
Expand All @@ -131,17 +130,18 @@ public void FillDestinationPixels(RowInterval rowInterval, Buffer2D<TPixel> dest
ref Vector4 tempRowBase = ref MemoryMarshal.GetReference(tempColSpan);

int top = kernel.StartIndex - this.currentWindow.Min;

ref Vector4 fpBase = ref transposedFirstPassBufferSpan[top];

for (int x = 0; x < this.destWidth; x++)
for (nint x = left; x < right; x++)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it's good to have nint, as in the Unsafe.Adds below this removes the sign extending move movsxd which is beneficial.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to have the loop as

Suggested change
for (nint x = left; x < right; x++)
for (nint x = 0; x < (right - left); x++)

so that the subtractions below x - left can be eliminated (shifted to the loop variable), so that it's just x.
I assume current JIT won't (completely) CSE that expression.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot. Not sure how I missed that.

{
ref Vector4 firstPassColumnBase = ref Unsafe.Add(ref fpBase, x * this.workerHeight);
ref Vector4 firstPassColumnBase = ref Unsafe.Add(ref fpBase, (x - left) * this.workerHeight);

// Destination color components
Unsafe.Add(ref tempRowBase, x) = kernel.ConvolveCore(ref firstPassColumnBase);
Unsafe.Add(ref tempRowBase, x - left) = kernel.ConvolveCore(ref firstPassColumnBase);
}

Span<TPixel> targetRowSpan = destination.DangerousGetRowSpan(y);
Span<TPixel> targetRowSpan = destination.DangerousGetRowSpan(y).Slice(left, width);

PixelOperations<TPixel>.Instance.FromVector4Destructive(this.configuration, tempColSpan, targetRowSpan, this.conversionModifiers);
}
Expand Down Expand Up @@ -170,6 +170,8 @@ private void CalculateFirstPassValues(RowInterval calculationInterval)
Span<Vector4> tempRowSpan = this.tempRowBuffer.GetSpan();
Span<Vector4> transposedFirstPassBufferSpan = this.transposedFirstPassBuffer.DangerousGetSingleSpan();

int left = this.targetWorkingRect.Left;
int right = this.targetWorkingRect.Right;
for (int y = calculationInterval.Min; y < calculationInterval.Max; y++)
{
Span<TPixel> sourceRow = this.source.DangerousGetRowSpan(y);
Expand All @@ -184,13 +186,13 @@ private void CalculateFirstPassValues(RowInterval calculationInterval)
// Span<Vector4> firstPassSpan = transposedFirstPassBufferSpan.Slice(y - this.currentWindow.Min);
ref Vector4 firstPassBaseRef = ref transposedFirstPassBufferSpan[y - this.currentWindow.Min];

for (int x = this.targetWorkingRect.Left; x < this.targetWorkingRect.Right; x++)
for (nint x = left; x < right; x++)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here:

Suggested change
for (nint x = left; x < right; x++)
for (nint x = 0; x < (right - left); x++)

?
And adjust below to just x instead of x - left.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I have to add an extra incremental here. I need both x & x - left.

{
ResizeKernel kernel = this.horizontalKernelMap.GetKernel(x - this.targetOrigin.X);

// optimization for:
// firstPassSpan[x * this.workerHeight] = kernel.Convolve(tempRowSpan);
Unsafe.Add(ref firstPassBaseRef, x * this.workerHeight) = kernel.Convolve(tempRowSpan);
Unsafe.Add(ref firstPassBaseRef, (x - left) * this.workerHeight) = kernel.Convolve(tempRowSpan);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,8 @@ public void ResizeWithBoxPadMode<TPixel>(TestImageProvider<TPixel> provider)
var options = new ResizeOptions
{
Size = new Size(image.Width + 200, image.Height + 200),
Mode = ResizeMode.BoxPad
Mode = ResizeMode.BoxPad,
PadColor = Color.HotPink
};

image.Mutate(x => x.Resize(options));
Expand Down Expand Up @@ -580,7 +581,8 @@ public void ResizeWithPadMode<TPixel>(TestImageProvider<TPixel> provider)
var options = new ResizeOptions
{
Size = new Size(image.Width + 200, image.Height),
Mode = ResizeMode.Pad
Mode = ResizeMode.Pad,
PadColor = Color.Lavender
};

image.Mutate(x => x.Resize(options));
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.