-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Rotation sugar and more #4151
base: main
Are you sure you want to change the base?
Rotation sugar and more #4151
Changes from 8 commits
b9f3c23
0f7d21a
ef5f283
1837cef
44d7cd0
b52c48c
5bd0471
800b22d
c3e9be3
30296cf
c5dd3e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -364,24 +364,72 @@ declare namespace sharp { | |
//#region Operation functions | ||
|
||
/** | ||
* Rotate the output image by either an explicit angle or auto-orient based on the EXIF Orientation tag. | ||
* Rotate the output image by either an explicit angle | ||
* or auto-orient based on the EXIF `Orientation` tag. | ||
* | ||
* If an angle is provided, it is converted to a valid positive degree rotation. For example, -450 will produce a 270deg rotation. | ||
* If an angle is provided, it is converted to a valid positive degree rotation. | ||
* For example, `-450` will produce a 270 degree rotation. | ||
* | ||
* When rotating by an angle other than a multiple of 90, the background colour can be provided with the background option. | ||
* When rotating by an angle other than a multiple of 90, | ||
* the background colour can be provided with the `background` option. | ||
* | ||
* If no angle is provided, it is determined from the EXIF data. Mirroring is supported and may infer the use of a flip operation. | ||
* If no angle is provided, it is determined from the EXIF data. | ||
* Mirroring is supported and may infer the use of a flip operation. | ||
* | ||
* The use of rotate implies the removal of the EXIF Orientation tag, if any. | ||
* The use of `rotate` without an angle will remove the EXIF `Orientation` tag, if any. | ||
Comment on lines
+367
to
+379
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not only a change to reflect the doc improvements, but also to sync with the docs created in the |
||
* | ||
* Method order is important when both rotating and extracting regions, for example rotate(x).extract(y) will produce a different result to extract(y).rotate(x). | ||
* @param angle angle of rotation. (optional, default auto) | ||
* @param options if present, is an Object with optional attributes. | ||
* Only one rotation can occur per pipeline (aside from an initial call without | ||
* arguments to orient via EXIF data). Previous calls to `rotate` in the same | ||
* pipeline will be ignored. | ||
* | ||
* Multi-page images can only be rotated by 180 degrees. | ||
* | ||
* Method order is important when rotating, resizing and/or extracting regions, | ||
* for example `.rotate(x).extract(y)` will produce a different result to `.extract(y).rotate(x)`. | ||
* | ||
* @example | ||
* const pipeline = sharp() | ||
* .rotate() | ||
* .resize(null, 200) | ||
* .toBuffer(function (err, outputBuffer, info) { | ||
* // outputBuffer contains 200px high JPEG image data, | ||
* // auto-rotated using EXIF Orientation tag | ||
* // info.width and info.height contain the dimensions of the resized image | ||
* }); | ||
* readableStream.pipe(pipeline); | ||
* | ||
* @example | ||
* const rotateThenResize = await sharp(input) | ||
* .rotate(90) | ||
* .resize({ width: 16, height: 8, fit: 'fill' }) | ||
* .toBuffer(); | ||
* const resizeThenRotate = await sharp(input) | ||
* .resize({ width: 16, height: 8, fit: 'fill' }) | ||
* .rotate(90) | ||
* .toBuffer(); | ||
* | ||
* @param {number} [angle=auto] angle of rotation. | ||
* @param {Object} [options] - if present, is an Object with optional attributes. | ||
* @param {string|Object} [options.background="#000000"] parsed by the [color](https://www.npmjs.org/package/color) module to extract values for red, green, blue and alpha. | ||
* @returns {Sharp} | ||
* @throws {Error} Invalid parameters | ||
* @returns A sharp instance that can be used to chain operations | ||
*/ | ||
rotate(angle?: number, options?: RotateOptions): Sharp; | ||
|
||
/** | ||
* Alias for calling `rotate()` with no arguments, which orients the image based | ||
* on EXIF orientsion. | ||
* | ||
* This operation is aliased to emphasize its purpose, helping to remove any | ||
* confusion between rotation and orientation. | ||
* | ||
* @example | ||
* const output = await sharp(input).autoOrient().toBuffer(); | ||
* | ||
* @returns {Sharp} | ||
*/ | ||
autoOrient(): Sharp | ||
|
||
/** | ||
* Flip the image about the vertical Y axis. This always occurs after rotation, if any. | ||
* The use of flip implies the removal of the EXIF Orientation tag, if any. | ||
|
@@ -900,6 +948,13 @@ declare namespace sharp { | |
} | ||
|
||
interface SharpOptions { | ||
/** | ||
* Auto-orient based on the EXIF `Orientation` tag, if present. | ||
* Mirroring is supported and may infer the use of a flip operation. | ||
* | ||
* Using this option will remove the EXIF `Orientation` tag, if any. | ||
*/ | ||
autoOrient?: boolean; | ||
/** | ||
* When to abort processing of invalid pixel data, one of (in order of sensitivity): | ||
* 'none' (least), 'truncated', 'error' or 'warning' (most), highers level imply lower levels, invalid metadata will always abort. (optional, default 'warning') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 In a separate branch (the one I originally created to prove my ideas before making this proposal), I kept the behavior of I could rewrite a few things in this PR and do the same here. The idea being that if you use The documentation and runtime warnings could deprecate the use of 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,10 +77,10 @@ class PipelineWorker : public Napi::AsyncWorker { | |
// Rotate and flip image according to Exif orientation | ||
std::tie(autoRotation, autoFlip, autoFlop) = CalculateExifRotationAndFlip(sharp::ExifOrientation(image)); | ||
image = sharp::RemoveExifOrientation(image); | ||
} else { | ||
rotation = CalculateAngleRotation(baton->angle); | ||
} | ||
|
||
rotation = CalculateAngleRotation(baton->angle); | ||
|
||
Comment on lines
-80
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
// Rotate pre-extract | ||
bool const shouldRotateBefore = baton->rotateBeforePreExtract && | ||
(rotation != VIPS_ANGLE_D0 || autoRotation != VIPS_ANGLE_D0 || | ||
|
@@ -102,18 +102,14 @@ class PipelineWorker : public Napi::AsyncWorker { | |
image = image.rot(autoRotation); | ||
autoRotation = VIPS_ANGLE_D0; | ||
} | ||
if (autoFlip) { | ||
if (autoFlip != baton->flip) { | ||
image = image.flip(VIPS_DIRECTION_VERTICAL); | ||
autoFlip = false; | ||
} else if (baton->flip) { | ||
image = image.flip(VIPS_DIRECTION_VERTICAL); | ||
baton->flip = false; | ||
} | ||
if (autoFlop) { | ||
if (autoFlop != baton->flop) { | ||
image = image.flip(VIPS_DIRECTION_HORIZONTAL); | ||
autoFlop = false; | ||
} else if (baton->flop) { | ||
image = image.flip(VIPS_DIRECTION_HORIZONTAL); | ||
baton->flop = false; | ||
} | ||
if (rotation != VIPS_ANGLE_D0) { | ||
|
@@ -405,11 +401,11 @@ class PipelineWorker : public Napi::AsyncWorker { | |
image = image.rot(autoRotation); | ||
} | ||
// Mirror vertically (up-down) about the x-axis | ||
if (baton->flip || autoFlip) { | ||
if (baton->flip != autoFlip) { | ||
image = image.flip(VIPS_DIRECTION_VERTICAL); | ||
} | ||
// Mirror horizontally (left-right) about the y-axis | ||
if (baton->flop || autoFlop) { | ||
if (baton->flop != autoFlop) { | ||
image = image.flip(VIPS_DIRECTION_HORIZONTAL); | ||
} | ||
// Rotate post-extract 90-angle | ||
|
@@ -640,6 +636,32 @@ class PipelineWorker : public Napi::AsyncWorker { | |
composite->input->access = access; | ||
std::tie(compositeImage, compositeImageType) = sharp::OpenInput(composite->input); | ||
compositeImage = sharp::EnsureColourspace(compositeImage, baton->colourspacePipeline); | ||
|
||
if (composite->input->autoOrient) { | ||
// Calculate angle of rotation | ||
VipsAngle compositeAutoRotation = VIPS_ANGLE_D0; | ||
bool compositeAutoFlip = false; | ||
bool compositeAutoFlop = false; | ||
|
||
// Rotate and flip image according to Exif orientation | ||
std::tie(compositeAutoRotation, compositeAutoFlip, compositeAutoFlop) = | ||
CalculateExifRotationAndFlip(sharp::ExifOrientation(compositeImage)); | ||
|
||
compositeImage = sharp::RemoveExifOrientation(compositeImage); | ||
|
||
if (compositeAutoRotation != VIPS_ANGLE_D0) { | ||
compositeImage = compositeImage.rot(compositeAutoRotation); | ||
} | ||
// Mirror vertically (up-down) about the x-axis | ||
if (compositeAutoFlip) { | ||
compositeImage = compositeImage.flip(VIPS_DIRECTION_VERTICAL); | ||
} | ||
// Mirror horizontally (left-right) about the y-axis | ||
if (compositeAutoFlop) { | ||
compositeImage = compositeImage.flip(VIPS_DIRECTION_HORIZONTAL); | ||
} | ||
} | ||
|
||
// Verify within current dimensions | ||
if (compositeImage.width() > image.width() || compositeImage.height() > image.height()) { | ||
throw vips::VError("Image to composite must have same dimensions or smaller"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
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.
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 forautoOrient
.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.
Update: I went ahead and replaced
options.useExifOrientation
withoptions.input.autoOrient
everywhere, and let that cascade into the C++ stuff as well.