Improve the Transform API #775

JimBobSquarePants · 2018-11-10T22:21:51Z

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

This PR dramatically simplifies all affine and projective transforms and fixes some issues we had with affine transforms.

Two new classes have been introduced into the public domain which allow the composition of matrices:

AffineTransformBuilder
ProjectiveTransformBuilder

All Transform<TPixel> methods now utilize these two classes which allows us to automatically resize canvases to cater for the transformed vectors.

For example an affine transform can be composed in the following manner:

var builder = new AffineTransformBuilder(image.Size())
    .AppendRotateMatrixDegrees(angleDeg)
    .AppendScaleMatrix(new SizeF(sx, sy))
    .AppendTranslationMatrix(new PointF(tx, ty));

image.Mutate(i => i.Transform(builder, KnownResamplers.Bicubic));

Both classes also have a Rectangle constructor overload that allows the transforming of areas of interest in the source image.

var rectangle = new Rectangle(48, 0, 48, 24);

var builder = new AffineTransformBuilder(rectangle)
    .AppendScaleMatrix(new SizeF(2, 1.5F));

image.Mutate(i => i.Transform(builder, KnownResamplers.Spline));

Additionally several layers of abstraction have now been removed. And performance of transforms improved by around 10%.

codecov · 2018-11-11T10:24:45Z

Codecov Report

Merging #775 into master will increase coverage by 0.04%.
The diff coverage is 94.6%.

@@            Coverage Diff             @@
##           master     #775      +/-   ##
==========================================
+ Coverage   88.57%   88.61%   +0.04%     
==========================================
  Files        1005     1008       +3     
  Lines       42859    42959     +100     
  Branches     3162     3162              
==========================================
+ Hits        37961    38067     +106     
+ Misses       4203     4191      -12     
- Partials      695      701       +6

Impacted Files	Coverage Δ
...ests/Processing/Transforms/AffineTransformTests.cs	`98.42% <100%> (-0.08%)`	⬇️
...rocessing/Processors/Transforms/RotateProcessor.cs	`100% <100%> (ø)`	⬆️
.../Processing/Processors/Transforms/SkewProcessor.cs	`84.61% <100%> (+9.61%)`	⬆️
.../Processors/Transforms/AffineTransformProcessor.cs	`100% <100%> (ø)`	⬆️
...sts/Processing/Transforms/TransformsHelpersTest.cs	`100% <100%> (ø)`	⬆️
src/ImageSharp/Processing/TransformExtensions.cs	`100% <100%> (+45.45%)`	⬆️
src/ImageSharp/Common/Helpers/ImageMaths.cs	`87.17% <100%> (ø)`	⬆️
.../Processing/Transforms/ProjectiveTransformTests.cs	`97.29% <100%> (+0.07%)`	⬆️
.../Processing/Transforms/TransformBuilderTestBase.cs	`100% <100%> (ø)`
...s/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs	`57.14% <100%> (+0.69%)`	⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7eb18f...e8f73b4. Read the comment docs.

…sts.cs

JimBobSquarePants · 2018-11-22T14:14:33Z

Bump 😉

antonfirsov · 2018-11-22T23:12:10Z

@JimBobSquarePants I will review this during the weekend.

antonfirsov

Good job! Can't spot anything fundamentally wrong here, but I think we need to address a few points before merging this:

API suggestions (simplification, completeness)
Something's wrong on .NET 4.6.2 (on my I7-7700HQ machine)
Moar and better tests for transform builders

src/ImageSharp/Processing/AffineTransformBuilder.cs

antonfirsov · 2018-11-25T16:00:49Z

src/ImageSharp/Processing/ProjectiveTransformBuilder.cs

+    /// <summary>
+    /// A helper class for constructing <see cref="Matrix4x4"/> instances for use in projective transforms.
+    /// </summary>
+    public class ProjectiveTransformBuilder


All my review comments for AffineTransfrormBuilder are also valid for ProjectiveTransformBuilder.

antonfirsov · 2018-11-25T16:05:06Z

src/ImageSharp/Processing/ProjectiveTransformBuilder.cs

+            this.matrices.Add(matrix);
+            return this;
+        }
+


Would be nice to also add the Rotate/Scale/Translate/Skew methods already present in AffineTransformBuilder.

Are they possible with Matrix4x4?

Yes, Affine is a subset of Projective. Not sure about the details with the Matrix4x4 class, but I guess you need to use the 3D API-s with Z=0 for translation and Z=1 for scale-like stuff.

