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

Calling DrawImage with top image that overhangs bottom image in Y direction fails with exception #2603

Closed
4 tasks done
aceoft opened this issue Dec 5, 2023 · 20 comments · Fixed by #2610
Closed
4 tasks done

Comments

@aceoft
Copy link

aceoft commented Dec 5, 2023

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 DEBUG and RELEASE mode
  • 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

@aceoft
Copy link
Author

aceoft commented Dec 5, 2023

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.

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Dec 6, 2023

Yeah, lots of changes.

#2447

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.

@aceoft
Copy link
Author

aceoft commented Dec 6, 2023

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.

@JimBobSquarePants
Copy link
Member

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.

@aceoft
Copy link
Author

aceoft commented Dec 7, 2023

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.

@aceoft
Copy link
Author

aceoft commented Dec 7, 2023

I checked the source code and I think I see the issue around these lines:

foregroundRectangle.Width += this.BackgroundLocation.X;

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:

imagesharp-3 0 2
Above: 3.0.2 behavior. The top/left of the top image is lost (expected due to negative coordinates).

imagesharp-3 1-intended
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.

imagesharp-3 1-actual
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!

@tehsoul
Copy link

tehsoul commented Dec 7, 2023

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

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Dec 8, 2023

@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

image

If we wanted to render the following result that contains what you both would consider a negative offset off the overlay

image

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?

@JimBobSquarePants
Copy link
Member

I'll have a look at the original issue but there will be no reversion to the buggy behavior present in previous versions.

@tehsoul
Copy link

tehsoul commented Dec 8, 2023

@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):

imagesharpregression

--> 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:
result

3.1.0 output (for my scenario, totally not what is intended):
result

@tocsoft
Copy link
Member

tocsoft commented Dec 8, 2023

@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.

@tehsoul
Copy link

tehsoul commented Dec 8, 2023

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");

result

--> 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:

result

@JimBobSquarePants
Copy link
Member

@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

@tocsoft
Copy link
Member

tocsoft commented Dec 8, 2023

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.

@tehsoul
Copy link

tehsoul commented Dec 8, 2023

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");
}

result1

[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");
}

result2

[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");
}

result3

[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");
}

result4

[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");
}

result5

[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");
}

result6

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Dec 8, 2023

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.

@tehsoul

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.

@tehsoul
Copy link

tehsoul commented Dec 8, 2023

@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!

@tocsoft
Copy link
Member

tocsoft commented Dec 10, 2023

fix has been merged

@tocsoft tocsoft closed this as completed Dec 10, 2023
@Visual-Vincent
Copy link

Visual-Vincent commented Jan 9, 2024

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

@JimBobSquarePants
Copy link
Member

@Visual-Vincent You've been super helpful already!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants