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

Expose ExifOrientationUtilities and ICommandConverter implementations #241

Merged
merged 12 commits into from
Apr 4, 2022

Conversation

JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented Mar 25, 2022

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 is a rework of #224 which exposes utilities for transforming various types based upon the EXIF orientation value contained
within an image EXIF metadata. In addition I've made the ICommandConverter<T> implementations public since they allow users to more easily unit test their own IImageWebProcessor implementations.

There's no functional changes other than ExifOrientationUtilities.Transform(Vector2...) will normalize the input position based upon a given min/max range so is more flexible.

I've deliberately kept the parsing of values from commands separate for now as it feels very specific to the ResizeWebProcessor and the code can be easily copied if required (it's not complex).

cc @ronaldbarendse

@JimBobSquarePants JimBobSquarePants added this to the v2.0.0 milestone Mar 25, 2022
@JimBobSquarePants JimBobSquarePants requested review from a team and deanmarcussen March 25, 2022 04:45
@JimBobSquarePants
Copy link
Member Author

@deanmarcussen This is likely the last PR before I release V2.0.0 would it be possible for you to do a performance run (or show me docs so I can refresh my memory how to do it myself?)

@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #241 (444faa1) into main (f91c093) will decrease coverage by 0%.
The diff coverage is 89%.

@@         Coverage Diff         @@
##           main   #241   +/-   ##
===================================
- Coverage    85%    85%   -1%     
===================================
  Files        71     72    +1     
  Lines      1969   2020   +51     
  Branches    291    291           