Theoretically yeah I guess. Feel free to have a pop.

antonfirsov · 2018-11-25T16:48:39Z

tests/ImageSharp.Tests/Processing/Transforms/AffineTransformTests.cs

@@ -17,7 +17,8 @@ public class AffineTransformTests
    {


Not sure whether it's related to the changes of the PR, but on my PC tests are failing for non-core2.1 targets. Something is wrong with the alpha blending:

I don't think this error is specific to transforms. This image never runs through that code but is a span copy. I think it's got something to do with Rgba32.

antonfirsov · 2018-11-25T16:51:15Z

tests/ImageSharp.Tests/Processing/Transforms/ProjectiveTransformBuilderTests.cs

+
+            Assert.Equal(b1.BuildMatrix(), b2.BuildMatrix());
+        }
+


We need better coverage for both AffineTransformBuilder and ProjectiveTransformBuilder! Common features could be tested with the same test code. I'll see whether I can help here.

Both have 100% coverage as far as I’m aware.

The specific creation methods (rotate etc.) seem uncovered for me + we can have a bit more semantical testing.

An idea that's easy to implement: a point transformed by a matrix created with the builder using a series of basic transforms should get to the expected position.

src/ImageSharp/Processing/AffineTransformBuilder.cs

antonfirsov · 2018-11-25T17:23:34Z

src/ImageSharp/Processing/Processors/Transforms/AffineTransformProcessor.cs

            this.TransformMatrix = matrix;
-            this.TargetDimensions = targetDimensions;
+            this.TargetDimensions = TransformUtils.GetTransformedSize(sourceSize, matrix);


Not sure whether the logic implemented in GetTransformedSize(...) fits the needs of all users. Maybe we should expose a AffineTransformProcessor(matrix, sampler, targetSize) constructor instead, and deal with the rest in the extension methods.

It’s seems to cover everything I could throw at it, shrinking and growing when need be. There’s actually similar code for handling free transforms in the net framework.

antonfirsov · 2018-11-25T17:48:21Z

@JimBobSquarePants one other API suggestion (which is pretty much related to my previous suggestions):

Asking the user to feed the source image size to Transform Builders seems unnecessary to me. The size is available on IImageProcessingContext<T> when a transform is being applied. We can prepend it to the matrix created by the Transform Builders. This will simplify the API usage on consumer side, and allow more flexibility.

antonfirsov · 2018-11-25T18:33:07Z

@JimBobSquarePants OK, how about this:

I push some more tests + extend the projective API right now, you deal with the rest?

How about the method namings? Should I change them? Still feel the word Matrix is unnecessary in method names, and it will become somewhat outdated if we shift to the matrix factory design.

JimBobSquarePants · 2018-11-25T18:37:07Z

Thanks for the review. Yeah sounds good. Let’s go ahead with naming changes, it is awfully wordy just now.

src/ImageSharp/Common/Helpers/ImageMaths.cs

src/ImageSharp/Processing/ProjectiveTransformBuilder.cs

…xible

antonfirsov · 2018-11-26T00:24:37Z

@JimBobSquarePants I also implemented my size-decoupling idea within the Affine API-s, but this is all for today. Projective API-s still need to be adapted.

And we still may want to find a workaround for the .NET 4.6.2 issue, however it's debug-only. (I think it is Vector2 tricking us again.)

src/ImageSharp/Processing/Processors/Transforms/RotateProcessor.cs

…dians back to PrependRotationRadians

JimBobSquarePants · 2018-11-27T10:34:21Z

And we still may want to find a workaround for the .NET 4.6.2 issue, however it's debug-only. (I think it is Vector2 tricking us again.)

@antonfirsov When you have time could you check this again? When I updated Core etc I think I got an implicit upgrade to System.Numerics.Vectors 4.5.0 so I want to see if there's any changes. Otherwise we may have to do something like the following which is super crappy.

ImageSharp/src/ImageSharp/Processing/Processors/Transforms/ProjectiveTransformProcessor.cs

Line 145 in 12a5fdf

Vector4 point = new Vector4(v3.X, v3.Y, 0, 0) / z;

JimBobSquarePants · 2018-11-27T17:10:35Z

@antonfirsov Aside from the alpha issue in debug I think we're done here.

antonfirsov · 2018-11-27T19:49:49Z

