sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.44k stars 1.89k forks source link

enhanced:img, imagetools, Sharp, and incorrectly rotated images #12262

Open happycollision opened 3 months ago

happycollision commented 3 months ago

Describe the bug

This is a bit tricky. It isn't a bug with enhanced:img, but I do think it might be enhanced:img's problem as the behavior is unexpected. I apologize if you'd rather have this as a feature request.

You may have noticed when using enhanced:img that some processed images get rotated for some reason. The underlying reason is not with imagetools—as discovered in an issue on the imagetools repo—but with Sharp. However, the owner of the Sharp repo maintains that this is the expected behavior, which seems pretty reasonable.

Summary: Some images are rotated via metadata, and some are rotated via rearranging the pixels. Sharp drops metadata during resize by default and so any images rotated via metadata will appear in a surprising orientation after said resize.

There is a workaround for this problem, which is good to know for users of Sharp. For users of imagetools, that knowledge is somewhat useful because they could opt to allow all metadata to stay in tact if they like:

imagetools({
  removeMetadata: false,
})

This works, and with my tests shows that the images are no longer displaying the wrong orientation. Of course has drawbacks because stripping metadata on the web is pretty important in many situations. But it at least is an option.

However, for users of enhanced:img, there is no way (that I am aware of) to pass config meant for imagetools. And since the order of operations seems to put rotate somewhere after resize, doing something like ?enhanced&rotate=0&w=300 doesn't really help.

Reproduction

You can follow the two links above for reproductions if you want to see them.

Logs

No response

System Info

(Doesn't really matter...)

  System:
    OS: macOS 14.4.1
    CPU: (8) arm64 Apple M1 Pro
    Memory: 959.30 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.10.0 - ~/.asdf/installs/nodejs/20.10.0/bin/node
    npm: 10.2.3 - ~/.asdf/plugins/nodejs/shims/npm
    pnpm: 9.0.6 - ~/.asdf/installs/nodejs/20.10.0/.npm/bin/pnpm
    bun: 1.0.3 - ~/.bun/bin/bun
  Browsers:
    Brave Browser: 124.1.65.126
    Safari: 17.4.1
  npmPackages:
    @sveltejs/adapter-netlify: ^3.0.0 => 3.0.2 
    @sveltejs/enhanced-img: ^0.2.0 => 0.2.0 
    @sveltejs/kit: ^2.0.0 => 2.5.5 
    @sveltejs/vite-plugin-svelte: ^3.0.0 => 3.1.0 
    svelte: 5.0.0-next.136 => 5.0.0-next.136 
    vite: ^5.0.3 => 5.2.8

Severity

blocking an upgrade

Additional Information

About the "severity" I chose: This blocks me from using enhanced:img, not SvelteKit. Not sure if that is the right way to mark this... so I chose "blocking an upgrade". Let me know if I should change it and I will be happy to.

I am not sure the place where such a problem should be handled. I agree that Sharp is behaving as intended. Should imagetools add a little sugar and "fix" the problem? Or should enhanced:img?


UPDATE: 2024-05-29

Lest anyone think I have not done some basic research and tinkering on this problem...

Background: EXIF Orientation

Working with image that use EXIF orientation can produce strange results. An image that looks perfectly fine on my computer...

on_computer

...behaves oddly when imported via ?enhanced and displayed on a webpage.

Step1

This is because the image (like many unaltered images captured with phones and cameras), uses EXIF metadata to tell the computer which way to rotate (and possibly flip) the image, rather than baking that rotation into the actual pixels of the image.

Attempt 1: Just rotate the image via rotate=X

We can try that...

Attempt1

But it rotates the image without rotating the canvas (in photo-editing parlance). See how the words "left" and "right" are missing? Huh. Moving on.

Rotate the image in CSS?

If I need to explain that using CSS is not the point of enhanced-image, then... I dunno. We just disagree. But yeah, this probably works in some cases.

Attempt 2: Tell imagetools not to strip metadata

That works!

Attempt2

But, there is no API within enhanced-img to do that. If you look closely at the code above, you'll see that I forked enhanced-img and added a way to pass the { removeMetadata: false } config.

That is something we can add to enhanced-img! So does that mean that allowing this config will solve the problem?

No.

Stripping metadata is a necessity for the web for multiple reasons. Not removing metadata just to get the image orientation right seems like a bad tradeoff to me.

Attempt 3: Use Sharp#rotate() (still a fork)

We are already in territory where we have to change enhanved-img to solve the problem. So let's try something that doesn't force us to keep our metadata around.

Let's try adding the following to the imagetools config:

const imagetools_opts = {
    extendTransforms: (builtIns) => {
        return [
            function noRotate(config, ctx) {
                if (!config.noRotate && !ctx.manualSearchParams.has("noRotate")) {
                    return
                }

                return function customTransform(image) {
                    return image.rotate(undefined, {
                        background: getBackground(config, image),
                    })
                }
            },
            ...builtIns,
        ]
    },
    /* ... snip ... */
}

The imagetools library has great hooks for us to add our own functionality. The noRotate transform defined above allows us to explicitly call Sharp's .rotate() method on the image. Calling without a specified rotation will actually use the EXIF data to rotate the image by rewriting pixels.

Now we can use noRotate in our import query params. Let's see how it works:

Attempt3

Uh... I am starting to suspect that there is some strange interaction between enhanced-img and imagetools...

Attempt 4: Just use imagetools

For a sanity check, I'll remove the defaultDirectives definitition that enhanced-img gives to imagetools config. This essentially turns enhanced-image into a very thin (almost absent) wrapper around imagetools.

So now we can basically just test this with imagetools alone...

Attempt4

It works! But we can't use enhanced-img. Hmm.

I also know that Sharp is a little particular about calling Sharp#rotate() multiple times. So, out of curiosity, if I wanted to now rotate my correctly-oriented image, can I? Let's rotate that by 90 degrees with ?enhanced&noRotate&rotate=90.

Attempt4 1

Oh okay, can't rotate again. Well that's too bad. Hm. Let's do 95 degrees for the heck of it, which I am sure won't work—

Attempt4 2

...

...

...

son-of-a

Conclusion

There are lots of gotchas and strange-seeming behaviors above. Some happen inside enhanced-image, some inside imagetools, and some (possibly) inside Sharp. There are more that I didn't mention as well.

Likely the most common case that needs to be solved is just to rotate the image properly based on its EXIF orientation, without further rotation. However, this tool is more general use than just "get my image rotated correctly". Some people will absolutely want to add their own rotation afterward. And it would be nice if that were not a crazy amount of trial and error to figure out.

And forget about trying to import via a glob on a bunch of images with different orientations, then performing additional rotations or flips.

I did open a PR to imagetools to solve the initial orientation problem completely seamlessly via an opt-in behavior change. With it, you can actually rotate an image 90 degrees after correcting its orientation. And since nobody wants to think about this stuff, the opt-in does not require any special query params. You just treat all your images the same way, regardless of metadata.

At least that is the goal.

jasonlyu123 commented 3 months ago

I feel like the more optimized way is to apply the "pixel rotation" rather than keeping metadata. From my understanding, I wouldn't phrase this as "incorrectly rotated image". The image is shot rotated but after stripping metadata, the image viewers no longer rotate it when presenting it.

To apply the "pixel rotation", we can pass a rotation option to imagetools based on the EXIF origination. But It probably makes more sense for imagetools to do this as a part of the stripping metadata.

happycollision commented 3 months ago

I've upstreamed a PR into imagetools. I'd like to get eyes on it to see if this makes sense to everyone. https://github.com/JonasKruckenberg/imagetools/pull/724

CaptainCodeman commented 3 months ago

I've always found Sharp / libvips to be pretty good, but a lot comes down to the rotation of the pixel data vs the exif flag.

Worth a read: https://www.daveperrett.com/articles/2012/07/28/exif-orientation-handling-is-a-ghetto/

Some test images that are useful: https://github.com/recurser/exif-orientation-examples

happycollision commented 3 months ago

The examples in that article are nuts. Glad things are better today than they were then.

After working on the PR in question inside imagetools, I do wonder if Sharp should actually expose options for how to model initial orientation. I agree with the maintainer that Sharp is working as intended, but I believe getting it right would be far easier inside Sharp than inside something built on top of Sharp.

CaptainCodeman commented 3 months ago

.rotate() should do it:

"If no angle is provided, it is determined from the EXIF data. Mirroring is supported and may infer the use of a flip operation."

https://sharp.pixelplumbing.com/api-operation#rotate

happycollision commented 3 months ago

Heh. If you can get all the tests I added to pass by using just .rotate(), I'll eat my hat. 😂

happycollision commented 2 months ago

I've finally gotten to where the code is nearly done and have made a proposal against the Sharp package directly: https://github.com/lovell/sharp/issues/4144