Skip to content
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

Ensure combination of resize and rotate operations is ordered (to match extract and rotate) #3319

Closed
3 tasks done
tomasklaen opened this issue Aug 3, 2022 · 6 comments
Closed
3 tasks done

Comments

@tomasklaen
Copy link

Possible bug

  • Running npm install sharp completes without error.
  • Running node -e "require('sharp')" completes without error.
  • I am using the latest version of sharp as reported by npm view sharp dist-tags.latest.
  System:
    OS: Windows 10 10.0.16299
    CPU: (4) x64 Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz
    Memory: 10.08 GB / 15.95 GB
  Binaries:
    Node: 18.7.0 - C:\Program Files\nodejs\node.EXE
    npm: 8.15.0 - C:\Program Files\nodejs\npm.CMD
  npmPackages:
    sharp: ^0.30.7 => 0.30.7

What are the steps to reproduce?

Resize an image into specific dimensions with fill fit mode, then try to rotate it 90 degrees.

What is the expected behavior?

Result image should be rotated, and have {oldHeight}x{oldWidth} dimensions, instead, it IS rotated, but somehow retained {oldWidth}x{oldHeight} dimensions.

Code sample

import sharp from 'sharp';

const image = sharp('./in.jpg');
image.resize({width: 400, height: 500, fit: 'fill'});
image.rotate(90);
await image.toFile('./out.jpg');

Sample image

painting.jpg

@tomasklaen
Copy link
Author

Happens for contain and cover fit modes as well.

I currently have to do this intermediate step in between resize and rotate to "fix" it by I guess resetting whatever is being mangled internally:

image = sharp(await image.png().toBuffer());

@lovell
Copy link
Owner

lovell commented Aug 4, 2022

As you've seen, you'll currently need to break this operation into two pipelines - see #1922, #2086 and related issues.

This has come up a few times, and I've been meaning to modify sharp to ensure that the combination of resize+rotate is also "ordered" to match the behaviour of extract+rotate.

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

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

Let's use this issue to track that enhancement.

@lovell lovell added enhancement and removed triage labels Aug 4, 2022
@lovell lovell changed the title Resize with fill fit breaks subsequent operations such as rotate Ensure combination of resize and rotate operations is ordered (to match extract and rotate) Aug 4, 2022
@tomasklaen
Copy link
Author

I'm running into more and more chaining issues where sharp produces unexpected results. For example:

image.extract(...).rotate(90).extract(...);

The result is a completely different region than expected. I guess the second extract overwrites the first one?

So what I currently have to do is, similar to the above workaround, I made this flush function:

const flush = async () => {
	const {data, info} = await image.raw().toBuffer({resolveWithObject: true});
	image = sharp(data, {raw: info});
};

And I call it in between every operation just to be sure sharp chaining issues don't break something.

If sharp doesn't really support chaining operations, it shouldn't encourage it via it's API design 😞. Or have some big warnings everywhere that only one operation type per chain is allowed, and only certain orders are supported, and all other quirks and limitations I'm currently not aware of.

@lovell
Copy link
Owner

lovell commented Aug 9, 2022

Please see the discussion at #241

lovell added a commit that referenced this issue Aug 20, 2022
Emit warnings when previous ops might be ignored
Flip and flop now occur before rotate, if any
@lovell
Copy link
Owner

lovell commented Aug 21, 2022

Commit 212a6e7 improves the situation slightly, adding examples and test cases, as well as emitting warnings when previous calls in the same pipeline will be ignored. This will be in v0.31.0.

@lovell lovell added this to the v0.31.0 milestone Aug 21, 2022
@lovell
Copy link
Owner

lovell commented Sep 5, 2022

v0.31.0 is now available with these improvements. Let's continue the other parts of this discussion at #241

@lovell lovell closed this as completed Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants