From 08b4242efe27c228a13ee49e9ab9bbaf71895367 Mon Sep 17 00:00:00 2001 From: Lovell Fuller Date: Mon, 21 Jul 2025 16:10:34 +0100 Subject: [PATCH] Ensure autoOrient occurs before non-90 rotation #4425 - Separate orient vs rotate ordering logic - Simplify EXIF auto-orient by using only rotate and/or flop --- docs/src/content/docs/changelog/v0.34.4.md | 3 + lib/constructor.js | 3 +- lib/resize.js | 11 +-- src/pipeline.cc | 80 ++++++++++------------ src/pipeline.h | 3 +- test/unit/rotate.js | 27 +++++++- 6 files changed, 74 insertions(+), 53 deletions(-) diff --git a/docs/src/content/docs/changelog/v0.34.4.md b/docs/src/content/docs/changelog/v0.34.4.md index 5ff63571..9855b09b 100644 --- a/docs/src/content/docs/changelog/v0.34.4.md +++ b/docs/src/content/docs/changelog/v0.34.4.md @@ -3,5 +3,8 @@ title: v0.34.4 - TBD slug: changelog/v0.34.4 --- +* Ensure `autoOrient` occurs before non-90 angle rotation. + [#4425](https://github.com/lovell/sharp/issues/4425) + * Ensure `autoOrient` removes existing metadata after shrink-on-load. [#4431](https://github.com/lovell/sharp/issues/4431) diff --git a/lib/constructor.js b/lib/constructor.js index 131a21a4..a349f42a 100644 --- a/lib/constructor.js +++ b/lib/constructor.js @@ -230,7 +230,8 @@ const Sharp = function (input, options) { angle: 0, rotationAngle: 0, rotationBackground: [0, 0, 0, 255], - rotateBeforePreExtract: false, + rotateBefore: false, + orientBefore: false, flip: false, flop: false, extendTop: 0, diff --git a/lib/resize.js b/lib/resize.js index 665e0c87..9e4aa78a 100644 --- a/lib/resize.js +++ b/lib/resize.js @@ -107,7 +107,7 @@ const mapFitToCanvas = { * @private */ function isRotationExpected (options) { - return (options.angle % 360) !== 0 || options.input.autoOrient === true || options.rotationAngle !== 0; + return (options.angle % 360) !== 0 || options.rotationAngle !== 0; } /** @@ -343,7 +343,7 @@ function resize (widthOrOptions, height, options) { } } if (isRotationExpected(this.options) && isResizeExpected(this.options)) { - this.options.rotateBeforePreExtract = true; + this.options.rotateBefore = true; } return this; } @@ -490,9 +490,12 @@ function extract (options) { // Ensure existing rotation occurs before pre-resize extraction if (isRotationExpected(this.options) && !isResizeExpected(this.options)) { if (this.options.widthPre === -1 || this.options.widthPost === -1) { - this.options.rotateBeforePreExtract = true; + this.options.rotateBefore = true; } } + if (this.options.input.autoOrient) { + this.options.orientBefore = true; + } return this; } @@ -566,7 +569,7 @@ function trim (options) { } } if (isRotationExpected(this.options)) { - this.options.rotateBeforePreExtract = true; + this.options.rotateBefore = true; } return this; } diff --git a/src/pipeline.cc b/src/pipeline.cc index cd4b468c..e2dd1182 100644 --- a/src/pipeline.cc +++ b/src/pipeline.cc @@ -92,30 +92,22 @@ class PipelineWorker : public Napi::AsyncWorker { // Calculate angle of rotation VipsAngle rotation = VIPS_ANGLE_D0; VipsAngle autoRotation = VIPS_ANGLE_D0; - bool autoFlip = false; bool autoFlop = false; if (baton->input->autoOrient) { // Rotate and flip image according to Exif orientation - std::tie(autoRotation, autoFlip, autoFlop) = CalculateExifRotationAndFlip(sharp::ExifOrientation(image)); + std::tie(autoRotation, autoFlop) = CalculateExifRotationAndFlop(sharp::ExifOrientation(image)); } rotation = CalculateAngleRotation(baton->angle); - // Rotate pre-extract - bool const shouldRotateBefore = baton->rotateBeforePreExtract && - (rotation != VIPS_ANGLE_D0 || autoRotation != VIPS_ANGLE_D0 || - autoFlip || baton->flip || autoFlop || baton->flop || - baton->rotationAngle != 0.0); - - if (shouldRotateBefore) { - image = sharp::StaySequential(image, - rotation != VIPS_ANGLE_D0 || - autoRotation != VIPS_ANGLE_D0 || - autoFlip || - baton->flip || - baton->rotationAngle != 0.0); + bool const shouldRotateBefore = baton->rotateBefore && + (rotation != VIPS_ANGLE_D0 || baton->flip || baton->flop || baton->rotationAngle != 0.0); + bool const shouldOrientBefore = (shouldRotateBefore || baton->orientBefore) && + (autoRotation != VIPS_ANGLE_D0 || autoFlop); + if (shouldOrientBefore) { + image = sharp::StaySequential(image, autoRotation != VIPS_ANGLE_D0); if (autoRotation != VIPS_ANGLE_D0) { if (autoRotation != VIPS_ANGLE_D180) { MultiPageUnsupported(nPages, "Rotate"); @@ -123,14 +115,20 @@ class PipelineWorker : public Napi::AsyncWorker { image = image.rot(autoRotation); autoRotation = VIPS_ANGLE_D0; } - if (autoFlip != baton->flip) { - image = image.flip(VIPS_DIRECTION_VERTICAL); - autoFlip = false; - baton->flip = false; - } - if (autoFlop != baton->flop) { + if (autoFlop) { image = image.flip(VIPS_DIRECTION_HORIZONTAL); autoFlop = false; + } + } + + if (shouldRotateBefore) { + image = sharp::StaySequential(image, rotation != VIPS_ANGLE_D0 || baton->flip || baton->rotationAngle != 0.0); + if (baton->flip) { + image = image.flip(VIPS_DIRECTION_VERTICAL); + baton->flip = false; + } + if (baton->flop) { + image = image.flip(VIPS_DIRECTION_HORIZONTAL); baton->flop = false; } if (rotation != VIPS_ANGLE_D0) { @@ -145,6 +143,7 @@ class PipelineWorker : public Napi::AsyncWorker { std::vector background; std::tie(image, background) = sharp::ApplyAlpha(image, baton->rotationBackground, false); image = image.rotate(baton->rotationAngle, VImage::option()->set("background", background)).copy_memory(); + baton->rotationAngle = 0.0; } } @@ -183,8 +182,7 @@ class PipelineWorker : public Napi::AsyncWorker { // When auto-rotating by 90 or 270 degrees, swap the target width and // height to ensure the behavior aligns with how it would have been if // the rotation had taken place *before* resizing. - if (!baton->rotateBeforePreExtract && - (autoRotation == VIPS_ANGLE_D90 || autoRotation == VIPS_ANGLE_D270)) { + if (autoRotation == VIPS_ANGLE_D90 || autoRotation == VIPS_ANGLE_D270) { std::swap(targetResizeWidth, targetResizeHeight); } @@ -206,7 +204,7 @@ class PipelineWorker : public Napi::AsyncWorker { // - input colourspace is not specified; bool const shouldPreShrink = (targetResizeWidth > 0 || targetResizeHeight > 0) && baton->gamma == 0 && baton->topOffsetPre == -1 && baton->trimThreshold < 0.0 && - baton->colourspacePipeline == VIPS_INTERPRETATION_LAST && !shouldRotateBefore; + baton->colourspacePipeline == VIPS_INTERPRETATION_LAST && !(shouldOrientBefore || shouldRotateBefore); if (shouldPreShrink) { // The common part of the shrink: the bit by which both axes must be shrunk @@ -398,7 +396,6 @@ class PipelineWorker : public Napi::AsyncWorker { image = sharp::StaySequential(image, autoRotation != VIPS_ANGLE_D0 || baton->flip || - autoFlip || rotation != VIPS_ANGLE_D0); // Auto-rotate post-extract if (autoRotation != VIPS_ANGLE_D0) { @@ -408,7 +405,7 @@ class PipelineWorker : public Napi::AsyncWorker { image = image.rot(autoRotation); } // Mirror vertically (up-down) about the x-axis - if (baton->flip != autoFlip) { + if (baton->flip) { image = image.flip(VIPS_DIRECTION_VERTICAL); } // Mirror horizontally (left-right) about the y-axis @@ -515,7 +512,7 @@ class PipelineWorker : public Napi::AsyncWorker { } // Rotate post-extract non-90 angle - if (!baton->rotateBeforePreExtract && baton->rotationAngle != 0.0) { + if (!baton->rotateBefore && baton->rotationAngle != 0.0) { MultiPageUnsupported(nPages, "Rotate"); image = sharp::StaySequential(image); std::vector background; @@ -656,21 +653,16 @@ class PipelineWorker : public Napi::AsyncWorker { if (composite->input->autoOrient) { // Respect EXIF Orientation VipsAngle compositeAutoRotation = VIPS_ANGLE_D0; - bool compositeAutoFlip = false; bool compositeAutoFlop = false; - std::tie(compositeAutoRotation, compositeAutoFlip, compositeAutoFlop) = - CalculateExifRotationAndFlip(sharp::ExifOrientation(compositeImage)); + std::tie(compositeAutoRotation, compositeAutoFlop) = + CalculateExifRotationAndFlop(sharp::ExifOrientation(compositeImage)); compositeImage = sharp::RemoveExifOrientation(compositeImage); - compositeImage = sharp::StaySequential(compositeImage, - compositeAutoRotation != VIPS_ANGLE_D0 || compositeAutoFlip); + compositeImage = sharp::StaySequential(compositeImage, compositeAutoRotation != VIPS_ANGLE_D0); if (compositeAutoRotation != VIPS_ANGLE_D0) { compositeImage = compositeImage.rot(compositeAutoRotation); } - if (compositeAutoFlip) { - compositeImage = compositeImage.flip(VIPS_DIRECTION_VERTICAL); - } if (compositeAutoFlop) { compositeImage = compositeImage.flip(VIPS_DIRECTION_HORIZONTAL); } @@ -1402,21 +1394,20 @@ class PipelineWorker : public Napi::AsyncWorker { Calculate the angle of rotation and need-to-flip for the given Exif orientation By default, returns zero, i.e. no rotation. */ - std::tuple - CalculateExifRotationAndFlip(int const exifOrientation) { + std::tuple + CalculateExifRotationAndFlop(int const exifOrientation) { VipsAngle rotate = VIPS_ANGLE_D0; - bool flip = false; bool flop = false; switch (exifOrientation) { case 6: rotate = VIPS_ANGLE_D90; break; case 3: rotate = VIPS_ANGLE_D180; break; case 8: rotate = VIPS_ANGLE_D270; break; - case 2: flop = true; break; // flop 1 - case 7: flip = true; rotate = VIPS_ANGLE_D90; break; // flip 6 - case 4: flop = true; rotate = VIPS_ANGLE_D180; break; // flop 3 - case 5: flip = true; rotate = VIPS_ANGLE_D270; break; // flip 8 + case 2: flop = true; break; + case 7: flop = true; rotate = VIPS_ANGLE_D270; break; + case 4: flop = true; rotate = VIPS_ANGLE_D180; break; + case 5: flop = true; rotate = VIPS_ANGLE_D90; break; } - return std::make_tuple(rotate, flip, flop); + return std::make_tuple(rotate, flop); } /* @@ -1641,7 +1632,8 @@ Napi::Value pipeline(const Napi::CallbackInfo& info) { baton->angle = sharp::AttrAsInt32(options, "angle"); baton->rotationAngle = sharp::AttrAsDouble(options, "rotationAngle"); baton->rotationBackground = sharp::AttrAsVectorOfDouble(options, "rotationBackground"); - baton->rotateBeforePreExtract = sharp::AttrAsBool(options, "rotateBeforePreExtract"); + baton->rotateBefore = sharp::AttrAsBool(options, "rotateBefore"); + baton->orientBefore = sharp::AttrAsBool(options, "orientBefore"); baton->flip = sharp::AttrAsBool(options, "flip"); baton->flop = sharp::AttrAsBool(options, "flop"); baton->extendTop = sharp::AttrAsInt32(options, "extendTop"); diff --git a/src/pipeline.h b/src/pipeline.h index d5d5b3fb..8edfff58 100644 --- a/src/pipeline.h +++ b/src/pipeline.h @@ -115,7 +115,8 @@ struct PipelineBaton { int angle; double rotationAngle; std::vector rotationBackground; - bool rotateBeforePreExtract; + bool rotateBefore; + bool orientBefore; bool flip; bool flop; int extendTop; diff --git a/test/unit/rotate.js b/test/unit/rotate.js index 218f4f99..0a7298b6 100644 --- a/test/unit/rotate.js +++ b/test/unit/rotate.js @@ -565,9 +565,9 @@ describe('Rotation', function () { .raw() .toBuffer(); - assert.strictEqual(r, 60); - assert.strictEqual(g, 73); - assert.strictEqual(b, 52); + assert.strictEqual(r, 61); + assert.strictEqual(g, 74); + assert.strictEqual(b, 51); }); it('Flip and rotate ordering', async () => { @@ -648,6 +648,27 @@ describe('Rotation', function () { assert.strictEqual(orientation, undefined); }); + it('Auto-orient and rotate 45', async () => { + const data = await sharp(fixtures.inputJpgWithLandscapeExif2, { autoOrient: true }) + .rotate(45) + .toBuffer(); + + const { width, height } = await sharp(data).metadata(); + assert.strictEqual(width, 742); + assert.strictEqual(height, 742); + }); + + it('Auto-orient, extract and rotate 45', async () => { + const data = await sharp(fixtures.inputJpgWithLandscapeExif2, { autoOrient: true }) + .extract({ left: 20, top: 20, width: 200, height: 100 }) + .rotate(45) + .toBuffer(); + + const { width, height } = await sharp(data).metadata(); + assert.strictEqual(width, 212); + assert.strictEqual(height, 212); + }); + it('Invalid autoOrient throws', () => assert.throws( () => sharp({ autoOrient: 'fail' }),