Rotation sugar and more

[happycollision]

Working branch for ideas discussed in this proposal.

• add tests
• fix flip/flop after auto-rotate
• add .autoOrient() (alias for .rotate())
• ensure you can chain an additional .rotate(angle) after .rotate()
• maybe add an option to always auto orient
• Drop TEMP commit
• Add constructor option { autoOrient: boolean }
• Add tests for composite option
• Add composite option { autoOrient: boolean }
• Documentation overhaul
• Add metadata adjustment

[happycollision]

Okay, @lovell, I think these changes are ready for a review.

There is a failing test, but it seems to be related to the recent change to recomb. (rebase error on my part)

[happycollision]

I find it very surprising that pulling this line out of the if/else block didn't make any tests fail. Perhaps it was never needed to begin with?

[happycollision]

General comment on all of these images: I don't know if you want me to pair this down, but I am brute-forcing via tests here.

[happycollision]

Not only a change to reflect the doc improvements, but also to sync with the docs created in the operation.js file.

[lovell]

Thank you very much for working on this.

Thinking a bit more about the public API for this, I prefer your original suggestion to add a constructor property as it can then be used consistently for additional inputs e.g. when compositing.

// PROPOSED API, NOT YET AVAILABLE
await sharp(input, { autoOrient: true })...

which would then also allow:

// PROPOSED API, NOT YET AVAILABLE
await sharp(base, { autoOrient: true })
  .composite([{
    input: respectExifOrientationOverlay,
    autoOrient: true
  }, {
    input: ignoreExifOrientationOverlay
  }])
  ...

This should involve moving the existing (internal) useExifOrientation option into inputDescriptor object and its various instances.

[happycollision]

I’ll probably make that change on or before Friday. I assume you still want to keep .rotate() working as is, since removing that would be a breaking change. But how about .autoOrient() (the method call)? I know I would personally like it to stay, because of other tools that do not expose a way to alter options for an instance, but do expose all methods.

[lovell]

I assume you still want to keep .rotate() working as is,

Yes please; it can be deprecated at some point in the future, but not yet.

because of other tools that do not expose a way to alter options for an instance

This feels like a missing feature of the tools. Do you have any examples? Let's try to improve the tools rather than create duplicate API.

[happycollision]

Summary

I think having both is valid for a couple reasons. But this is not my library and I certainly have no idea what goes through your head as the maintainer. I am thrilled that any of this is happening at all, and I'll gladly show the final, merged PR to the maintainers of the libraries mentioned below and encourage them to allow downstream users to have access to the new feature.

Explanation

This feels like a missing feature of the tools. Do you have any examples?

I agree with that sentiment, and it makes sense as the ideal. Let's take one case to illustrate a general problem that I assume arises naturally, though.

The whole reason I got interested in creating my proposal for these changes was because I'd like to use a great tool called svelte/enhanced-img. It uses another tool called imagetools which is what ultimately calls into Sharp. I described the problems there in a couple issues and briefly in my proposal.

Imagetools won't allow a rotation of either undefined, and there is no way for me to specify options for Sharp instances. Ideally, that library author would have noted the importance of .rotate(), but it is understandable to have missed it because of the name.

That tool seems to ignore the current options you can specify in Sharp. This is likely because the kind of things you do in the options for Sharp are out of scope. Reading raw data, creating new images from scratch, adding text on top of an image, etc. These are not what you might expect as common cases for a tool like this. Its main job is to take an image and allow the developer to created tailored versions of that image on the fly when importing them into their various components for use in a website.

All this to say, I can see where a developer of a tool might breeze right past options and go straight to all the methods documentation to see "how do I manipulate an image I've already loaded into a Sharp instance?"

Whether right or wrong, I understand why this might happen.

Adding an explicit method called autoOrient would help prevent other authors from missing the subtlety inherent to the .rotate method or from overlooking the options object since it is otherwise irrelevant to them. They'd just see it in the list of image operations and realize, "Oh I need to support this because that sounds like a real issue".

Let's try to improve the tools rather than create duplicate API.

In writing up the response above, I read the docs about Sharp's options object a little more intentionally than I have in the past. I'd argue that .autoOrient as a method fits the current ethos of Sharp, whereas adding it only as an option does not.

Whether technically correct at the level of implementation or not, the options seem concerned with how to read or create new things. The methods seem concerned with manipulating the image after you've read or created it. Adjusting the orientation feels very much like the latter and not like the former.

I wholeheartedly agree that making auto orientation available as a composite option makes sense. That is a feature I previously knew nothing about, and it would be a shame if autoOrient wasn't supported there.

Ultimately, I have convinced myself that both sharp(img).autoOrient() and sharp(base).composite([{ input: img, autoOrient: true }]) are valid uses. Which, based on the inner workings, might mean that sharp(img, { autoOrient: true }) will naturally fall out of that as well.

[happycollision]

I've added the options object API and gotten it working with composite images as well.

I assume that the docs need a little love at this point.

[happycollision]

Dropping the check for auto-orientation here seems to be what the rotate function actually should do if it is going to warn. Calling .rotate().rotate(90) is a legit operation—even if we now prefer .autoOrient().rotate(90). What we ought to warn about is an operation where commands are effectively dropped: .rotate(90).rotate(180). I changed the test to reflect this as well.

[happycollision]

I am commenting on this file only so this conversation can be in a single thread. These thoughts are about the entire PR.

Everything in this PR is backwards-compatible based on documentation, but not in actual effect.

The changes in behavior for .rotate() are evidenced by the failing tests that I added in one of my first commits. I am guessing there will be some people out there who are depending on the old behavior.

In a separate branch (the one I originally created to prove my ideas before making this proposal), I kept the behavior of .rotate() untouched while also introducing a new way of invoking auto-orientation.

I could rewrite a few things in this PR and do the same here. The idea being that if you use .rotate(), the re-orientation and subsequent calls to rotate would produce the old output, while using auto-orient would always invoke the new behavior.

The documentation and runtime warnings could deprecate the use of .rotate(), and encourage autoOrient. Then the code that preserves the old behavior could be removed at the next major.

Between the tests on my old branch which I could pull into this one, and the tests on this branch, I think it’d be fairly easy to juggle these behaviors.

[lovell]

Thanks for working on this, I'm very busy at the moment and haven't had a chance to review this. Given there might be slightly breaking changes this change is unlikely to make it into the next patch release, so I'll revisit once that is published.

[happycollision]

Take your time. I’d like to be sure to get this right than rush something out.

[happycollision]

Given there might be slightly breaking changes

I should have mentioned this last week, but the branch I was originally playing with preserved the old behavior and only changed to the new behavior as an opt-in. The only thing I never finished getting to work correctly were all the old tests when you used the ENV flag that I proposed, but you don't even want that, so I could probably get all of this working in a way that preserves the old behavior AND introduces the new. Then, you could just deprecate rotate(undefined) if you like.

[labsforge]

wow can't wait to see this merged 🙏

[lovell]

@happycollision Hi Don, I'm starting to prepare the next big release and your PR is on the list of things I'd like to include. Is this something you're still interested in? I realise it's been a while so no worries if not. The work-in-progress branch is at https://github.com/lovell/sharp/tree/hat

[happycollision]

Absolutely! What’s your timeline? There were lots of questions I had as I was developing that I’ll need to refresh myself with and I want to make sure I move at an appropriate clip for your schedule.

[lovell]

Great, glad to hear it, I've tried to answer some of your questions inline. This is open source so there are no deadlines or ETAs.

It's looking like the next sharp release will be dependent on an as-yet-unreleased libvips v8.16.1 so this PR isn't blocking anything right now.

[happycollision]

I've rebased my branch on the current state of hat. I'll probably have time this week to address feedback.

[happycollision]

@lovell, the kids gave me the gift of all falling asleep on time tonight, so I think I've finally addressed all your feedback. I am guessing that you'll still have some more thoughts on this PR. So let me clarify what has happened to this point:

• .rotate(undefined) is still supported, but is not documented. It calls .autoOrient() under the hood.
• multiple calls to .rotate(number) will only respect the final call. (This is not new to expected behavior, but is new to actual behavior)
• Adds instance.autoOrient(), sharp(img, {autoOrient: true}), and sharp(base).composite([{ input: img, autoOrient: true }]) to perform an initial rotation on the image based on exif data if present. Any calls to .rotate() thereafter are (effectively) applied after the initial auto orientation is applied.
• fixes flip and flop behaviors so that they can be applied after an exif auto rotation has been applied.

All of this is at the current tip of the hat branch.

The docs at this point might need a touch more work that I have done so far. But that is where this PR stands at this moment.

---

I changed the target branch to hat to make my changes more clear.

[happycollision]

I need to update this documentation and also the docs on rotate to indicate that this is the preferred method.

[happycollision]

Oh no those workflow runs. I’ll have a look when I can.