@JimBobSquarePants checking it now. Is the issue reproducible on your PC? (.NET 4.6.2/4.7.2 test execution + Debug)

antonfirsov · 2018-11-27T19:54:16Z

@JimBobSquarePants for me it still fails.

We either should do the Vector2 -> Vector4 workaround, or leave a note somewhere in the contribution guide that Debug test execution is only supported on .NET Core 2.1.

antonfirsov · 2018-11-27T20:06:00Z

@JimBobSquarePants what is still missing:

Unit Tests for TransformUtils: Should test the created transformations for correctness using sample input points paired with expected transformed destination points. I would say this is critical, but maybe I can live without it, if we can't do it now.
Skew in ProjectiveTransformBuilder (Could be added in a follow up PR)
Unit tests for AppendSkew: Similar to rotation tests: we should test that the transformation being appended/prepended is the same transformation that is produced by TransformUtils. (Less critical, could be added in a follow up PR)

JimBobSquarePants · 2018-11-27T20:16:20Z

@antonfirsov I get 283 failed tests running in debug on net472. Even things as simple as Rgba32 equality. Considering we're using ushort.Equals there I don't think they are fixable.

Just tried to create a reduced sample for the Rgba32 comparison error. Seems to work fine, very frustrating.

JimBobSquarePants · 2018-11-27T20:17:14Z

Unit Tests for TransformUtils: Should test the created transformations for correctness using sample input points paired with expected transformed destination points. I would say this is critical, but maybe I can live without it, if we can't do it now.

You're a hard taskmaster you know that right? 😝

antonfirsov · 2018-11-27T20:48:36Z

Opend #780 to "deal" with the Debug mode issues, we probalby shouldn't bother working around this in product code any longer.

I don't want to be too hard at this PR, up to you to decide. I'm just trying to save our future selves from writing these tests when we touch the code again 😄

JimBobSquarePants · 2018-11-28T14:26:43Z

@antonfirsov I've added all the Skew overloads to both builders and relevant additional tests for them also. Any additional tests we can add when cleaning up.

Improve the Transform API

JimBobSquarePants added 7 commits November 5, 2018 10:55

Add base becnhmark

e79f26b

Merge branch 'master' into js/better-transforms

b3951a7

Better Rotate

dfc926e

Cleanup TransformKernel and handle negative transform dimensions

2afd0d5

Introduce AffineTransformBuilder

8205216

Introduce ProjectiveTransformBuilder

5589bd3

Update tests/Images/External

8b0e276

JimBobSquarePants requested review from antonfirsov, tocsoft and dlemstra November 10, 2018 22:21

JimBobSquarePants added 2 commits November 10, 2018 22:22

Merge branch 'master' into js/better-transforms

8aea059

Change tolerance for 32bit framework builds.

15093ba

JimBobSquarePants added 6 commits November 11, 2018 21:33

Improve transform builder coverage

13b9e9c

Cleanup and add dedicated transform builder tests

8a47a18

Merge branch 'master' into js/better-transforms

bb3cc93

Tighten up coverage on new AotCompilerTools

6a81923

Update tests/ImageSharp.Tests/Processing/Transforms/AffineTransformTe…

1c258dc

…sts.cs

Merge branch 'master' into js/better-transforms

d6c24a3

JimBobSquarePants changed the title ~~Js/better transforms~~ Improve the Transform API Nov 22, 2018

Merge branch 'master' into js/better-transforms

43b2276

antonfirsov requested changes Nov 25, 2018

View reviewed changes

antonfirsov added 3 commits November 25, 2018 22:52

add more tests

da50180

rename methods + extend API

cc2589f

Translation, Scale -> ProjectiveTransformBuilder

0774d5e

JimBobSquarePants commented Nov 25, 2018

View reviewed changes

src/ImageSharp/Common/Helpers/ImageMaths.cs Outdated Show resolved Hide resolved

JimBobSquarePants commented Nov 25, 2018

View reviewed changes

src/ImageSharp/Processing/ProjectiveTransformBuilder.cs Outdated Show resolved Hide resolved

antonfirsov added 2 commits November 26, 2018 01:09

Make AffineTransformProcessor and AffineTransformBuilder API more fle…

103d129

…xible

fix rotate and skew processors

eab8a56

antonfirsov reviewed Nov 26, 2018

View reviewed changes

src/ImageSharp/Processing/Processors/Transforms/RotateProcessor.cs Show resolved Hide resolved

