From ff269db0bdb8c38941acef7f35a55fe3ae3dd92b Mon Sep 17 00:00:00 2001 From: Anthony Truskinger Date: Tue, 30 Apr 2019 21:55:44 +1000 Subject: [PATCH] Now throws a better excpetion DrawImage source does not overlap target (#877) * No longer throws when DrawImage source does not overlap target Previously, when DrawImage was used to overlay an image, in cases where the source image did not overlap the target image, a very confusing error was reported: "System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. Parameter name: MaxDegreeOfParallelism" Now, when this case happens, the DrawImage method will simply not affect the target image, which is the same way FillRegionProcessor handles such cases. ParallelHelper.IterRows also now does more validation of the input rectangle so that any further cases of this kind of problem throw a more relvant exception. Note I switched from DebugGuard to Guard because IterRows is a public API and thus should always validate its inputs. Fixes #875 * Refines DrawImage non-overlap error logic Adresses PR feedback in #877. Changes DrawImage shortcut to be less than or equal to. Also changes maxX calculation to use `Right` over `Width`. This is a semantic change that reflects intention better. No actual change because Top,Left for that rectanngle should always be 0,0. And adds more informative argument names to ParallelHelper.IterRows error message. * Non-overlapping DrawImage now throws Adressing PR feedback from #877 DrawImage now throws when the source image does not overlap, with a useful error message. Also improved the error messages for IterRows (and added validation to the other IterRows method) * DrawImage overlap test changed to support RELEASE The tests on the CI server are run in RELEASE which wrap the expected exception with an ImageProcessingException. * Adress feedback for DrawImage exception DrawImage throws an ImageProcessor exception which makes it easier to catch. And reverted IterRows to use Guard helpers --- .../Processors/Drawing/DrawImageProcessor.cs | 8 +++- .../Common/ParallelUtils/ParallelHelper.cs | 17 +++++++- .../ImageSharp.Tests/Drawing/DrawImageTest.cs | 42 +++++++++++++++++++ .../Helpers/ParallelHelperTests.cs | 35 ++++++++++++++++ 4 files changed, 100 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp.Drawing/Processing/Processors/Drawing/DrawImageProcessor.cs b/src/ImageSharp.Drawing/Processing/Processors/Drawing/DrawImageProcessor.cs index 0957904c62..6c03537eb3 100644 --- a/src/ImageSharp.Drawing/Processing/Processors/Drawing/DrawImageProcessor.cs +++ b/src/ImageSharp.Drawing/Processing/Processors/Drawing/DrawImageProcessor.cs @@ -71,7 +71,7 @@ namespace SixLabors.ImageSharp.Processing.Processors.Drawing 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); @@ -81,6 +81,12 @@ namespace SixLabors.ImageSharp.Processing.Processors.Drawing var workingRect = Rectangle.FromLTRB(minX, minY, maxX, maxY); + // not a valid operation because rectangle does not overlap with this image. + if (workingRect.Width <= 0 || workingRect.Height <= 0) + { + throw new ImageProcessingException("Cannot draw image because the source image does not overlap the target image."); + } + ParallelHelper.IterateRows( workingRect, configuration, diff --git a/src/ImageSharp/Common/ParallelUtils/ParallelHelper.cs b/src/ImageSharp/Common/ParallelUtils/ParallelHelper.cs index a930b8390f..2c2d045a57 100644 --- a/src/ImageSharp/Common/ParallelUtils/ParallelHelper.cs +++ b/src/ImageSharp/Common/ParallelUtils/ParallelHelper.cs @@ -45,7 +45,7 @@ namespace SixLabors.ImageSharp.ParallelUtils in ParallelExecutionSettings parallelSettings, Action body) { - DebugGuard.MustBeGreaterThan(rectangle.Width, 0, nameof(rectangle)); + ValidateRectangle(rectangle); int maxSteps = DivideCeil(rectangle.Width * rectangle.Height, parallelSettings.MinimumPixelsProcessedPerTask); @@ -87,6 +87,8 @@ namespace SixLabors.ImageSharp.ParallelUtils Action> body) where T : unmanaged { + ValidateRectangle(rectangle); + int maxSteps = DivideCeil(rectangle.Width * rectangle.Height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); @@ -142,5 +144,18 @@ namespace SixLabors.ImageSharp.ParallelUtils [MethodImpl(MethodImplOptions.AggressiveInlining)] private static int DivideCeil(int dividend, int divisor) => 1 + ((dividend - 1) / divisor); + + private static void ValidateRectangle(Rectangle rectangle) + { + Guard.MustBeGreaterThan( + rectangle.Width, + 0, + $"{nameof(rectangle)}.{nameof(rectangle.Width)}"); + + Guard.MustBeGreaterThan( + rectangle.Height, + 0, + $"{nameof(rectangle)}.{nameof(rectangle.Height)}"); + } } } \ No newline at end of file diff --git a/tests/ImageSharp.Tests/Drawing/DrawImageTest.cs b/tests/ImageSharp.Tests/Drawing/DrawImageTest.cs index 374454afba..b07f508834 100644 --- a/tests/ImageSharp.Tests/Drawing/DrawImageTest.cs +++ b/tests/ImageSharp.Tests/Drawing/DrawImageTest.cs @@ -131,6 +131,48 @@ namespace SixLabors.ImageSharp.Tests background.DebugSave(provider, testOutputDetails: "Positive"); } } + [Theory] + [WithSolidFilledImages(100, 100, 255, 255, 255, PixelTypes.Rgba32)] + public void ImageShouldHandlePositiveLocationTruncatedOverlay(TestImageProvider provider) + { + using (Image background = provider.GetImage()) + using (var overlay = new Image(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 provider, int x, int y) + { + using (Image background = provider.GetImage()) + using (var overlay = new Image(Configuration.Default, 10, 10, Rgba32.Black)) + { + ImageProcessingException ex = Assert.Throws(Test); + + Assert.Contains("does not overlap", ex.ToString()); + + void Test() + { + background.Mutate(context => context.DrawImage(overlay, new Point(x, y), GraphicsOptions.Default)); + } + } + } private static void VerifyImage( TestImageProvider provider, diff --git a/tests/ImageSharp.Tests/Helpers/ParallelHelperTests.cs b/tests/ImageSharp.Tests/Helpers/ParallelHelperTests.cs index ef6b133f75..ee309e0e30 100644 --- a/tests/ImageSharp.Tests/Helpers/ParallelHelperTests.cs +++ b/tests/ImageSharp.Tests/Helpers/ParallelHelperTests.cs @@ -9,6 +9,7 @@ using System.Threading; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.ParallelUtils; +using SixLabors.ImageSharp.PixelFormats; using SixLabors.Memory; using SixLabors.Primitives; @@ -334,5 +335,39 @@ namespace SixLabors.ImageSharp.Tests.Helpers 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); + + ArgumentOutOfRangeException ex = Assert.Throws( + () => 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); + + ArgumentOutOfRangeException ex = Assert.Throws( + () => ParallelHelper.IterateRowsWithTempBuffer(rect, parallelSettings, (rows, memory) => { })); + + Assert.Contains(width <= 0 ? "Width" : "Height", ex.Message); + } } } \ No newline at end of file