-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
feat: add experimental image orientation fix #724
base: main
Are you sure you want to change the base?
Conversation
|
I've discovered that the |
The options object will allow more configuration without blowing up the arity of the function. Backwards compatible, though I suggest deprecating the old style if you keep this change.
To stay backwards compatible and ensure that current users are not affected by upcoming changes, we need to test the current behavior of the `flip`, `flop`, and `rotate` transforms when applied to images with EXIF orientation metadata. The images added to __fixtures__ are all rotated via EXIF orientation. Whether or not they appear correctly in your image viewer is dependent on its ability to read that orientation metadata. The resulting PNG files show the problems that people run into when they don't account for orientation metadata in their image processing. The next commit will introduce a new setting that attempts to give developers the results they actually expect.
This experimental setting is intended to give developers an easy way to treat their images intuitively based on how they actually look on screen, instead of how they are actually written on disk. There is an overhead to certain orientations that might be too high a cost to pay, but that is why this is experimental. Working on this feature has made me believe that the best place to add the DX improvement is actually within Sharp itself. In the meantime there is something here worth trying out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: It is probably better to write tests for this experimental feature in a completely separate file, and group them all together, instead of spreading them across the existing flip
, flop
, and rotate
tests.
@@ -32,6 +32,7 @@ | |||
"dev": "rollup -cw", | |||
"build": "rollup -c", | |||
"test": "vitest run", | |||
"test:watch": "vitest watch", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't get into watch
mode without this, but I didn't mean to add it to the commit. Will drop.
@@ -32,6 +32,7 @@ | |||
"dev": "rollup -cw", | |||
"build": "rollup -c", | |||
"test": "vitest run", | |||
"test:watch": "vitest watch", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. I meant to drop this change.
experimental?: { | ||
/** | ||
* Sharp's handling of image orientation probably does not match the | ||
* expectations of users of imagetools. Enabling this option tries tp preserve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo s/tp/to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect the arrow to point left in this one. I am missing something on the vite
side of things at the moment.
Addresses #700 via an experimental setting.
🚨 I have not done everything on this list yet. I will do them, but I wanted to get some eyes on this draft first.
A new setting is added to both Vite config and as an option to pass into
applyTransforms
.This opt-in feature will pre-
rotate
/flip
/flop
an image based on the EXIF orientation data so that downstream developers can think about their images without first considering whether or not the image appearance is affected by metadata or by actual pixel locations. Two images that look the same might have completely different results when applyingflip
,flop
, orrotate
. If you takeremoveMetadata
into account, there are even more permutations to consider.This new setting removes the need to consider all these things.
PR?)
I have attempted to test initial behavior and ensure that, unless you opt-in, that behavior is exactly the same. So no, there shouldn't be any breaking changes.