antonfirsov and others added 8 commits November 26, 2018 01:29

StyleCop

81e16e4

Rotation around custom center point. Rename PrependCenteredRotationRa…

bbfb2a0

…dians back to PrependRotationRadians

Use existing method. (There's precedence)

5d034d5

Remove "Matrix" suffix

cfa7cfd

Update dependencies

ffba817

Match APIs

7ea9912

Add large jpeg to 32bit skip

a1b74ea

Remove unused variables

964ebcc

Add Rotate(degrees, origin)

35ff16d

antonfirsov approved these changes Nov 27, 2018

View reviewed changes

Add Skew methods and tests.

8cdb728

Merge branch 'master' into js/better-transforms

e8f73b4

JimBobSquarePants merged commit f9c94e2 into master Nov 28, 2018

JimBobSquarePants deleted the js/better-transforms branch November 28, 2018 18:29

antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019

Merge pull request SixLabors#775 from SixLabors/js/better-transforms

4fe951c

Improve the Transform API

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the Transform API #775

Improve the Transform API #775

JimBobSquarePants commented Nov 10, 2018

codecov bot commented Nov 11, 2018 •

edited

Loading

JimBobSquarePants commented Nov 22, 2018

antonfirsov commented Nov 22, 2018

antonfirsov left a comment

antonfirsov Nov 25, 2018

antonfirsov Nov 25, 2018

JimBobSquarePants Nov 25, 2018

antonfirsov Nov 25, 2018

JimBobSquarePants Nov 25, 2018

antonfirsov Nov 25, 2018

JimBobSquarePants Nov 25, 2018

JimBobSquarePants Nov 27, 2018

antonfirsov Nov 25, 2018

JimBobSquarePants Nov 25, 2018

antonfirsov Nov 25, 2018 •

edited

Loading

antonfirsov Nov 25, 2018

JimBobSquarePants Nov 25, 2018

antonfirsov commented Nov 25, 2018 •

edited

Loading

antonfirsov commented Nov 25, 2018 •

edited

Loading

JimBobSquarePants commented Nov 25, 2018

antonfirsov commented Nov 26, 2018 •

edited

Loading

JimBobSquarePants commented Nov 27, 2018

JimBobSquarePants commented Nov 27, 2018

antonfirsov commented Nov 27, 2018 •

edited

Loading

antonfirsov commented Nov 27, 2018 •

edited

Loading

antonfirsov commented Nov 27, 2018 •

edited

Loading

JimBobSquarePants commented Nov 27, 2018 •

edited

Loading

JimBobSquarePants commented Nov 27, 2018

antonfirsov commented Nov 27, 2018

JimBobSquarePants commented Nov 28, 2018

Improve the Transform API #775

Improve the Transform API #775

Conversation

JimBobSquarePants commented Nov 10, 2018

Prerequisites

Description

codecov bot commented Nov 11, 2018 • edited Loading

Codecov Report

JimBobSquarePants commented Nov 22, 2018

antonfirsov commented Nov 22, 2018

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Nov 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov commented Nov 25, 2018 • edited Loading

antonfirsov commented Nov 25, 2018 • edited Loading

JimBobSquarePants commented Nov 25, 2018

antonfirsov commented Nov 26, 2018 • edited Loading

JimBobSquarePants commented Nov 27, 2018

JimBobSquarePants commented Nov 27, 2018

antonfirsov commented Nov 27, 2018 • edited Loading

antonfirsov commented Nov 27, 2018 • edited Loading

antonfirsov commented Nov 27, 2018 • edited Loading

JimBobSquarePants commented Nov 27, 2018 • edited Loading

JimBobSquarePants commented Nov 27, 2018

antonfirsov commented Nov 27, 2018

JimBobSquarePants commented Nov 28, 2018

codecov bot commented Nov 11, 2018 •

edited

Loading

antonfirsov Nov 25, 2018 •

edited

Loading

antonfirsov commented Nov 25, 2018 •

edited

Loading

antonfirsov commented Nov 25, 2018 •

edited

Loading

antonfirsov commented Nov 26, 2018 •

edited

Loading

antonfirsov commented Nov 27, 2018 •

edited

Loading

antonfirsov commented Nov 27, 2018 •

edited

Loading

antonfirsov commented Nov 27, 2018 •

edited

Loading

JimBobSquarePants commented Nov 27, 2018 •

edited

Loading