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

Respect EXIF orientation when resizing #217

Merged
merged 6 commits into from
Feb 6, 2022
Merged

Conversation

JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented Feb 4, 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

In addition to #215 we need the ability to resize images containing EXIF orientation metadata that specifies rotation based upon the users observed expectation without updating the EXIF metadata.

This PR updates the ResizeWebProcessor to swap dimensions parsed from command when a value indicating EXIF rotation is detected. This behavior can be turned off per request by passing the command orient=false (Note I've deliberately not used an r prefix to make this more reusable for other potential transforming processors).

cc\ @ronaldbarendse

@JimBobSquarePants JimBobSquarePants added this to the v2.0.0 milestone Feb 4, 2022
@JimBobSquarePants JimBobSquarePants requested a review from a team February 4, 2022 02:35
@codecov
Copy link

codecov bot commented Feb 4, 2022

Codecov Report

Merging #217 (65cb128) into master (316434c) will decrease coverage by 0%.
The diff coverage is 97%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #217   +/-   ##
=====================================
- Coverage      87%    87%   -1%     
=====================================
  Files          70     70           
  Lines        1811   1826   +15     
  Branches      266    269    +3     
=====================================
+ Hits         1577   1589   +12     
- Misses        165    167    +2     
- Partials       69     70    +1     
Flag Coverage Δ
unittests 87% <97%> (-1%) ⬇️

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

Impacted Files Coverage Δ
...rc/ImageSharp.Web/Processors/ResizeWebProcessor.cs 93% <97%> (+1%) ⬆️
...ching/LruCache/ConcurrentTLruCache{TKey,TValue}.cs 52% <0%> (-3%) ⬇️

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 4c6821e...65cb128. Read the comment docs.

@JimBobSquarePants
Copy link
Member Author

Question. Do we even need #215 anymore? I'm thinking we actually don't. It's probably best we don't provide a destructive operation - People can implement their own should they need to.

@ronaldbarendse
Copy link
Contributor

Note I've deliberately not used an r prefix to make this more reusable for other potential transforming processors

Totally agree and to make it more reusable, the logic should probably also be in a separate helper class. Maybe it should even be a part of ImageSharp itself, so you can easily get the auto-oriented size (and/or crop coordinates) when doing manual transforms?

I see you've already started on related changes in SixLabors/ImageSharp#1976, so maybe add an OrientationHelper that accepts sizes, coordinates, rectangles, etc. and transforms them based on the EXIF value? Would also be nice to add a new ExifSize() extension method to ImageInfoExtensions (or add an overload to Size(bool autoOrient = true) that auto-orients the dimensions)?

For ImageSharp.Web the logic could then be moved to OrientationWebHelper, which contains the command key (as public const) and related parsing logic that can be used by any web processor that want to be orientation aware (and use the same command to opt-out).

Question. Do we even need #215 anymore? I'm thinking we actually don't. It's probably best we don't provide a destructive operation - People can implement their own should they need to.

This processor still has a different responsibility: auto-orienting the image for browsers/viewers that don't support auto-rotating based on the EXIF value or when you need to disable this functionality (using image-orientation: none; in your CSS), but want the image to be displayed correctly.

@JimBobSquarePants
Copy link
Member Author

JimBobSquarePants commented Feb 6, 2022

@ronaldbarendse I don't think moving the current logic into some sort of public helper is worth the maintenance effort to try and design something genuinely useful. If you want to make your web processors do similar ImageSharp exposes all you need to allow that. (Note also that Compand uses the same approach.)

I've no plans to add such helpers to ImageSharp. This work is an abstraction which operates in the opposite way to the correct methods in ImageSharp.

@JimBobSquarePants JimBobSquarePants merged commit 376c0e6 into master Feb 6, 2022
@JimBobSquarePants JimBobSquarePants deleted the js/smart-resize branch February 6, 2022 07:36
@ronaldbarendse
Copy link
Contributor

Shouldn't the ranchor and rxy values also respect the EXIF orientation? Otherwise they will select the 'wrong' portion when cropping.

Might be easier to mutate the ResizeOptions once, so all parsing methods can be kept clean.

@JimBobSquarePants
Copy link
Member Author

JimBobSquarePants commented Feb 6, 2022

@ronaldbarendse Good catch, I'll open a PR today.

EDIT. AnchorPosition is hard!

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.

2 participants