Calling DrawImage with top image that overhangs bottom image in Y direction fails with exception
aceoft opened this issue · 20 comments
Prerequisites
- I have written a descriptive issue title
- I have verified that I am running the latest version of ImageSharp
- I have verified if the problem exist in both
DEBUGandRELEASEmode - I have searched open and closed issues to ensure it has not already been reported
ImageSharp version
3.1
Other ImageSharp packages and versions
SixLabors.ImageSharp.Drawing 2.0.1
Environment (Operating system, version and so on)
Win10 22H2
.NET Framework version
7
Description
After upgrading the ImageSharp nuget package today, several of my unit tests started failing. They are all related to overlaying images that hang over the bottom edge of the bottom image.
The exception being thrown is:
System.AggregateException: One or more errors occurred. (Specified argument was out of the range of valid values. (Parameter 'DangerousGetRowSpan(100). Y was out of range. Height=100')) ---> System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'DangerousGetRowSpan(100). Y was out of range. Height=100')
It only seems to happen when a Y value is larger than the height of the bottom image.
I'm just getting started with ImageSharp and have been impressed by the amount of thought and hard work that has been put into the library. Thank you, and hopefully I provided everything we need here.
Steps to Reproduce
Test method to reproduce the exception:
public void TestOverlayImageOffEdge()
{
using var bottom = new Image<Rgb24>(100, 100, Color.Black);
using var top = new Image<Rgb24>(100, 100, Color.White);
// OK (expected)
bottom.Mutate(i => i.DrawImage(top, new Point(0, 0), 1f));
// OK (expected, overhangs X direction by 1 pixel)
bottom.Mutate(i => i.DrawImage(top, new Point(1, 0), 1f));
// throws System.AggregateException
bottom.Mutate(i => i.DrawImage(top, new Point(0, 1), 1f));
}I had a quick look at the source code and wondered if providing a source rectangle would help -- it did. The below line appears to be a workaround.
// OK
bottom.Mutate(i => i.DrawImage(top, new Point(0, 1), new Rectangle(0, 0, 100, 99), 1f));Images
No response
It appears that more than one behavior was changed in this area. I am also having trouble with overlaying images with the origin point being outside the dimensions of the bottom image.
For example, an origin of -100, -100 appears to be getting reset to 0, 0 before overlaying now.
Yeah, lots of changes.
For example, an origin of -100, -100 appears to be getting reset to 0, 0 before overlaying now.
That makes sense. The rectangle passed represents the source bounds to be drawn so you would pass a positive XY point to offset an image negatively.
Thank you for your response! I'm sorry, I wasn't clear enough in my last statement. I meant passing negative values for the X or Y values in the location (renamed to backgroundLocation) argument.
In 3.0.2, the top image would originate from that location correctly (you end up losing part of the left/top of the top image as intended). In 3.1, it doesn't seem to allow a backgroundLocation with negative values. It silently resets to 0, 0 and draws from there.
No, it’s ok, I got you the first time. If you look at the linked issue you will see why the behaviour was changed. (That change was specifically called out as breaking btw in our blog post).
Essentially negative offsetting the background position cannot work (previous implementation was buggy). You should specify the region you want to draw as a positive offset of the overlay image.
Cool, thanks for the follow up. I see there was an intended breaking change regarding the extra parameter/parameter names, but was the other breaking change (no longer supporting negative positions) intended? I don't see it mentioned in the post, changelist, or code review.
It appears that it is indeed possible to do this safely using the region like you described. Should that have been implemented alongside the other changes so that the outward behavior wasn't changed, with the result of breaking consuming code? That to me feels like the breaking change that could have been avoided and may have been unintentional.
Now that it's the default behavior in 3.1, I can see how it would be undesirable to change it again. I would at least consider throwing an exception if the geometric coordinates are going to be completely rejected/manipulated before drawing. At least that way we'd know we aren't going to get the result we expected. In that case my unit tests would have failed. As it was, I only noticed because I happened to look closely at one of the thumbnails and noticed it wasn't drawn correctly anymore.
I'm afraid I've gotten off into the weeds a bit though and distracted from the original point of this filed issue though.
Again, mad respect to you and your team and all the contributors for creating this amazing library for us.
I checked the source code and I think I see the issue around these lines:
The foregroundRectangle.Width is adjusted by the background location X value if it was negative. This is resulting in cropping off the right side of the top image, not the left. Same for the Y direction just below that.
I created a test project to demonstrate the difference to make sure I wasn't going crazy, but here is an illustration:
Above: 3.0.2 behavior. The top/left of the top image is lost (expected due to negative coordinates).
Above: what I believe the code in 3.1 is intended to do. The output would remain the same. I believe adjusting the rectangle's X an Y (in addition to removing it from the Width and Height) could have this effect. I am not an expert at what's happening below there, though, so I could be wrong.
Above: what is actually happening. The top image is indeed painted in the same box, but it originates from the top left corner and cuts off the bottom/right of the image instead of the top/left.
Thanks again for your time!
Just want to chime in that I see the exact same behavior of appearing to not respect negative origins and resetting to 0 after having bumped to 3.1.0.
I was going to investigate and create an issue if I got a few hours for it but you beat me to it with this great explanation!
I reverted to 3.0.2 for now
@tehsoul Please do not revert. That's the worst possible thing you could do!!!
The behavior in 3.0.2 is incorrect and you are basing your implementations on incorrect behavior. I will try to explain why.
Take for example the overlay image shared in issue #2447
If we wanted to render the following result that contains what you both would consider a negative offset off the overlay
you should write the following code:
using var src = Image.Load<Rgba32>("numbergrid.png");
using var dst = new Image<Rgba32>(100, 100, new Rgba32(0, 255, 255));
dst.Mutate(c => {
c.DrawImage(src, new Point(64, 10), new Rectangle(32, 32, 32, 32), 1.0f);
});
dst.SaveAsPng("result.png");As you can see from the code says to render the source image at position [64,10] taking from the source image a rectangle at position [32,32] and size 32x32.
To consider this a negative offset in coordinate terms is incorrect. The rectangle parameter specifically represents the area of interest within the overlay bounds to draw.
Ignoring negative values is a common way to normalize out of bounds properties hence why Rectangle.Intersect exists in here, System.Drawing, and many other libraries.
@aceoft What you think is the "intended behavior" is absolutely not intended at all. We explicitly want to avoid negative values in the overlay rectangle and the background location because they simply do not make sense.
I will repeat this again. The behavior you saw in v3.0.2 was unintentional and incorrect.
Does that make things clear?
I'll have a look at the original issue but there will be no reversion to the buggy behavior present in previous versions.
@JimBobSquarePants I agree with the logic in your example.
However, here is a very simplified version of what I'm trying to achieve (screenrecording from manually doing the action I want to automate in Paint.NET):
--> basically my code starts with the top left point as input, which is where I'm drawing it on the destination layer:
using var src = Image.Load<Rgba32>("numbergrid.png");
using var dst = new Image<Rgba32>(512, 512, Color.HotPink);
dst.Mutate(x=>x.DrawImage(src, new Point(-300, -150), 1.0f));
dst.SaveAsPng("result.png");3.0.2 output:
3.1.0 output (for my scenario, totally not what is intended):
@JimBobSquarePants the difference is between passing a Rectangle and just a Point. Based on the images provided I would have to agree that behaviour is broken in the case of a Point rather than a Rectangle. For a Point I would expect a negative offset to work and show the bottom right of the overlay image, however when passing a Rectangle I do agree it should be the explicit region defined.
I.e. Passing a Rectangle implies a crop followed by a draw operation, but passing a Point implies a straight draw operation leaving only the part of the overlay that happens to intersect visible.
To add to this, the following related behavior is also weird and imo inconsistent in 3.1.0:
Moving the image positively on the X axis so it overlaps destination - WORKS
using var src = Image.Load<Rgba32>("numbergrid.png");
using var dst = new Image<Rgba32>(512, 512, Color.HotPink);
dst.Mutate(x => x.DrawImage(src, new Point(300, 0), 1.0f));
dst.SaveAsPng("result.png");
--> works as expected
Moving the image positively on the Y axis so it overlaps destination - FAILS WITH EXCEPTION
using var src = Image.Load<Rgba32>("numbergrid.png");
using var dst = new Image<Rgba32>(512, 512, Color.HotPink);
dst.Mutate(x => x.DrawImage(src, new Point(300, 0), 1.0f));
dst.SaveAsPng("result.png");Fails with error:
System.AggregateException
One or more errors occurred. (Specified argument was out of the range of valid values. (Parameter 'DangerousGetRowSpan(537). Y was out of range. Height=512')) (Specified argument was out of the range of valid values. (Parameter 'DangerousGetRowSpan(623). Y was out of range. Height=512')) (Specified argument was out of the range of valid values. (Parameter 'DangerousGetRowSpan(580). Y was out of range. Height=512')) (Specified argument was out of the range of valid values. (Parameter 'DangerousGetRowSpan(512). Y was out of range. Height=512'))
--> this worked in 3.0.2 as expected:
@tocsoft Passing a negative point implies virtual pixels outside of the background image. I don’t think that is wise and is inconsistent with the rest of the library. It’s the same situation for our ImageBrush in Drawing.
@tehsoul that’s literally the raised issue. We should be clipping the vertical bounds
I would argue this is more inconstant with the other Drawing APIs. if you do the same operation with a vector I'm pretty sure it'll work as expected, i.e. drawing a circle with a negative offset will draw the bottom right portion of the circle not the top left.
For anyone running into this in the future, here is a naive custom extension method as a quick workaround for drawing to any point
@JimBobSquarePants this is basically the behavior from 3.0.2 when drawing to a point (which could be negative and/or cause a part of the foreground to be cut off on the Y axis if the target canvas didn't have enough vertical real estate)
public static class DrawingExtensions
{
/// <summary>
/// Draws the given image together with the currently processing image by blending their pixels.
/// </summary>
/// <param name="source">The current image processing context.</param>
/// <param name="foreground">The image to draw on the currently processing image.</param>
/// <param name="backgroundLocation">The location on the currently processing image at which to draw. - can be negative or cause the result to flow over the source</param>
/// <param name="opacity">The opacity of the image to draw. Must be between 0 and 1.</param>
/// <returns>The <see cref="IImageProcessingContext"/>.</returns>
public static IImageProcessingContext DrawImageToAPointRegressionProof(
this IImageProcessingContext source,
Image foreground,
Point backgroundLocation,
float opacity)
{
// revert to 0 for negative X and Y
var newPoint = new Point(
Math.Max(backgroundLocation.X, 0),
Math.Max(backgroundLocation.Y, 0));
// adapt cropping rectangle accordingly
var boundX = backgroundLocation.X < 0 ? -backgroundLocation.X : 0;
var boundY = backgroundLocation.Y < 0 ? -backgroundLocation.Y : 0;
var boundWidth = foreground.Bounds.Width - boundX;
var boundHeight = foreground.Bounds.Height - boundY;
// adjust (reduce) for output canvas if needed
var canvasSize = source.GetCurrentSize();
var remainingWidthOnCanvas = canvasSize.Width - newPoint.X;
var remainingHeightOnCanvas = canvasSize.Height - newPoint.Y;
boundWidth = Math.Min(boundWidth, remainingWidthOnCanvas);
boundHeight = Math.Min(boundHeight, remainingHeightOnCanvas);
var newBounds = new Rectangle(
boundX,
boundY,
boundWidth,
boundHeight
);
GraphicsOptions options = source.GetGraphicsOptions();
return source.ApplyProcessor(new DrawImageProcessor(foreground, newPoint, newBounds, options.ColorBlendingMode, options.AlphaCompositionMode, opacity));
}
}Tests:
[Fact]
public void MoveToLeftAndUp()
{
using var src = Image.Load<Rgba32>("numbergrid.png");
using var dst = new Image<Rgba32>(src.Size.Width + 50, src.Size.Height + 50, Color.HotPink);
dst.Mutate(x => x.DrawImageToAPointRegressionProof(src, new Point(-300, -150), 1.0f));
dst.SaveAsPng("result1.png");
}
[Fact]
public void MoveToRightAndDown()
{
using var src = Image.Load<Rgba32>("numbergrid.png");
using var dst = new Image<Rgba32>(src.Size.Width + 50, src.Size.Height + 50, Color.HotPink);
dst.Mutate(x => x.DrawImageToAPointRegressionProof(src, new Point(300, 150), 1.0f));
dst.SaveAsPng("result2.png");
}
[Fact]
public void MoveToRight()
{
using var src = Image.Load<Rgba32>("numbergrid.png");
using var dst = new Image<Rgba32>(src.Size.Width + 50, src.Size.Height + 50, Color.HotPink);
dst.Mutate(x => x.DrawImageToAPointRegressionProof(src, new Point(300, 0), 1.0f));
dst.SaveAsPng("result3.png");
}
[Fact]
public void MoveDown()
{
using var src = Image.Load<Rgba32>("numbergrid.png");
using var dst = new Image<Rgba32>(src.Size.Width + 50, src.Size.Height + 50, Color.HotPink);
dst.Mutate(x => x.DrawImageToAPointRegressionProof(src, new Point(0, 150), 1.0f));
dst.SaveAsPng("result4.png");
}
[Fact]
public void MoveToMiddle()
{
using var src = Image.Load<Rgba32>("numbergrid.png");
using var dst = new Image<Rgba32>(src.Size.Width + 50, src.Size.Height + 50, Color.HotPink);
dst.Mutate(x => x.DrawImageToAPointRegressionProof(src, new Point(20, 20), 1.0f));
dst.SaveAsPng("result5.png");
}
[Fact]
public void MoveToLeftAndUpOnASmallerCanvas()
{
using var src = Image.Load<Rgba32>("numbergrid.png");
using var dst = new Image<Rgba32>(src.Size.Width -50, src.Size.Height - 50, Color.HotPink);
dst.Mutate(x => x.DrawImageToAPointRegressionProof(src, new Point(-20, -20), 1.0f));
dst.SaveAsPng("result6.png");
}
I would argue this is more inconstant with the other Drawing APIs. if you do the same operation with a vector I'm pretty sure it'll work as expected, i.e. drawing a circle with a negative offset will draw the bottom right portion of the circle not the top left.
@tocsoft Drawing the elliptical path though doesn't provide an overload explicitly declaring areas of interest, whereas this method does. They're two separate operations and the parameters provided here are explicit.
The point of the changes to the parameter names and what I've tried to stress here is I want to remove ambiguity here. Allowing both parameters to determine an offset of the overlay rectangle makes the method confusing.
That's a LOT of code for this which can be used to determine the correct parameter value for the Rectangle overload for your scenario.
public static Rectangle GetRelativeIntersection(Rectangle rect1, Rectangle rect2)
{
// Find the intersection
Rectangle intersection = Rectangle.Intersect(rect1, rect2);
// If there is no intersection, return an empty rectangle
if (intersection.IsEmpty)
{
return Rectangle.Empty;
}
// Adjust the intersection rectangle to be relative to rect1's top-left corner
intersection.Offset(-rect1.Left, -rect1.Top);
return intersection;
}Negative offset example
Rectangle overlay = new(-50, -50, 100, 100);
Rectangle background = new(0, 0, 100, 100);
Rectangle interest = GetRelativeIntersection(overlay, background); // (50, 50, 50, 50);Positive offset example
Rectangle overlay = new(50, 50, 100, 100);
Rectangle background = new(0, 0, 100, 100);
Rectangle interest = GetRelativeIntersection(overlay, background); // (0, 0, 50, 50);I don't like that this issue has now been hijacked. There's a bug, I'll fix that bug unless someone wants to spend the time doing so instead of posting screeds of code here.
@JimBobSquarePants
For what it's worth - I'm just trying to be thorough and convey the issue I'm talking about as clearly as I can - as you initially seemed to be talking about a different scenario than me and @aceoft were with the negative point origins
Sorry, and thanks for the obviously much better code + picking up the bugs!
fix has been merged
It's a bummer that I didn't notice this issue earlier, as I might've been able to assist. Happy to see that it was resolved in such a swift manner, though!
Back when I wrote #2447 I was working on a solution to the issue myself and did manage to come up with one that I backported to 2.1.x as part of a WIP update to an older project of mine. Sadly due to a busy schedule I never got around to making a PR before @JimBobSquarePants had already prepared one. Based on my testing it seems I had unknowingly also solved this (future) issue.
My implementation was committed here for anyone interested, though the code is not as clean and neat as the official solution:
Visual-Vincent/DoomWriter@d521acc
@Visual-Vincent You've been super helpful already!!