Cropped images are sometimes 1 pixel too small #123

Activity

1. jwagner commented on Mar 20, 2023

   

   jwagner

   on Mar 20, 2023

   Owner

   More actions

   Hey @wave-dash thanks for the detailed report. I don't think this is the correct repo though. Can you please report it against https://github.com/jwagner/smartcrop-cli ? Looks like a rounding error somewhere. I don't have time to go digging for it right now but I should get to it eventually. :)
2. wave-dash commented on Mar 21, 2023

   

   wave-dash

   on Mar 21, 2023

   Author

   More actions

   Sure thing, sorry about that. This repo seemed more active, and I figured the code for the two would be mostly the same.
3. kamiyo commented on Aug 6, 2023

   

   kamiyo

   on Aug 6, 2023

   More actions

   I actually have a similar problem using smartcrop-sharp. When passing it options like this:

   const { topCrop } = await smartcrop.crop(imageFile, {
               minScale: 1.0,
               width: 3,
               height: 2,
           });
   

   OR

   const { topCrop } = await smartcrop.crop(imageFile, {
               minScale: 1.0,
               width: 4000,
               height: 2666,
           });
   

   It always returns a height of 2656 instead. I think the issues are here:

   smartcrop.js/smartcrop.js

   Lines 120 to 121 in de2d143

   	options.cropWidth = ~~(options.cropWidth * prescale);
   	options.cropHeight = ~~(options.cropHeight * prescale);

   
   

   smartcrop.js/smartcrop.js

   Lines 147 to 148 in de2d143

   	crop.x = ~~(crop.x / prescale);
   	crop.y = ~~(crop.y / prescale);

   In the first pair of lines, I believe you are scaling down the dimensions for the downsampled image (for processing ease, I'm assuming). But since it's floored, once you scale back up in the second pair of lines, it won't be at the correct width, especially if the initial wasn't a multiple of 256.

   To prove the above example in pseudocode,

   prescale = 0.064
   downsampledCropHeight = floor(2666 * 0.064) = 170
   finalCropHeight = floor(170 / 0.064) = 2656
   

   I don't know what the solution should be. You could force based on aspect ratio. But as an end user, I guess I can just use the x and y of the crop, and force the dimensions myself.

   Just letting you know where I think the issue lies.

   👀React with 👀1Reacted by David Vaness