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

Correctly handle identity transforms with varying dimensions #1945

Merged
merged 10 commits into from
Jan 25, 2022

Conversation

JimBobSquarePants
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Fixes #1911

@JimBobSquarePants JimBobSquarePants requested a review from a team January 18, 2022 07:13
@JimBobSquarePants JimBobSquarePants added this to the 2.0.0 milestone Jan 18, 2022
{
// The clone will be blank here copy all the pixel data over
source.GetPixelMemoryGroup().CopyTo(destination.GetPixelMemoryGroup());
var interest = Rectangle.Intersect(this.SourceRectangle, destination.Bounds());
Copy link
Member

@antonfirsov antonfirsov Jan 21, 2022

Choose a reason for hiding this comment

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

It looks like the rest of AffineTransformProcessor<T> code doesn't respect SourceRectangle.

The following two tests have a tiny difference in input matrices, yet they produce very different results after this change in the PR:

[Theory]
[WithTestPatternImages(100, 100, PixelTypes.Rgba32)]
public void Identity<TPixel>(TestImageProvider<TPixel> provider)
    where TPixel : unmanaged, IPixel<TPixel>
{
    using Image<TPixel> src = provider.GetImage();

    Matrix3x2 m = Matrix3x2.Identity;
    Rectangle r = new Rectangle(25, 25, 50, 50);
    src.Mutate(x => x.Transform(r, m, new Size(100, 100), KnownResamplers.Bicubic));
    src.DebugSave(provider);
}

[Theory]
[WithTestPatternImages(100, 100, PixelTypes.Rgba32)]
public void AlmostIdentity<TPixel>(TestImageProvider<TPixel> provider)
    where TPixel : unmanaged, IPixel<TPixel>
{
    using Image<TPixel> src = provider.GetImage();

    Matrix3x2 m = Matrix3x2.CreateRotation(0.0001f, new Vector2(50, 50));
    Rectangle r = new Rectangle(25, 25, 50, 50);
    src.Mutate(x => x.Transform(r, m, new Size(100, 100), KnownResamplers.Bicubic));
    src.DebugSave(provider);
}

Identity:
image

AlmostIdentity:
image

On master they are equally broken not respecting the SourceRectangle. It seems like ProjectiveTransform suffers from the same issue.

I wonder how to proceed? I see two options:

  1. Open a tracking issue & fix the processor
  2. Drop sourceRectangle the Transform overloads so the API is in sync with actual functionality, and open a tracking issue to implement them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well caught! I actually thought they we're and I'd just made an oversight.

I'll actually have a look at the entire implementation and see if it can be a quick 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.

Fix was easy enough. I've updated two ref files to match correct results but wondering what the best approach for coverage is. I could add refs for the tests you provided and do one using Matrix4x4.CreateRotationX for perspective equivalents but would like to avoid more ref images if possible.

Copy link
Member

Choose a reason for hiding this comment

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

More ref images should not cause trouble as long as they are small (a few bigger ones can weight much more than many small ones). We can transform the 30x30 central part of a 60x60 image to keep the output as small as possible.
I would turn AlmostIdentity into a theory and test a few more rotation angles. (Eg pi/4 -pi/4) We should see it rotating smoothly and the epsilon rotation should be visually indistinguishable from the special-cased 0 rotation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@JimBobSquarePants
Copy link
Member Author

@antonfirsov I've seen this pop up 2X now. Should we be concerned or is it a simple memory pressure thing driven by the environment.
https://github.com/SixLabors/ImageSharp/runs/4913118322?check_suite_focus=true#step:13:319

@antonfirsov
Copy link
Member

It should be environmental, will attempt to fix, or at least reduce flakiness by moving it out of process so at least it doesn't race with parallel tests for the same virtual address space.

@JimBobSquarePants
Copy link
Member Author

Ace. I'll rerun these anyway to get he green light

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

Looks good, well done!

@JimBobSquarePants JimBobSquarePants merged commit a9e718c into master Jan 25, 2022
@JimBobSquarePants JimBobSquarePants deleted the js/fix-1191 branch January 25, 2022 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ImageProcessingException when resizing an image of similar size to the target.
2 participants