lovell / sharp

High performance Node.js image processing, the fastest module to resize JPEG, PNG, WebP, AVIF and TIFF images. Uses the libvips library.
https://sharp.pixelplumbing.com
Apache License 2.0
29.01k stars 1.29k forks source link

Proposal: opt-in mode to pre-orient images #4144

Open happycollision opened 3 months ago

happycollision commented 3 months ago

Please forgive me for not using your feature request template. I still hit all the points that you are intending me to supply, but I have done so using a different narrative structure.

Proposal: add an opt-in mode to conceptualize all commands starting from display orientation instead of actual orientation

I have already done 80% (?) of the work (assuming you like my approach) for this proposal in a fork. You can look at the tests if you want to see the changes I made to achieve the API I am proposing. It is still a work in progress, but the new mode already works as intended.

Summary

Sharp manipulates images based on actual orientation instead of display orientation. This creates surprising results that many developers can fix with .rotate(). But sometimes .rotate() is not viable or unavailable. (Read more below for why that might be.)

I'd like to propose an opt-in mode that allows users to issue commands to Sharp based on starting from the display orientation of an image instead of the actual orientation. In other words, Sharp should effectively load an image, auto-orient it and then let users continue to issue .rotate and .flip/.flop commands.

See the "This solution" section below for the finer details.

Terminology

If there is agreed-upon language for these concepts, I am unaware, so I will use the following:

Actual Orientation EXIF Orientation Display Orientation
8
no value

Problems inherent with EXIF-oriented image rotation in Sharp

Anyone who's spent enough time manipulating images with Sharp or a library which uses it under the hood knows that some images are oriented via EXIF data and some are oriented by virtue of how the pixels are arranged in the file. When you strip the metadata from images that use it for orientation, your images might display differently than they did before. A solution to this problem is to use the .rotate method without any arguments. This flips/flops and rotates an image to orient it based on the EXIF orientation, then strips the EXIF orientation data. The result is an image whose (removed) EXIF orientation is now baked in to the image. The display orientation is now the actual orientation, and it matches the previous EXIF orientation's instructions.

There are some problems that arise even when developers are aware of Sharp's .rotate(undefined) functionality.

Certain orientations prevent flip/flop after .rotate()

If you use .rotate() on an image with certain EXIF orientations, you will not be able to flip or flop after that, because those operations were already performed.

EXIF Orientation can flip can flop
1
2
3
4
5
6
7
8

You cannot rotate again from the display orientation

What if you want to apply the EXIF orientation and then rotate 90 degrees? Sharp does not allow multiple calls to .rotate, so sharp(img).rotate().rotate(90) will not work.

"Does not allow" really means "is not supported". If you want to rotate your image any value other than a multiple of 90, then calling rotate twice works just fine. Example: sharp(img).rotate().rotate(45) rotates 45 degrees from the display orientation, but is not a documented behavior.

Workarounds

Sharp gives you all the tools you need to work around these situations yourself. So if you are the one using Sharp directly you have two options:

Workaround 1: Buffer out first, then read and manipulate again

One solution is to sharp(sharp(image).rotate().toBuffer()).rotate(90).

Workaround 2: Do the orientation yourself, correcting for desired rotation/flip/flop

Another solution is to do the entire re-orientation manually.

  1. Get the EXIF orientation: figure out the flip/flop and rotation needed
  2. XOR the flip/flop against what your desire is
  3. Add your desired rotation to the final rotation amount
  4. Give sharp the resulting commands

You basically pretend Sharp doesn't have an auto-rotate feature at all.

How these workarounds fall short

Either of these work, but there are two issues I see with them: Users of Sharp get more complexity than they bargained for, and users of downstream tools might be completely unable to achieve what they desire.

Users of Sharp

Users of Sharp directly need to dig into EXIF orientation and understand how it works, which might be more than they bargained for when they wanted to rotate an image. Many of them will probably be just fine with .rotate() and call it a day. But anyone who wants to also apply their own rotations or flip/flop will need to strap in and read up on EXIF orientations.

Users of downstream tools

Users of downstream tools are stuck if the tool creator didn't provide a way to deal with EXIF-oriented images. (That is what led me here.)

An example is the imagetools library which gives users a way to declare image transformations at import. That tool is used by an even further downstream tool for SvelteKit called @sveltejs/enhanced-img, which modifies your HTML to create responsive images via srcset. It is incredibly convenient, but as of now, there is no great way to deal with images that are oriented via their EXIF data.

import MyImage from "./image.jpg?enhanced"
// Whoops. Any EXIF-oriented images will display wrong

import MyImage from "./image.jpg?enhanced&rotate=0"
// Nice try, but nope. That ends up being a no-op.

There are obviously tons of other tools on NPM that use Sharp. How many of them handle EXIF orientation in ways that work for all their users?