===================================
+ Hits       1682   1724   +42     
- Misses      209    212    +3     
- Partials     78     84    +6     
Flag Coverage Δ
unittests 85% <89%> (-1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...Sharp.Web/Commands/Converters/ArrayConverter{T}.cs 100% <ø> (ø)
...mageSharp.Web/Commands/Converters/EnumConverter.cs 85% <ø> (ø)
.../Commands/Converters/IntegralNumberConverter{T}.cs 100% <ø> (ø)
...eSharp.Web/Commands/Converters/ListConverter{T}.cs 81% <ø> (ø)
src/ImageSharp.Web/FormatUtilities.cs 100% <ø> (ø)
src/ImageSharp.Web/PathUtilities.cs 100% <ø> (ø)
...eSharp.Web/Providers/PhysicalFileSystemProvider.cs 86% <50%> (ø)
src/ImageSharp.Web/FormattedImage.cs 82% <77%> (+1%) ⬆️
...harp.Web/Middleware/ImageSharpMiddlewareOptions.cs 89% <88%> (-1%) ⬇️
src/ImageSharp.Web/ExifOrientationUtilities.cs 89% <89%> (ø)
... and 4 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 f91c093...444faa1. Read the comment docs.

@ronaldbarendse
Copy link
Contributor

Thanks for making this utility class public, as Umbraco will also need to use the same transforms to convert crop coordinates to a crop rectangle. I did notice some strange outputs though, which I can reproduce (on main branch) by changing to rxy=1,1 in the sample project:

Landscape images

Portrait images

These images should all resize/crop to the bottom right corner, but that's currently not the case...

@JimBobSquarePants
Copy link
Member Author

I did notice some strange outputs though, which I can reproduce (on main branch) by changing to rxy=1,1 in the sample project:

Awww maaaan! I thought I'd cracked this! That's frustrating.

@JimBobSquarePants
Copy link
Member Author

JimBobSquarePants commented Mar 25, 2022

@ronaldbarendse That's it fixed now. Silly mistake

@JimBobSquarePants JimBobSquarePants changed the title Expose ExifOrientationUtilities Expose ExifOrientationUtilities and ICommandConverter implementations Mar 26, 2022
@ronaldbarendse
Copy link
Contributor

Awesome! I've tested the new logic with the CropWebProcessor and can confirm this is now working correctly using the following code:

// Values from crop command (0-1)
float left, top, right, bottom;

// Transform left/top and right/bottom points
ushort orientation = GetExifOrientation(image, commands, parser, culture);
Vector2 xy1 = ExifOrientationUtilities.Transform(new Vector2(left, top), Vector2.Zero, Vector2.One, orientation);
Vector2 xy2 = ExifOrientationUtilities.Transform(new Vector2(right, bottom), Vector2.Zero, Vector2.One, orientation);

// Get correct points (as they can get swapped) and convert to pixel based rectangle
Size size = image.Image.Size();
var cropRectangle = Rectangle.FromLTRB(
    (int)MathF.Round(MathF.Min(xy1.X, xy2.X) * size.Width),
    (int)MathF.Round(MathF.Min(xy1.Y, xy2.Y) * size.Height),
    (int)MathF.Round(MathF.Max(xy1.X, xy2.X) * size.Width),
    (int)MathF.Round(MathF.Max(xy1.Y, xy2.Y) * size.Height));

Maybe transforming and scaling the values could also be done in a single matrix/vector operation, but getting to the above code was already a goal achieved 😉

Also good to see you're exposing the converters to help unit testing, can you also do the same for the FormattedImage constructor? I see that's also used in the ImageSharp.Web tests and Umbraco currently resorts to reflection to do the same for testing the CropWebProcessor: https://github.com/umbraco/Umbraco-CMS/blob/ad66d61b0403cb7c8bf760467eb841a160206c8a/tests/Umbraco.Tests.UnitTests/Umbraco.Web.Common/ImageProcessors/CropWebProcessorTests.cs#L47-L78

@JimBobSquarePants
Copy link
Member Author

JimBobSquarePants commented Mar 26, 2022

Maybe transforming and scaling the values could also be done in a single matrix/vector operation, but getting to the above code was already a goal achieved 😉

Ah but you're already scaling the crop rectangle down. If you'd passed the image bounds as min, max and the topleft/rightbottom as the actual pixel values you get automatic scaling internally so your results match.

It looks like you could avoid the round/min operation by using ExifOrientationUtilities.Transform(Size size, ushort orientation). This will always give you the correct size. You can also explicitly cast a RectangleF to a Rectangle. or use Rectangle.Truncate(RectangleF rectangle) if you think that looks neater.

I've just made that constructor public for you. 😄

@ronaldbarendse
Copy link
Contributor

Ah but you're already scaling the crop rectangle down. If you'd passed the image bounds as min, max and the topleft/rightbottom as the actual pixel values you get automatic scaling internally so your results match.

But then the actual pixel values would be normalized to a 0-1 value again when doing the matrix transform and I'd have to multiply the top-left/right-bottom and set the max value to the right width/height... The CropWebProcessor already accepts percentage based coordinates (actually relative distances from all 4 sides), so it's already limited to 0-1, removing the need to know the actual image size.

It looks like you could avoid the round/min operation by using ExifOrientationUtilities.Transform(Size size, ushort orientation). This will always give you the correct size. You can also explicitly cast a RectangleF to a Rectangle. or use Rectangle.Truncate(RectangleF rectangle) if you think that looks neater.

I've tried that, but the transform might result in the top-left point not to be the actual top-left point of the rectangle, but instead the top-right, bottom-left or bottom-right one. Using the min/max ensures you get the correct points of the rectangle and rounding instead of truncating seems to give more consistent results.

@ronaldbarendse
Copy link
Contributor

ronaldbarendse commented Mar 27, 2022

Another thing to note: whenever the transform short-circuits on the orientation, the min/max normalization is skipped. Shouldn't this always be applied, so the position is always between those values?

@JimBobSquarePants
Copy link
Member Author

Another thing to note: whenever the transform short-circuits on the orientation, the min/max normalisation is skipped. Shouldn't this always be applied, so the position is always between those values?

No. If it doesn't require transforming we shouldn't touch the input value.

I'm gonna have a quick look at he transform function. Think I missed a trick to solve your incorrect output.

@JimBobSquarePants
Copy link
Member Author

Just fixed the Vector2 transform so that it uses the correct dimensions for descaling and added a bunch of tests so you should now be able to do the following.

// Values from crop command (0-1)
float left, top, right, bottom;

// Transform left/top and right/bottom points
ushort orientation = GetExifOrientation(image, commands, parser, culture);
Vector2 xy1 = ExifOrientationUtilities.Transform(new Vector2(left, top), Vector2.Zero, Vector2.One, orientation);
Vector2 xy2 = ExifOrientationUtilities.Transform(new Vector2(right, bottom), Vector2.Zero, Vector2.One, orientation);

// Scale points to a pixel based rectangle.
Size size = ExifOrientationUtilities.Transform(image.Image.Size());
Rectangle.Round(
    RectangleF.FromLTRB(
        xy1.X * size.Width,
        xy1.Y * size.Height,
        xy2.X * size.Width,
        xy2.Y * size.Height));

@ronaldbarendse
Copy link
Contributor

Another thing to note: whenever the transform short-circuits on the orientation, the min/max normalization is skipped. Shouldn't this always be applied, so the position is always between those values?

After reading the tests, I now get why you need a min/max value: not to clamp the input value, but to correctly rotate/flip on a specific plane. It might be helpful to add the following test cases to TransformVectorData, as that directly makes this clear:

{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.Unknown, new Vector2(-25F, -25F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.TopLeft, new Vector2(-25F, -25F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.TopRight, new Vector2(175F, -25F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.BottomRight, new Vector2(175F, 125F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.BottomLeft, new Vector2(-25F, 125F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.LeftTop, new Vector2(-25F, -25F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.RightTop, new Vector2(-25F, 175F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.RightBottom, new Vector2(125F, 175F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.LeftBottom, new Vector2(125F, -25F) }

so you should now be able to do the following

Unfortunately not, as the xy1 and xy2 values still might get transformed to points that are not the top-left and bottom-right points anymore and thus RectangleF.FromLTRB() not creating the correct rectangle. Looking at the reference source of System.Windows.Rect(Point point1, Point point2), this does something similar as well. You might want to add a constructor that accepts 2 points to the Rectangle and RectangleF structs in ImageSharp to make this easier in the future, but I guess this won't be a very common use-case to support...

src/ImageSharp.Web/Processors/ResizeWebProcessor.cs Outdated Show resolved Hide resolved
Comment on lines 95 to 98
if (orientation is >= ExifOrientationMode.Unknown and <= ExifOrientationMode.TopLeft)
{
return anchor;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this short-circuit really have a performance improvement compared to the switch statements below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah though I should swap the arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, swapping the switch statements to orientation and then anchor would make more sense👍🏻

@JimBobSquarePants
Copy link
Member Author

Those test cases make no sense. The provided vectors are not within the min/max range. Min and max allow proportional scaling of vector dimension lengths. 0-150 becomes 0-1.

@ronaldbarendse
Copy link
Contributor

Those test cases make no sense. The provided vectors are not within the min/max range. Min and max allow proportional scaling of vector dimension lengths. 0-150 becomes 0-1.

I agree that the test cases might not make much sense in the usage context (transforming a point within a plane), but it does show/validate that the min/max values aren't clamping the value, but instead the values 'wrap around' them. I still think that's a valid thing to test...

I'm very happy to see these changes are going to be a part of the v2 release: it's simply awesome to have ImageSharp.Web support EXIF-orientation aware image resizing and provide some utility classes to do the same in your own custom processors! 🎉

Comment on lines 95 to 98
if (orientation is >= ExifOrientationMode.Unknown and <= ExifOrientationMode.TopLeft)
{
return anchor;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, swapping the switch statements to orientation and then anchor would make more sense👍🏻

{ new Vector2(25F, 25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.LeftTop, new Vector2(25F, 25F) },
{ new Vector2(25F, 25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.RightTop, new Vector2(25F, 125F) },
{ new Vector2(25F, 25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.RightBottom, new Vector2(75F, 125F) },
{ new Vector2(25F, 25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.LeftBottom, new Vector2(75F, 25F) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{ new Vector2(25F, 25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.LeftBottom, new Vector2(75F, 25F) }
{ new Vector2(25F, 25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.LeftBottom, new Vector2(75F, 25F) },
// Validate the transformed value wraps around the min/max (instead of clamping)
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.Unknown, new Vector2(-25F, -25F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.TopLeft, new Vector2(-25F, -25F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.TopRight, new Vector2(175F, -25F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.BottomRight, new Vector2(175F, 125F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.BottomLeft, new Vector2(-25F, 125F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.LeftTop, new Vector2(-25F, -25F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.RightTop, new Vector2(-25F, 175F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.RightBottom, new Vector2(125F, 175F) },
{ new Vector2(-25F, -25F), Vector2.Zero, new Vector2(150, 100), ExifOrientationMode.LeftBottom, new Vector2(125F, -25F) }

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not what this is testing nor should be. I'm testing two things.

  1. That the input values are scaled correctly from 25, 25 to 16.667, .25.
  2. That the input values are transformed to match a precalculated expected result.

See the asci diagram here. 25, 25 is marked X as the tested position

        //                  150
        // ┌─────┬─────┬─────┬─────┬─────┬─────┐
        // │     │     │     │     │     │     │
        // │     │    FFFFFFFFFFFFFFF    │     │
        // ├─────X────F┌─────┬─────┬─────┼─────┤
        // │     │    F│     │     │     │     │
        // │     │    FFFFFFFFFFFFFFF    │     │
        // ├─────┼────F┌─────┬─────┬─────┼─────┤ 100
        // │     │    F│     │     │     │     │
        // │     │    F│     │     │     │     │
        // ├─────┼────F├─────┼─────┼─────┼─────┤
        // │     │    F│     │     │     │     │
        // │     │     │     │     │     │     │
        // └─────┴─────┴─────┴─────┴─────┴─────┘

Should I be testing a position greater than the centre to ensure my previous issue is covered. Probably but -25,-25 are non-sensical input values given the scaling bounds of 0,0 and 150,100.

TL;DR
The vector position MUST be within the Min/Max vector range.

Copy link
Contributor

Choose a reason for hiding this comment

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

The vector position MUST be within the Min/Max vector range.

I guess my current issue is that this isn't validated/enforced and the result is unclear.

Wouldn't it make sense to throw an ArgumentOutOfRangeException if the position is outside of the min/max range:

if (position.X < min.X || position.Y < min.Y)
{
    throw new ArgumentOutOfRangeException(nameof(position), position, $"The position X and Y values should be greater than or equal to the min X and Y values ({min}).");
}

if (position.X > max.X || position.Y > max.Y)
{
    throw new ArgumentOutOfRangeException(nameof(position), position, $"The position X and Y values should be lower than or equal to the max X and Y values ({max}).");
}

Or if you don't want any exceptions in this code-path, enforce this by clamping the values by adding this to the top of the method and then use normalized instead:

var normalized = new Vector2(Math.Clamp(position.X, min.X, max.X), Math.Clamp(position.Y, min.Y, max.Y));

I'm fine with clamping the input value myself, but just wanted to point this out 😉


From my testing, the following 'out of bounds' center positions currently result in the same 2 crops:

/Portrait_0.jpg?width=60&height=50&rxy=0,0
/Portrait_0.jpg?width=60&height=50&rxy=0,-1

/Portrait_0.jpg?width=60&height=50&rxy=0,1
/Portrait_0.jpg?width=60&height=50&rxy=0,2

But even through the resulting image is the same, because the URL is different they are cached as 2 separate files. This might not be a big deal, as that is currently also the case when using a negative width or height (and if both are 0 or lower, it will not even resize, but still cache the image), but still something to be aware of. And this can be manually worked-around by removing any invalid commands in the OnParseCommandsAsync callback, similar to how the maximum 4000x4000 size is handled...

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about this again, clamping the input value should probably be done in the (web)processor itself, just like the rxy/CenterCoordinates currently is.

The issue I pointed out is still valid though: any known commands are used to generate the cache key/hash, even if the command value is invalid, causing the same resulting image to be cached multiple times using different keys/hashes.

To make this a little bit more complex: different web processors can use the same commands (which might be the case for width, height, orient and compand), so command value sanitation isn't something a single processor should be responsible for. You don't want to remove a command that's invalid for one processor, but valid for another (and thus does result in a different result image).

Again, this isn't a big deal and can otherwise be worked around manually by removing any invalid command values using OnParseCommandsAsync (or in a more advanced setup, using an HMAC to validate whether the commands/values are generated by your own application).

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue I pointed out is still valid though: any known commands are used to generate the cache key/hash, even if the command value is invalid, causing the same resulting image to be cached multiple times using different keys/hashes.

Yeah this is problematic. OnParseCommandsAsync is the best place to deal with it for the built in processors. A bounds check there would be sufficient.

To make this a little bit more complex: different web processors can use the same commands (which might be the case for width, height, orient and compand)

If someone is using width/height and have both ResizeWebProcessor and their own custom processor enabled they're in for a whole world of pain.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added basic sanitation.

444faa1

Comment on lines 94 to 180
{
if (orientation is >= ExifOrientationMode.Unknown and <= ExifOrientationMode.TopLeft)
{
return anchor;
}

/*
New anchor position is determined by calculating the direction of the anchor relative to the source image.
In the following example, the TopRight anchor becomes BottomRight

T L
+-----------------------+ +--------------+
| *| | |
| | | |
L | TL | R | |
| | | |
| | B | LB | T
| | | |
+-----------------------+ | |
B | |
| |
| *|
+--------------+
R
*/
return anchor switch
{
AnchorPositionMode.Center => anchor,
AnchorPositionMode.Top => orientation switch
{
ExifOrientationMode.BottomLeft or ExifOrientationMode.BottomRight => AnchorPositionMode.Bottom,
ExifOrientationMode.LeftTop or ExifOrientationMode.RightTop => AnchorPositionMode.Left,
ExifOrientationMode.LeftBottom or ExifOrientationMode.RightBottom => AnchorPositionMode.Right,
_ => anchor,
},
AnchorPositionMode.Bottom => orientation switch
{
ExifOrientationMode.BottomLeft or ExifOrientationMode.BottomRight => AnchorPositionMode.Top,
ExifOrientationMode.LeftTop or ExifOrientationMode.RightTop => AnchorPositionMode.Right,
ExifOrientationMode.LeftBottom or ExifOrientationMode.RightBottom => AnchorPositionMode.Left,
_ => anchor,
},
AnchorPositionMode.Left => orientation switch
{
ExifOrientationMode.TopRight or ExifOrientationMode.BottomRight => AnchorPositionMode.Right,
ExifOrientationMode.LeftTop or ExifOrientationMode.LeftBottom => AnchorPositionMode.Top,
ExifOrientationMode.RightTop or ExifOrientationMode.RightBottom => AnchorPositionMode.Bottom,
_ => anchor,
},
AnchorPositionMode.Right => orientation switch
{
ExifOrientationMode.TopRight or ExifOrientationMode.BottomRight => AnchorPositionMode.Left,
ExifOrientationMode.LeftTop or ExifOrientationMode.LeftBottom => AnchorPositionMode.Bottom,
ExifOrientationMode.RightTop or ExifOrientationMode.RightBottom => AnchorPositionMode.Top,
_ => anchor,
},
AnchorPositionMode.TopLeft => orientation switch
{
ExifOrientationMode.TopRight or ExifOrientationMode.LeftBottom => AnchorPositionMode.TopRight,
ExifOrientationMode.BottomRight or ExifOrientationMode.RightTop => AnchorPositionMode.BottomLeft,
ExifOrientationMode.BottomLeft or ExifOrientationMode.RightBottom => AnchorPositionMode.BottomRight,
_ => anchor,
},
AnchorPositionMode.TopRight => orientation switch
{
ExifOrientationMode.TopRight or ExifOrientationMode.RightTop => AnchorPositionMode.TopLeft,
ExifOrientationMode.BottomLeft or ExifOrientationMode.LeftBottom => AnchorPositionMode.BottomRight,
ExifOrientationMode.BottomRight or ExifOrientationMode.LeftTop => AnchorPositionMode.BottomLeft,
_ => anchor,
},
AnchorPositionMode.BottomRight => orientation switch
{
ExifOrientationMode.TopRight or ExifOrientationMode.LeftBottom => AnchorPositionMode.BottomLeft,
ExifOrientationMode.BottomLeft or ExifOrientationMode.RightTop => AnchorPositionMode.TopRight,
ExifOrientationMode.BottomRight or ExifOrientationMode.RightBottom => AnchorPositionMode.TopLeft,
_ => anchor,
},
AnchorPositionMode.BottomLeft => orientation switch
{
ExifOrientationMode.TopRight or ExifOrientationMode.RightTop => AnchorPositionMode.BottomRight,
ExifOrientationMode.BottomLeft or ExifOrientationMode.LeftBottom => AnchorPositionMode.TopLeft,
ExifOrientationMode.BottomRight or ExifOrientationMode.LeftTop => AnchorPositionMode.TopRight,
_ => anchor,
},
_ => anchor,
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the inverted switch statement:

Suggested change
{
if (orientation is >= ExifOrientationMode.Unknown and <= ExifOrientationMode.TopLeft)
{
return anchor;
}
/*
New anchor position is determined by calculating the direction of the anchor relative to the source image.
In the following example, the TopRight anchor becomes BottomRight
T L
+-----------------------+ +--------------+
| *| | |
| | | |
L | TL | R | |
| | | |
| | B | LB | T
| | | |
+-----------------------+ | |
B | |
| |
| *|
+--------------+
R
*/
return anchor switch
{
AnchorPositionMode.Center => anchor,
AnchorPositionMode.Top => orientation switch
{
ExifOrientationMode.BottomLeft or ExifOrientationMode.BottomRight => AnchorPositionMode.Bottom,
ExifOrientationMode.LeftTop or ExifOrientationMode.RightTop => AnchorPositionMode.Left,
ExifOrientationMode.LeftBottom or ExifOrientationMode.RightBottom => AnchorPositionMode.Right,
_ => anchor,
},
AnchorPositionMode.Bottom => orientation switch
{
ExifOrientationMode.BottomLeft or ExifOrientationMode.BottomRight => AnchorPositionMode.Top,
ExifOrientationMode.LeftTop or ExifOrientationMode.RightTop => AnchorPositionMode.Right,
ExifOrientationMode.LeftBottom or ExifOrientationMode.RightBottom => AnchorPositionMode.Left,
_ => anchor,
},
AnchorPositionMode.Left => orientation switch
{
ExifOrientationMode.TopRight or ExifOrientationMode.BottomRight => AnchorPositionMode.Right,
ExifOrientationMode.LeftTop or ExifOrientationMode.LeftBottom => AnchorPositionMode.Top,
ExifOrientationMode.RightTop or ExifOrientationMode.RightBottom => AnchorPositionMode.Bottom,
_ => anchor,
},
AnchorPositionMode.Right => orientation switch
{
ExifOrientationMode.TopRight or ExifOrientationMode.BottomRight => AnchorPositionMode.Left,
ExifOrientationMode.LeftTop or ExifOrientationMode.LeftBottom => AnchorPositionMode.Bottom,
ExifOrientationMode.RightTop or ExifOrientationMode.RightBottom => AnchorPositionMode.Top,
_ => anchor,
},
AnchorPositionMode.TopLeft => orientation switch
{
ExifOrientationMode.TopRight or ExifOrientationMode.LeftBottom => AnchorPositionMode.TopRight,
ExifOrientationMode.BottomRight or ExifOrientationMode.RightTop => AnchorPositionMode.BottomLeft,
ExifOrientationMode.BottomLeft or ExifOrientationMode.RightBottom => AnchorPositionMode.BottomRight,
_ => anchor,
},
AnchorPositionMode.TopRight => orientation switch
{
ExifOrientationMode.TopRight or ExifOrientationMode.RightTop => AnchorPositionMode.TopLeft,
ExifOrientationMode.BottomLeft or ExifOrientationMode.LeftBottom => AnchorPositionMode.BottomRight,
ExifOrientationMode.BottomRight or ExifOrientationMode.LeftTop => AnchorPositionMode.BottomLeft,
_ => anchor,
},
AnchorPositionMode.BottomRight => orientation switch
{
ExifOrientationMode.TopRight or ExifOrientationMode.LeftBottom => AnchorPositionMode.BottomLeft,
ExifOrientationMode.BottomLeft or ExifOrientationMode.RightTop => AnchorPositionMode.TopRight,
ExifOrientationMode.BottomRight or ExifOrientationMode.RightBottom => AnchorPositionMode.TopLeft,
_ => anchor,
},
AnchorPositionMode.BottomLeft => orientation switch
{
ExifOrientationMode.TopRight or ExifOrientationMode.RightTop => AnchorPositionMode.BottomRight,
ExifOrientationMode.BottomLeft or ExifOrientationMode.LeftBottom => AnchorPositionMode.TopLeft,
ExifOrientationMode.BottomRight or ExifOrientationMode.LeftTop => AnchorPositionMode.TopRight,
_ => anchor,
},
_ => anchor,
};
}
/*
New anchor position is determined by calculating the direction of the anchor relative to the source image.
In the following example, the TopRight anchor becomes BottomRight
T L
+-----------------------+ +--------------+
| *| | |
| | | |
L | TL | R | |
| | | |
| | B | LB | T
| | | |
+-----------------------+ | |
B | |
| |
| *|
+--------------+
R
*/
=> orientation switch
{
ExifOrientationMode.TopRight => anchor switch
{
AnchorPositionMode.Center => AnchorPositionMode.Center,
AnchorPositionMode.Top => AnchorPositionMode.Top,
AnchorPositionMode.Bottom => AnchorPositionMode.Bottom,
AnchorPositionMode.Left => AnchorPositionMode.Right,
AnchorPositionMode.Right => AnchorPositionMode.Left,
AnchorPositionMode.TopLeft => AnchorPositionMode.TopRight,
AnchorPositionMode.TopRight => AnchorPositionMode.TopLeft,
AnchorPositionMode.BottomRight => AnchorPositionMode.BottomLeft,
AnchorPositionMode.BottomLeft => AnchorPositionMode.BottomRight,
_ => anchor
},
ExifOrientationMode.BottomRight => anchor switch
{
AnchorPositionMode.Center => AnchorPositionMode.Center,
AnchorPositionMode.Top => AnchorPositionMode.Bottom,
AnchorPositionMode.Bottom => AnchorPositionMode.Top,
AnchorPositionMode.Left => AnchorPositionMode.Right,
AnchorPositionMode.Right => AnchorPositionMode.Left,
AnchorPositionMode.TopLeft => AnchorPositionMode.BottomRight,
AnchorPositionMode.TopRight => AnchorPositionMode.BottomLeft,
AnchorPositionMode.BottomRight => AnchorPositionMode.TopLeft,
AnchorPositionMode.BottomLeft => AnchorPositionMode.TopRight,
_ => anchor
},
ExifOrientationMode.BottomLeft => anchor switch
{
AnchorPositionMode.Center => AnchorPositionMode.Center,
AnchorPositionMode.Top => AnchorPositionMode.Bottom,
AnchorPositionMode.Bottom => AnchorPositionMode.Top,
AnchorPositionMode.Left => AnchorPositionMode.Left,
AnchorPositionMode.Right => AnchorPositionMode.Right,
AnchorPositionMode.TopLeft => AnchorPositionMode.BottomLeft,
AnchorPositionMode.TopRight => AnchorPositionMode.BottomRight,
AnchorPositionMode.BottomRight => AnchorPositionMode.TopRight,
AnchorPositionMode.BottomLeft => AnchorPositionMode.TopLeft,
_ => anchor
},
ExifOrientationMode.LeftTop => anchor switch
{
AnchorPositionMode.Center => AnchorPositionMode.Center,
AnchorPositionMode.Top => AnchorPositionMode.Left,
AnchorPositionMode.Bottom => AnchorPositionMode.Right,
AnchorPositionMode.Left => AnchorPositionMode.Top,
AnchorPositionMode.Right => AnchorPositionMode.Bottom,
AnchorPositionMode.TopLeft => AnchorPositionMode.TopLeft,
AnchorPositionMode.TopRight => AnchorPositionMode.BottomLeft,
AnchorPositionMode.BottomRight => AnchorPositionMode.BottomRight,
AnchorPositionMode.BottomLeft => AnchorPositionMode.TopRight,
_ => anchor
},
ExifOrientationMode.RightTop => anchor switch
{
AnchorPositionMode.Center => AnchorPositionMode.Center,
AnchorPositionMode.Top => AnchorPositionMode.Left,
AnchorPositionMode.Bottom => AnchorPositionMode.Right,
AnchorPositionMode.Left => AnchorPositionMode.Bottom,
AnchorPositionMode.Right => AnchorPositionMode.Top,
AnchorPositionMode.TopLeft => AnchorPositionMode.BottomLeft,
AnchorPositionMode.TopRight => AnchorPositionMode.TopLeft,
AnchorPositionMode.BottomRight => AnchorPositionMode.TopRight,
AnchorPositionMode.BottomLeft => AnchorPositionMode.BottomRight,
_ => anchor
},
ExifOrientationMode.RightBottom => anchor switch
{
AnchorPositionMode.Center => AnchorPositionMode.Center,
AnchorPositionMode.Top => AnchorPositionMode.Right,
AnchorPositionMode.Bottom => AnchorPositionMode.Left,
AnchorPositionMode.Left => AnchorPositionMode.Bottom,
AnchorPositionMode.Right => AnchorPositionMode.Top,
AnchorPositionMode.TopLeft => AnchorPositionMode.BottomRight,
AnchorPositionMode.TopRight => AnchorPositionMode.TopRight,
AnchorPositionMode.BottomRight => AnchorPositionMode.TopLeft,
AnchorPositionMode.BottomLeft => AnchorPositionMode.BottomLeft,
_ => anchor
},
ExifOrientationMode.LeftBottom => anchor switch
{
AnchorPositionMode.Center => AnchorPositionMode.Center,
AnchorPositionMode.Top => AnchorPositionMode.Right,
AnchorPositionMode.Bottom => AnchorPositionMode.Left,
AnchorPositionMode.Left => AnchorPositionMode.Top,
AnchorPositionMode.Right => AnchorPositionMode.Bottom,
AnchorPositionMode.TopLeft => AnchorPositionMode.TopRight,
AnchorPositionMode.TopRight => AnchorPositionMode.BottomRight,
AnchorPositionMode.BottomRight => AnchorPositionMode.BottomLeft,
AnchorPositionMode.BottomLeft => AnchorPositionMode.TopLeft,
_ => anchor
},
_ => anchor
};

Comment on lines 77 to 127
{ AnchorPositionMode.Center, ExifOrientationMode.TopLeft, AnchorPositionMode.Center },
{ AnchorPositionMode.Center, ExifOrientationMode.BottomLeft, AnchorPositionMode.Center },
{ AnchorPositionMode.Center, ExifOrientationMode.LeftBottom, AnchorPositionMode.Center },
{ AnchorPositionMode.Top, ExifOrientationMode.BottomLeft, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Top, ExifOrientationMode.BottomRight, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Top, ExifOrientationMode.LeftTop, AnchorPositionMode.Left },
{ AnchorPositionMode.Top, ExifOrientationMode.RightTop, AnchorPositionMode.Left },
{ AnchorPositionMode.Top, ExifOrientationMode.LeftBottom, AnchorPositionMode.Right },
{ AnchorPositionMode.Top, ExifOrientationMode.RightBottom, AnchorPositionMode.Right },
{ AnchorPositionMode.Bottom, ExifOrientationMode.BottomLeft, AnchorPositionMode.Top },
{ AnchorPositionMode.Bottom, ExifOrientationMode.BottomRight, AnchorPositionMode.Top },
{ AnchorPositionMode.Bottom, ExifOrientationMode.LeftTop, AnchorPositionMode.Right },
{ AnchorPositionMode.Bottom, ExifOrientationMode.RightTop, AnchorPositionMode.Right },
{ AnchorPositionMode.Bottom, ExifOrientationMode.LeftBottom, AnchorPositionMode.Left },
{ AnchorPositionMode.Bottom, ExifOrientationMode.RightBottom, AnchorPositionMode.Left },
{ AnchorPositionMode.Left, ExifOrientationMode.TopRight, AnchorPositionMode.Right },
{ AnchorPositionMode.Left, ExifOrientationMode.BottomRight, AnchorPositionMode.Right },
{ AnchorPositionMode.Left, ExifOrientationMode.LeftTop, AnchorPositionMode.Top },
{ AnchorPositionMode.Left, ExifOrientationMode.LeftBottom, AnchorPositionMode.Top },
{ AnchorPositionMode.Left, ExifOrientationMode.RightTop, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Left, ExifOrientationMode.RightBottom, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Right, ExifOrientationMode.TopRight, AnchorPositionMode.Left },
{ AnchorPositionMode.Right, ExifOrientationMode.BottomRight, AnchorPositionMode.Left },
{ AnchorPositionMode.Right, ExifOrientationMode.LeftTop, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Right, ExifOrientationMode.LeftBottom, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Right, ExifOrientationMode.RightTop, AnchorPositionMode.Top },
{ AnchorPositionMode.Right, ExifOrientationMode.RightBottom, AnchorPositionMode.Top },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.TopRight, AnchorPositionMode.TopRight },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.LeftBottom, AnchorPositionMode.TopRight },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.BottomRight, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.RightTop, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.BottomLeft, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.RightBottom, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.TopRight, ExifOrientationMode.TopRight, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.TopRight, ExifOrientationMode.RightTop, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.TopRight, ExifOrientationMode.BottomLeft, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.TopRight, ExifOrientationMode.LeftBottom, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.TopRight, ExifOrientationMode.BottomRight, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.TopRight, ExifOrientationMode.LeftTop, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.TopRight, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.LeftBottom, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.BottomLeft, AnchorPositionMode.TopRight },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.RightTop, AnchorPositionMode.TopRight },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.BottomRight, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.RightBottom, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.TopRight, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.RightTop, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.BottomLeft, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.LeftBottom, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.BottomRight, AnchorPositionMode.TopRight },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.LeftTop, AnchorPositionMode.TopRight },
Copy link
Contributor

Choose a reason for hiding this comment

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

And this the complete matrix of AnchorPositionMode and ExifOrientationMode to test:

Suggested change
{ AnchorPositionMode.Center, ExifOrientationMode.TopLeft, AnchorPositionMode.Center },
{ AnchorPositionMode.Center, ExifOrientationMode.BottomLeft, AnchorPositionMode.Center },
{ AnchorPositionMode.Center, ExifOrientationMode.LeftBottom, AnchorPositionMode.Center },
{ AnchorPositionMode.Top, ExifOrientationMode.BottomLeft, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Top, ExifOrientationMode.BottomRight, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Top, ExifOrientationMode.LeftTop, AnchorPositionMode.Left },
{ AnchorPositionMode.Top, ExifOrientationMode.RightTop, AnchorPositionMode.Left },
{ AnchorPositionMode.Top, ExifOrientationMode.LeftBottom, AnchorPositionMode.Right },
{ AnchorPositionMode.Top, ExifOrientationMode.RightBottom, AnchorPositionMode.Right },
{ AnchorPositionMode.Bottom, ExifOrientationMode.BottomLeft, AnchorPositionMode.Top },
{ AnchorPositionMode.Bottom, ExifOrientationMode.BottomRight, AnchorPositionMode.Top },
{ AnchorPositionMode.Bottom, ExifOrientationMode.LeftTop, AnchorPositionMode.Right },
{ AnchorPositionMode.Bottom, ExifOrientationMode.RightTop, AnchorPositionMode.Right },
{ AnchorPositionMode.Bottom, ExifOrientationMode.LeftBottom, AnchorPositionMode.Left },
{ AnchorPositionMode.Bottom, ExifOrientationMode.RightBottom, AnchorPositionMode.Left },
{ AnchorPositionMode.Left, ExifOrientationMode.TopRight, AnchorPositionMode.Right },
{ AnchorPositionMode.Left, ExifOrientationMode.BottomRight, AnchorPositionMode.Right },
{ AnchorPositionMode.Left, ExifOrientationMode.LeftTop, AnchorPositionMode.Top },
{ AnchorPositionMode.Left, ExifOrientationMode.LeftBottom, AnchorPositionMode.Top },
{ AnchorPositionMode.Left, ExifOrientationMode.RightTop, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Left, ExifOrientationMode.RightBottom, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Right, ExifOrientationMode.TopRight, AnchorPositionMode.Left },
{ AnchorPositionMode.Right, ExifOrientationMode.BottomRight, AnchorPositionMode.Left },
{ AnchorPositionMode.Right, ExifOrientationMode.LeftTop, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Right, ExifOrientationMode.LeftBottom, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Right, ExifOrientationMode.RightTop, AnchorPositionMode.Top },
{ AnchorPositionMode.Right, ExifOrientationMode.RightBottom, AnchorPositionMode.Top },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.TopRight, AnchorPositionMode.TopRight },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.LeftBottom, AnchorPositionMode.TopRight },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.BottomRight, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.RightTop, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.BottomLeft, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.RightBottom, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.TopRight, ExifOrientationMode.TopRight, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.TopRight, ExifOrientationMode.RightTop, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.TopRight, ExifOrientationMode.BottomLeft, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.TopRight, ExifOrientationMode.LeftBottom, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.TopRight, ExifOrientationMode.BottomRight, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.TopRight, ExifOrientationMode.LeftTop, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.TopRight, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.LeftBottom, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.BottomLeft, AnchorPositionMode.TopRight },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.RightTop, AnchorPositionMode.TopRight },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.BottomRight, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.RightBottom, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.TopRight, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.RightTop, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.BottomLeft, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.LeftBottom, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.BottomRight, AnchorPositionMode.TopRight },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.LeftTop, AnchorPositionMode.TopRight },
{ AnchorPositionMode.Center, ExifOrientationMode.Unknown, AnchorPositionMode.Center },
{ AnchorPositionMode.Center, ExifOrientationMode.TopLeft, AnchorPositionMode.Center },
{ AnchorPositionMode.Center, ExifOrientationMode.TopRight, AnchorPositionMode.Center },
{ AnchorPositionMode.Center, ExifOrientationMode.BottomRight, AnchorPositionMode.Center },
{ AnchorPositionMode.Center, ExifOrientationMode.BottomLeft, AnchorPositionMode.Center },
{ AnchorPositionMode.Center, ExifOrientationMode.LeftTop, AnchorPositionMode.Center },
{ AnchorPositionMode.Center, ExifOrientationMode.RightTop, AnchorPositionMode.Center },
{ AnchorPositionMode.Center, ExifOrientationMode.RightBottom, AnchorPositionMode.Center },
{ AnchorPositionMode.Center, ExifOrientationMode.LeftBottom, AnchorPositionMode.Center },
{ AnchorPositionMode.Top, ExifOrientationMode.Unknown, AnchorPositionMode.Top },
{ AnchorPositionMode.Top, ExifOrientationMode.TopLeft, AnchorPositionMode.Top },
{ AnchorPositionMode.Top, ExifOrientationMode.TopRight, AnchorPositionMode.Top },
{ AnchorPositionMode.Top, ExifOrientationMode.BottomRight, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Top, ExifOrientationMode.BottomLeft, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Top, ExifOrientationMode.LeftTop, AnchorPositionMode.Left },
{ AnchorPositionMode.Top, ExifOrientationMode.RightTop, AnchorPositionMode.Left },
{ AnchorPositionMode.Top, ExifOrientationMode.RightBottom, AnchorPositionMode.Right },
{ AnchorPositionMode.Top, ExifOrientationMode.LeftBottom, AnchorPositionMode.Right },
{ AnchorPositionMode.Bottom, ExifOrientationMode.Unknown, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Bottom, ExifOrientationMode.TopLeft, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Bottom, ExifOrientationMode.TopRight, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Bottom, ExifOrientationMode.BottomRight, AnchorPositionMode.Top },
{ AnchorPositionMode.Bottom, ExifOrientationMode.BottomLeft, AnchorPositionMode.Top },
{ AnchorPositionMode.Bottom, ExifOrientationMode.LeftTop, AnchorPositionMode.Right },
{ AnchorPositionMode.Bottom, ExifOrientationMode.RightTop, AnchorPositionMode.Right },
{ AnchorPositionMode.Bottom, ExifOrientationMode.RightBottom, AnchorPositionMode.Left },
{ AnchorPositionMode.Bottom, ExifOrientationMode.LeftBottom, AnchorPositionMode.Left },
{ AnchorPositionMode.Left, ExifOrientationMode.Unknown, AnchorPositionMode.Left },
{ AnchorPositionMode.Left, ExifOrientationMode.TopLeft, AnchorPositionMode.Left },
{ AnchorPositionMode.Left, ExifOrientationMode.TopRight, AnchorPositionMode.Right },
{ AnchorPositionMode.Left, ExifOrientationMode.BottomRight, AnchorPositionMode.Right },
{ AnchorPositionMode.Left, ExifOrientationMode.BottomLeft, AnchorPositionMode.Left },
{ AnchorPositionMode.Left, ExifOrientationMode.LeftTop, AnchorPositionMode.Top },
{ AnchorPositionMode.Left, ExifOrientationMode.RightTop, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Left, ExifOrientationMode.RightBottom, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Left, ExifOrientationMode.LeftBottom, AnchorPositionMode.Top },
{ AnchorPositionMode.Right, ExifOrientationMode.Unknown, AnchorPositionMode.Right },
{ AnchorPositionMode.Right, ExifOrientationMode.TopLeft, AnchorPositionMode.Right },
{ AnchorPositionMode.Right, ExifOrientationMode.TopRight, AnchorPositionMode.Left },
{ AnchorPositionMode.Right, ExifOrientationMode.BottomRight, AnchorPositionMode.Left },
{ AnchorPositionMode.Right, ExifOrientationMode.BottomLeft, AnchorPositionMode.Right },
{ AnchorPositionMode.Right, ExifOrientationMode.LeftTop, AnchorPositionMode.Bottom },
{ AnchorPositionMode.Right, ExifOrientationMode.RightTop, AnchorPositionMode.Top },
{ AnchorPositionMode.Right, ExifOrientationMode.RightBottom, AnchorPositionMode.Top },
{ AnchorPositionMode.Right, ExifOrientationMode.LeftBottom, AnchorPositionMode.Bottom },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.Unknown, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.TopLeft, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.TopRight, AnchorPositionMode.TopRight },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.BottomRight, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.BottomLeft, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.LeftTop, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.RightTop, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.RightBottom, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.TopLeft, ExifOrientationMode.LeftBottom, AnchorPositionMode.TopRight },
{ AnchorPositionMode.TopRight, ExifOrientationMode.Unknown, AnchorPositionMode.TopRight },
{ AnchorPositionMode.TopRight, ExifOrientationMode.TopLeft, AnchorPositionMode.TopRight },
{ AnchorPositionMode.TopRight, ExifOrientationMode.TopRight, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.TopRight, ExifOrientationMode.BottomRight, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.TopRight, ExifOrientationMode.BottomLeft, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.TopRight, ExifOrientationMode.LeftTop, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.TopRight, ExifOrientationMode.RightTop, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.TopRight, ExifOrientationMode.RightBottom, AnchorPositionMode.TopRight },
{ AnchorPositionMode.TopRight, ExifOrientationMode.LeftBottom, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.Unknown, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.TopLeft, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.TopRight, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.BottomRight, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.BottomLeft, AnchorPositionMode.TopRight },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.LeftTop, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.RightTop, AnchorPositionMode.TopRight },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.RightBottom, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.BottomRight, ExifOrientationMode.LeftBottom, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.Unknown, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.TopLeft, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.TopRight, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.BottomRight, AnchorPositionMode.TopRight },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.BottomLeft, AnchorPositionMode.TopLeft },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.LeftTop, AnchorPositionMode.TopRight },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.RightTop, AnchorPositionMode.BottomRight },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.RightBottom, AnchorPositionMode.BottomLeft },
{ AnchorPositionMode.BottomLeft, ExifOrientationMode.LeftBottom, AnchorPositionMode.TopLeft },

Copy link
Contributor

Choose a reason for hiding this comment

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

I applied this test matrix to the current transform and got 2 failing tests:

CanTransformAnchor(anchor: TopLeft, orientation: 3, expected: BottomRight)
Assert.Equal() Failure
Expected: BottomRight
Actual:   BottomLeft

CanTransformAnchor(anchor: TopLeft, orientation: 4, expected: BottomLeft)
Assert.Equal() Failure
Expected: BottomLeft
Actual:   BottomRight

I believe the test cases are correct, so that would mean the current implementation contains a bug:

  • Orientation 3 (bottom/right - upside down) should invert both horizontally and vertically, so top/left should become bottom/right
  • Orientation 4 (bottom/left - flipped back-to-front and upside down) should only invert vertically, so top/left should become bottom/left

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a good chance you found a bug - orientation is a bit of a brain twister! I will review your matrix and implementation asap. Really appreciate the effort you're making here btw! 👍

@@ -168,7 +168,7 @@ public void ResizeWebProcessor_RespectsOrientation_Center(ushort orientation)
image.Metadata.ExifProfile.SetValue(ExifTag.Orientation, orientation);
using var formatted = new FormattedImage(image, PngFormat.Instance);

PointF expected = GetExpectedCenter(orientation, new PointF(x, y));
PointF expected = ExifOrientationUtilities.Transform(new Vector2(x, y), Vector2.Zero, Vector2.One, orientation);
ResizeOptions options = ResizeWebProcessor.GetResizeOptions(formatted, commands, parser, culture);
Assert.Equal(expected, options.CenterCoordinates);
Copy link
Contributor

@ronaldbarendse ronaldbarendse Mar 30, 2022

Choose a reason for hiding this comment

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

This test failed on my machine, because my decimal separator is a comma and the interpolated string above uses the current culture: $"{x},{y}". This can be fixed by updating it to FormattableString.Invariant($"{x},{y}") 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, my bad.

Fix anchor position transforms and tests in ExifOrientationUtilities
@JimBobSquarePants JimBobSquarePants merged commit 249be0d into main Apr 4, 2022
@JimBobSquarePants JimBobSquarePants deleted the js/helpers branch April 4, 2022 09:40
@ronaldbarendse
Copy link
Contributor

Great to see this getting merged! 🎉

One note about the sanitization though: this really isn't specific to the width, height and rxy values, as any invalid command value will have this issue. Invalid enums (like ResizeMode and AnchorPositionMode), booleans or custom command values (like rsampler) all return a default when the supplied value is invalid, but still use the input to generate the cache key/hash.

This is basically a chicken-egg problem, as you only know which command values are valid when processing the image (or you have to duplicate all the command parsing), but that's cached by a key that's calculated using values that should prevent all that processing in the first place.

I don't think the solution is in adding custom sanitization to the default OnParseCommandsAsync or to introduce some kind of command value validators: a better solution would be to ensure users can't manually fiddle with the command values in the first place, like using the PresetOnlyQueryCollectionRequestParser or adding HMAC validation to the generated URL.

The current default sanitization also doesn't remove invalid width or height commands and only removes those commands if both are larger than 4000 (making ?width=4000&height=999999 still an accepted size):

// It's a good idea to have this to provide very basic security.
uint width = c.Parser.ParseValue<uint>(
c.Commands.GetValueOrDefault(ResizeWebProcessor.Width),
c.Culture);
uint height = c.Parser.ParseValue<uint>(
c.Commands.GetValueOrDefault(ResizeWebProcessor.Height),
c.Culture);
if (width > 4000 && height > 4000)
{
c.Commands.Remove(ResizeWebProcessor.Width);
c.Commands.Remove(ResizeWebProcessor.Height);
}
float[] coordinates = c.Parser.ParseValue<float[]>(c.Commands.GetValueOrDefault(ResizeWebProcessor.Xy), c.Culture);
if (coordinates.Length != 2
|| coordinates[1] < 0 || coordinates[1] > 1
|| coordinates[0] < 0 || coordinates[0] > 1)
{
c.Commands.Remove(ResizeWebProcessor.Xy);
}

@JimBobSquarePants
Copy link
Member Author

It’s not designed as a catch all. It’s very basic example sanitation to show where this stuff should happen. If someone wants something more comprehensive like HMAC this is where they’d do it.

Remember I provide tools, not implementations.

@ronaldbarendse
Copy link
Contributor

@JimBobSquarePants Looks like you forgot to make SimpleCommandConverter<T> public:

internal sealed class SimpleCommandConverter<T> : ICommandConverter<T>

This class is required to test the CropWebProcessor that ships with Umbraco.

@JimBobSquarePants
Copy link
Member Author

Will sneak it in with #250

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

Successfully merging this pull request may close these issues.

2 participants