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
Expand Up @@ -71,7 +71,7 @@ protected override void OnFrameApply(ImageFrame<TPixelDst> source, Rectangle sou
Rectangle bounds = targetImage.Bounds();

int minX = Math.Max(this.Location.X, sourceRectangle.X);
int maxX = Math.Min(this.Location.X + bounds.Width, sourceRectangle.Width);
int maxX = Math.Min(this.Location.X + bounds.Width, sourceRectangle.Right);
int targetX = minX - this.Location.X;

int minY = Math.Max(this.Location.Y, sourceRectangle.Y);
Expand All @@ -81,6 +81,12 @@ protected override void OnFrameApply(ImageFrame<TPixelDst> source, Rectangle sou

var workingRect = Rectangle.FromLTRB(minX, minY, maxX, maxY);

// not a valid operation because rectangle does not overlap with this image.
Guard.IsFalse(
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, we only use Guard for trivial argument validation at the beginning of the methods.

I think this is what we should do:

  • Throw an ImageProcessingException if the condition is not matched
  • In ImageProcessor<T>.Apply(...) we should detect (catch and rethrow) ImageProcessingException

@dlemstra @JimBobSquarePants thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I think this check is superfluous and the errors in ParallelHelper.ValidateRectangle should be ArgumentOutOfRangeException via Guard.MustBeGreaterThan.

We already capture the inner exception and throw an ImageProcessingException in release mode.

@atruskie sorry to be messing you around here.

Copy link
Member

Choose a reason for hiding this comment

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

My comment was actually addressing a concern from @atruskie:

I 'm pretty unhappy with the gymnastics required to expect an exception from an ImageProcessor. Any suggestions?

However, the more I think about fixing this, the further it leads, so let's not waste time on it in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@antonfirsov I've followed your advice here already, and it leads to a nicer test.

@JimBobSquarePants I'd like to leave both errors in because they represent different types of error. One is descriptive for a user mistake. The other, in IterRows, really is exceptional and represents that that code should never have been called with the given argument.

Copy link
Member

Choose a reason for hiding this comment

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

I’m trying to avoid as much as possible, per-processor error handling. A negative rectangle would cause an error in many places and multiple handlers would have to be added.

I do wonder whether we should be using Recangle.Intersect to avoid the possibility as we do in several places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't see this last message before I pushed.

I feel like I'm not doing a good job at understanding what you'd like. The options:

  1. Throw in DrawImage if there is not an overlap with a descriptive error message (my preference)
  2. Have no-effect in DrawImage if there is not an overlap (my original version of this fix, possibly what you mean by doing Rectangle.Intersect checks)?
  3. Rely on the exception in IterRows when a non-overlap occurs (I think this is only marginally better than the original confusing error message this PR is seeking to address)
  4. Some other option?

I'm happy to keep making changes until I get this right :-) , it just feels a little like walking in circles.

Copy link
Member

Choose a reason for hiding this comment

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

In retrospect I think you're doing the correct thing for now with the API we have. I'm going to update and merge this. Thanks for your help! 😄

workingRect.Width <= 0 || workingRect.Height <= 0,
nameof(this.Location),
"Cannot draw image because the source image does not overlap the target image.");

ParallelHelper.IterateRows(
workingRect,
configuration,
Expand Down
17 changes: 16 additions & 1 deletion src/ImageSharp/Common/ParallelUtils/ParallelHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public static void IterateRows(
in ParallelExecutionSettings parallelSettings,
Action<RowInterval> body)
{
DebugGuard.MustBeGreaterThan(rectangle.Width, 0, nameof(rectangle));
ValidateRectangle(rectangle);

int maxSteps = DivideCeil(rectangle.Width * rectangle.Height, parallelSettings.MinimumPixelsProcessedPerTask);

Expand Down Expand Up @@ -87,6 +87,8 @@ public static void IterateRowsWithTempBuffer<T>(
Action<RowInterval, Memory<T>> body)
where T : unmanaged
{
ValidateRectangle(rectangle);

int maxSteps = DivideCeil(rectangle.Width * rectangle.Height, parallelSettings.MinimumPixelsProcessedPerTask);

int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps);
Expand Down Expand Up @@ -142,5 +144,18 @@ public static void IterateRowsWithTempBuffer<T>(

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static int DivideCeil(int dividend, int divisor) => 1 + ((dividend - 1) / divisor);

private static void ValidateRectangle(Rectangle rectangle)
{
Guard.IsTrue(
rectangle.Width > 0,
$"{nameof(rectangle)}.{nameof(rectangle.Width)}",
"Can't iterate rows on a with a rectangle that has a width less than or equal to 0");

Guard.IsTrue(
rectangle.Height > 0,
$"{nameof(rectangle)}.{nameof(rectangle.Height)}",
"Can't iterate rows on a with a rectangle that has a height less than or equal to 0");
}
}
}
56 changes: 56 additions & 0 deletions tests/ImageSharp.Tests/Drawing/DrawImageTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,62 @@ public void ImageShouldHandlePositiveLocation(TestImageProvider<Rgba32> provider
background.DebugSave(provider, testOutputDetails: "Positive");
}
}
[Theory]
[WithSolidFilledImages(100, 100, 255, 255, 255, PixelTypes.Rgba32)]
public void ImageShouldHandlePositiveLocationTruncatedOverlay(TestImageProvider<Rgba32> provider)
{
using (Image<Rgba32> background = provider.GetImage())
using (var overlay = new Image<Rgba32>(50, 50))
{
overlay.Mutate(x => x.Fill(Rgba32.Black));

const int xy = 75;
Rgba32 backgroundPixel = background[xy - 1, xy - 1];
Rgba32 overlayPixel = overlay[0, 0];

background.Mutate(x => x.DrawImage(overlay, new Point(xy, xy), PixelColorBlendingMode.Normal, 1F));

Assert.Equal(Rgba32.White, backgroundPixel);
Assert.Equal(overlayPixel, background[xy, xy]);

background.DebugSave(provider, testOutputDetails: "PositiveTruncated");
}
}

[Theory]
[WithSolidFilledImages(100, 100, 255, 255, 255, PixelTypes.Rgba32, -30, -30)]
[WithSolidFilledImages(100, 100, 255, 255, 255, PixelTypes.Rgba32, 130, -30)]
[WithSolidFilledImages(100, 100, 255, 255, 255, PixelTypes.Rgba32, 130, 130)]
[WithSolidFilledImages(100, 100, 255, 255, 255, PixelTypes.Rgba32, -30, 130)]
public void NonOverlappingImageThrows(TestImageProvider<Rgba32> provider, int x, int y)
{
using (Image<Rgba32> background = provider.GetImage())
using (var overlay = new Image<Rgba32>(Configuration.Default, 10, 10, Rgba32.Black))
{
Exception ex = Assert.ThrowsAny<Exception>(Test);
string message = null;
if (ex is ArgumentException)
{
message = ex.Message;
}
else if (ex is ImageProcessingException imageProcessingException)
{
Assert.IsType<ArgumentException>(imageProcessingException.InnerException.InnerException);
message = imageProcessingException.InnerException.InnerException.Message;
}
else
{
Assert.True(false, "Unexpected exception: " + ex?.GetType().FullName);
}

Assert.Contains("does not overlap", message);

void Test()
{
background.Mutate(context => context.DrawImage(overlay, new Point(x, y), GraphicsOptions.Default));
}
}
}

private static void VerifyImage<TPixel>(
TestImageProvider<TPixel> provider,
Expand Down
35 changes: 35 additions & 0 deletions tests/ImageSharp.Tests/Helpers/ParallelHelperTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

using SixLabors.ImageSharp.Memory;
using SixLabors.ImageSharp.ParallelUtils;
using SixLabors.ImageSharp.PixelFormats;
using SixLabors.Memory;
using SixLabors.Primitives;

Expand Down Expand Up @@ -334,5 +335,39 @@ void FillRow(int y, Buffer2D<Point> buffer)
TestImageExtensions.CompareBuffers(expected.Span, actual.Span);
}
}

[Theory]
[InlineData(0, 10)]
[InlineData(10, 0)]
[InlineData(-10, 10)]
[InlineData(10, -10)]
public void IterateRowsRequiresValidRectangle(int width, int height)
{
var parallelSettings = new ParallelExecutionSettings();

var rect = new Rectangle(0, 0, width, height);

ArgumentException ex = Assert.Throws<ArgumentException>(
() => ParallelHelper.IterateRows(rect, parallelSettings, (rows) => { }));

Assert.Contains(width <= 0 ? "width" : "height", ex.Message);
}

[Theory]
[InlineData(0, 10)]
[InlineData(10, 0)]
[InlineData(-10, 10)]
[InlineData(10, -10)]
public void IterateRowsWithTempBufferRequiresValidRectangle(int width, int height)
{
var parallelSettings = new ParallelExecutionSettings();

var rect = new Rectangle(0, 0, width, height);

ArgumentException ex = Assert.Throws<ArgumentException>(
() => ParallelHelper.IterateRowsWithTempBuffer<Rgba32>(rect, parallelSettings, (rows, memory) => { }));

Assert.Contains(width <= 0 ? "width" : "height", ex.Message);
}
}
}