You could argue that the creators of all these tools should consider this problem themselves and do the requisite corrections directly in their own code or provide an API that actually allows users to declare what they want. You could also argue that the developers who use these tools should pre-transform their images as well. (They could use Sharp to do it!)

Why should Sharp address this issue?

You can already manipulate images in Sharp to get your desired rotation. All it takes is for developers to write more/different code when they use Sharp. Here's what everyone can do with the tools that Sharp already provides:

But that is a lot of slow churn with a low chance of success.

Sharp already has all the information needed to satisfy all these users' desire: "I want to manipulate my images starting from how I see them displayed. I don't care about EXIF orientation."

If Sharp implements this proposal, none of the churn mentioned above needs to happen, and surely some code can be removed from projects already working around this issue.

This solution

It seems to me there are two ways to think about manipulating images with Sharp (or any tool).

  1. Start manipulation from the actual orientation
  2. Start manipulation from the display orientation

Sharp uses option 1.

I'd like to propose an opt-in mode to Sharp that behaves as if all images start from their display orientation (whether actual or EXIF), and manipulates images from there.

You could opt in to this mode on a per instance basis via options.

sharp(img, { useInitialOrientation: true })

Now, if you want to rotate an image slightly from how it displays, you can just do this:

// Multiples of 90 are no different from everything else
sharp(img, {useInitialOrientation: true}).rotate(10)
sharp(img, {useInitialOrientation: true}).rotate(90)

Alternatively—for people like me using libraries built on Sharp that do not expose it—you could use an environment variable to default Sharp into this mode for all instances.

$ SHARP_USE_INITIAL_ORIENTATION=true npm run do-something-with-sharp

I hope this proposal is worth some consideration. I do have a POC branch going as I mentioned before. I have added several tests to ensure that the behavior I am describing works for all EXIF orientations. I am now in the process of fixing the approximately 15 existing tests that fail when you use the ENV var.

lovell commented 3 months ago

Thanks for the detailed proposal. My understanding of this is that there are 2 main topics being discussed here, both of which are a good idea.

  1. Fix the bug to allow the use of a flip and/or flip operation when also using parameter-less rotate() for images containing EXIF Orientations 2, 4, 5 or 7.
  2. Add syntactic sugar for the parameter-less rotate(), either a constructor property as you suggest or perhaps autoOrient() (e.g. #711). This would involve improving the docs to make it clear that both auto-orientation and rotate(angle) can be used in the same pipeline and will deprecate use of the parameter-less rotate().

(Providing a runtime environment variable to control this feels like pain for me rather than benefit for thee, so no interest in that, sorry.)

happycollision commented 3 months ago

Sounds like a winning compromise! I completely understand that taking on new features is very tricky since now you have to support them. I think adding the syntactic sugar for .rotate() will actually solve the problem in most cases for downstream wrappers, because now they would need to support an additional command with a different name, thus preventing them from missing the nuance inherent to .rotate's various invocations.

And I could be wrong (because I got in and out of several patches of weeds) but I think that sharp(img).rotate().rotate(90) does not currently work the way one might think about it. The 90 degree rotation is not applied in addition to the orientation, but rather instead of it. Double check me, but I am pretty sure that is the case. I'll adjust my branch to change the API to match what you'd prefer. At the very least, I'll rebase against some tests that fail on the cases we'd be trying to adjust for.

labsforge commented 2 weeks ago

I'm having exactly this problem. I need to pipe rotations to 1st auto-orient and that add display rotation. I got surprised with this issue because I only use the original photo for the final export, and until then I use a smaller resized image that already got the .rotate() and got saved without metadata, so all previews/thumbnails were showing fine and I assumed everything was working. But when I use the original file, the display orientation is always ignored and only the auto-orientation from exif is applied. doing sharp(input.withMetadata().rotate().toBuffer()).rotate(90) may be a workaround, but would be better if not needed.

happycollision commented 2 weeks ago

Hey @labsforge, I’ve got a PR open in this repo with a branch that might do what you want. It is definitely not official and I will almost certainly rebase it as @lovell requests changes and considers how best to incorporate it into such a long-standing project.

It seems like he is really busy at the moment, so I doubt that work will be accomplished soon.

In the meantime, it’s probably safe enough to fork my branch to have your own copy that won’t change out from under you, and follow the instructions on how to build it locally.

If all of that is a bit much, the workaround you’ve been using ought to work and—I am only guessing—probably doesn’t degrade your images in a noticeable fashion.

lovell commented 2 weeks ago

Thanks both, the autoOrient proposal is still on the list for inclusion in the next major-ish release.

labsforge commented 2 weeks ago

Hi @happycollision I've seen your PR and can't wait to see it on master 🙌 for now I'll be using the workaround and watch how this discussion develops. I believe sharp(await sharp(input).withMetadata().rotate().toBuffer()).withMetadata() will do it for now. Thank you all