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: bad extract area #1134

Closed
pieh opened this issue Feb 20, 2018 · 6 comments
Closed

smartcrop: bad extract area #1134

pieh opened this issue Feb 20, 2018 · 6 comments

Comments

@pieh
Copy link

pieh commented Feb 20, 2018

In very specific case resizing with cropping set to sharp.strategy.attention or sharp.strategy.entropy will result with error smartcrop: bad extract area

Repository to reproduce - https://github.com/pieh/sharp-smartcrop-bad-extract-area-repro

This happens when width is set to 1110 and height is unspecified using image from above repository (it is 1600 x 1200)

In readme of repository I placed more outputs where it works fine:

  • with crop set to sharp.gravity.center
  • with slightly higher and lower widths (1120 and 1100 respectively)
  • with specified height (both calculated using source image aspect ratio and full image height)

This was originally reported in gatsbyjs/gatsby#4142 (just to keep paper trail - reproduction repository should be enough)

@lovell lovell added the triage label Feb 21, 2018
@lovell
Copy link
Owner

lovell commented Feb 21, 2018

Hi Michal, thank you for this detailed report and accompanying test repository.

The "attention" and "entropy" crop strategies rely on the resize() operation being provided with both a width and height but this isn't checked or enforced until after passing control to libvips.

I think the best fix here is for sharp to ignore the strategy choice should either width or height be missing as that situation will result in no crop taking place anyway. I'll take a look at this.

@lovell lovell added bug and removed triage labels Feb 21, 2018
@lovell lovell added this to the v0.19.1 milestone Feb 21, 2018
@pieh
Copy link
Author

pieh commented Feb 21, 2018

When resizing with just width (or height) crop strategy/gravity shouldn't have any effect - it's just scaling without cropping, right? What I find weird is that it works fine (doesn't throw error) with some width (f.e. 1120px or 1100px in my example) and not with others (1110px).

I will add conditional check to not use attention or entropy crop strategy in gatsby for now if it's just scaling and not cropping.

pieh added a commit to pieh/gatsby that referenced this issue Feb 21, 2018
KyleAMathews pushed a commit to gatsbyjs/gatsby that referenced this issue Feb 22, 2018
@lovell
Copy link
Owner

lovell commented Feb 22, 2018

This error occurs when cumulative rounding during the resize operation occasionally produces an image 1px smaller along one edge than the expected target dimensions. The bug here is that there's no conditional guard to prevent a "smartcrop" operation taking place when the image is already small enough.

I've added your example dimensions as a new unit test locally, which currently fails, and expect to push the fix that will get this test to pass within the next 24 hours. Thanks for reporting!

@lovell
Copy link
Owner

lovell commented Feb 24, 2018

v0.19.1 now available with the fix for this.

@ralrom
Copy link

ralrom commented Mar 10, 2018

Hi, I am getting the same error with a 1024x1034px image on version 0.20.0. I am also using gatsbyjs and I crop all images to a max width of 1024px

I have a project with quite a few images (about 100) and this is the only image that's causing problems:

stmichael-church

@lovell
Copy link
Owner

lovell commented Mar 11, 2018

@ralrom Please can you open a new issue with a minimal code sample (using only sharp) that exhibits this behaviour.

Repository owner locked and limited conversation to collaborators Mar 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants