Apply updates to XY and Anchor positions based on EXIF metadata #221
Conversation
src/ImageSharp.Web/FormattedImage.cs
Outdated
| /// </summary> | ||
| /// <param name="orientation">The decoded orientation. Use <see cref="ExifOrientationMode"/> for comparison.</param> | ||
| /// <returns>The <see cref="bool"/> indicating whether the image contains EXIF metadata indicating that the image is rotated.</returns> | ||
| public bool IsExifRotated(out ushort orientation) |
There was a problem hiding this comment.
I've made this public as it's useful for other processor implementations. The EXIF parsing logic is identical to AutoOrientProcessor<T>.GetExifOrientation()
There was a problem hiding this comment.
Why is this only checking for rotated images and not flipped ones? I think that made sense when only considering width/height, but because the ResizeWebProcessor also needs to handle the correct X/Y and anchor position, this should also return true when it's flipped.
I'd also rename it to TryGetExifOrientation(out ushort orientation) and return whether the image contains a valid EXIF orientation value or not. Then just check if it's false or the orientation is set to 1 (top/left) before doing any of the transforms.
There was a problem hiding this comment.
Why is this only checking for rotated images and not flipped ones?
Because rotated is an important metric as that affects Size. Flipped does not affect that property and I wanted a quick way to tell, writing the method to reflect that desire. Can easily refactor though to suit your design and move rotate the check inside the processor itself.
There was a problem hiding this comment.
There you go. Refactored. Much cleaner now.
| return anchor; | ||
| } | ||
|
|
||
| /* |
There was a problem hiding this comment.
I had to use a notepad for this bit. I think my logic is sound but could do with a second opinion.
Codecov Report
@@ Coverage Diff @@
## master #221 +/- ##
======================================
Coverage 86% 87%
======================================
Files 70 70
Lines 1828 1928 +100
Branches 268 283 +15
======================================
+ Hits 1588 1680 +92
- Misses 170 174 +4
- Partials 70 74 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I have discovered this PR additionally fixes a null exception for images lacking orientation metadata 👍 |
|
Just added a bunch of manual testing to confirm my approach. Seems to work very well. |
|
|
||
| /// <summary> | ||
| /// Used to temporarily store cache resolver reads to reduce the overhead of cache lookups. | ||
| /// </summary> | ||
| private static readonly ConcurrentTLruCache<string, (IImageCacheResolver, ImageCacheMetadata)> CacheResolverLru | ||
| = new(1024, TimeSpan.FromMinutes(5)); | ||
| = new(1024, TimeSpan.FromSeconds(30)); |
There was a problem hiding this comment.
are you going to pop these back up? or make them configurable?
There was a problem hiding this comment.
I guess we could make them configurable but I've no idea what to name the properties (InMemoryCacheDuration?)
I found that if I deleted the cache folder when something was stored in there it prompted a null reference exception in the cache resolver. I dropped the times to make that less likely.
There was a problem hiding this comment.
Ah... They're private static. We can't pass options.
There was a problem hiding this comment.
Ah... They're private static. We can't pass options.
True, there might be an argument for making them non static, for performance.
In our scenario, we have multiple instances of the middleware (per tenant), so multiple instances of the cache per tenant, is perhaps desirable, as the items count will be less, so the lookup a little faster.
(then becomes configurable)
Something we can think about later though :)
There was a problem hiding this comment.
Yeah that might well be an idea then.
There was a problem hiding this comment.
I've added my suggestions in PR #224, as the current implementation doesn't correctly parse/transform the center coordinates and isn't easily re-usable in custom processors.
| } | ||
|
|
||
| AffineTransformBuilder builder = new(); | ||
| Size size = image.Image.Size(); |
There was a problem hiding this comment.
The image size shouldn't be required when dealing with coordinates ranging from 0 to 1.
There was a problem hiding this comment.
I'd actually forgotten that those coordinates were 0-1. They're not documented well on the underlying type. Suggested fixes are much more useful here that in a separate PR.
I don't want the other changes. I don't want to have to create yet another public type, especially one that is mostly filled with methods specific to ResizeWebProcessor. The PointF Transform method for example requires the X/Y properties to be within a 0-1 range. Not useful for anything that currently exists. You're creating solutions to problems I don't and won't have. Processor implementation outside of this library are the responsibility of whoever develops them not me. I'm building a framework, not a utility belt.
There was a problem hiding this comment.
I've implemented a proper fix for XY transforms now anyway, took me longer than I'm proud of to figure out. If you look at the image above and compare it to the code the process actually becomes pretty clear.
There was a problem hiding this comment.
Most of those methods are specific to the ResizeOptions indeed, which can be re-used on different web processors that require resizing (e.g. when trying to crop to a specific target rectangle, which Umbraco does in the CropWebProcessor based on crop coordinates), so it's not really specific to ResizeWebProcessor.
Making the PointF transform use a different range would be as simple as moving the Size variable to a parameter, but I get the point you're making here 😉
I've quickly tested the latest changes and it all seems to work as expected 👍🏻
There was a problem hiding this comment.
Thanks for testing, that's very useful 👍
Prerequisites
Description
Following on from #217 This PR updates
ResizeWebProcessorto correctly handleXYandAnchorcommands when processing EXIF orientated images.There's a lot of work involved in handling this with most of the logic based upon some of the work upstream in ImageSharp when normalizing orientation values. The following image will help explain the affect on an image each rotation value has.
cc @ronaldbarendse