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

Cropped images are sometimes 1 pixel too small #123

Open
wave-dash opened this issue Mar 20, 2023 · 3 comments
Open

Cropped images are sometimes 1 pixel too small #123

wave-dash opened this issue Mar 20, 2023 · 3 comments

Comments

@wave-dash
Copy link

smartcrop --width 100 --height 400 %1 %~n1_out%~x1

When I run this command on kitty.jpg from the test suite, the output image is 99x400.

2023-03-19_22-47-36_dopus_crop

I assume this happens because of the skewed aspect ratio of the requested crop, I've encountered a similar "missing pixel" problem when cropping in Photoshop. Is there a way to force exact dimensions for the output?

@jwagner
Copy link
Owner

jwagner commented Mar 20, 2023

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

@wave-dash
Copy link
Author

Sure thing, sorry about that. This repo seemed more active, and I figured the code for the two would be mostly the same.

@kamiyo
Copy link

kamiyo commented Aug 6, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants