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]

Not sure if you'd rather do this or rename useExifOrientation. The main part of the pipeline is looking for useExifOrientation to do auto orientation, while the composite part of the pipeline looks for autoOrient.

[happycollision]

Update: I went ahead and replaced options.useExifOrientation with options.input.autoOrient everywhere, and let that cascade into the C++ stuff 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]

This warning says that previous rotations will be ignored, but that isn't actually true unless you reset options.angle and options.rotationAngle to 0 here. Unless you do, these two sets of commands will produce a different output:

sharp(img)
  .rotate(90) // cardinal angle
  .rotate(5)
// output will be rotated 95 degrees

sharp(img)
  .rotate(91) // non-cardinal angle
  .rotate(5)
// output will be rotated 5 degrees

Want me to throw in a fix for that? Or alter the warning? Or just leave as is?

Suggested change

	this.options.debuglog('ignoring previous rotate options');
	this.options.debuglog('ignoring previous rotate options');
	this.options.angle = 0;
	this.options.rotationAngle = 0;

[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.

[happycollision]

It occurs to me now that I am trying to test this new version of Sharp with @sveltejs/enhanced-img that the metadata is not adjusted in time for some useful operations to take place. For example:

sharp(img, {autoOrient: true]}).metadata()

The above will show the metadata before any auto-orientation takes place, which seems like a headache from the perspective of someone using the tool. If I am asking sharp to auto-orient the image, why would I want the old width and height?

I see several ways to work this issue:

1. No change. All metadata is from the input state of the image, the example getNormalSize function in the docs is all anyone needs.
2. If autoOrient is true, drop "orientation" and swap width and height as necessary from metadata.
3. Add an option to .metadata({ autoOrient: boolean }) that opts you in to view the adjusted metadata.
4. Export a function autoOrientMetadata(metadata) that devs can use as needed on metadata objects.
5. Add an appliedOrientation property that reports the width and height again, changing them if necessary based on exif orientation:

{
  format: 'jpeg',
  width: 4032,
  height: 3024,
  space: 'srgb',
  channels: 3,
  depth: 'uchar',
  density: 72,
  chromaSubsampling: '4:2:0',
  isProgressive: false,
  resolutionUnit: 'inch',
  hasProfile: false,
  hasAlpha: false,
  orientation: 6,
+ appliedOrientation: {
+   width: 3024,
+   height: 4032
+ }
}

It seems like a useful thing to just include that metadata so developers can see it when they inspect the thing and remain blissfully ignorant about which transforms are needed for each orientation.

[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 🙏