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

smartcrop with 90/270 rotation: bad extract area #1154

Closed
ralrom opened this issue Mar 12, 2018 · 3 comments
Closed

smartcrop with 90/270 rotation: bad extract area #1154

ralrom opened this issue Mar 12, 2018 · 3 comments

Comments

@ralrom
Copy link

ralrom commented Mar 12, 2018

It seems that rotating an image before cropping it causes a bad extract area error. This is possibly related to #1134

Here is the minimal breaking setup

@lovell lovell added the triage label Mar 12, 2018
@lovell
Copy link
Owner

lovell commented Mar 13, 2018

Thanks, I've been able to reproduce this problem.

The input image appears to have an EXIF orientation value of 256, which falls outside the 0-8 validity range. This invalid orientation value is clipped to 8, which will result in a 270 degree rotation to "correct". This means even if it were to work, the output might not be what you'd expect.

Broken metadata aside, there's still an underlying problem. Here's a minimal test case that ignores the EXIF orientation but still fails with the smartcrop: bad extract area error in sharp v0.20.0.

// https://raw.githubusercontent.com/ralrom/sharp-bug-repro/master/images/apocalypse.jpg
sharp('apocalypse.jpg') // 1024x1034
  .rotate(270)
  .resize(256)
  .crop(sharp.strategy.entropy)
  .toFile('breaks.jpg', console.log);

@lovell lovell added bug and removed triage labels Mar 13, 2018
@lovell lovell changed the title Bad extract area on rotated images smartcrop with 90/270 rotation: bad extract area Mar 13, 2018
@lovell
Copy link
Owner

lovell commented Mar 13, 2018

The underlying problem here was that both dimensions had suffered a cumulative rounding error, one above the target dimensions and the other below.

It's an edge case so thank you for taking the time to report it with a reproducible test case.

Commit f60f7da adds these dimensions as a unit test and provides the fix to clamp values before attempting the smartcrop operation. This will be in v0.20.1.

@lovell lovell added this to the v0.20.1 milestone Mar 13, 2018
@lovell
Copy link
Owner

lovell commented Mar 17, 2018

v0.20.1 now available, thank you for reporting this problem.

@lovell lovell closed this as completed Mar 17, 2018
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