-
-
Notifications
You must be signed in to change notification settings - Fork 460
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
Replace Round function with Ceil in fit method (Black line) #250
base: master
Are you sure you want to change the base?
Conversation
There is a problem when fitWidth = 100.234. Round method make fitWidth equals to 100, but it should be 101 (to prevent black line appearing)
Not too unsurprisingly, two tests fail because of this change. The question is, what is correct:
/cc #240 (comment) |
@Dynom Ceil should be only for fitWidth var. I changed it. |
We need to add new test
So ceil only for fitWidth will crash some vertical images. Looks like we need to change algorithm. There is a problem that we can't have "half of the pixel", so i think there are 2 ways:
|
When I look at the implementation in libvips, it's way more complicated and contains many more decision branches (including: https://github.com/libvips/libvips/blob/1a836052385c220ad32f9ce0c9a3363f1f919ad6/libvips/arithmetic/max.c#L534). I think we should proceed with leaning more on vips_thumbnail_buffer() (see comment). That would take care of the resize/resample/fit/crop and general scaling. |
@lastonoga hi, @Dynom hi, we are using this app as a fork in our company and had the same issue, i have applied lastonoga's fix to our repo. i just wanted to let you know that i do this and thank you for the solution. when you merge this i will reset and update from the repo. have a nice day |
@evrenkutar the fix isn't "correct" for all situations, which is why we haven't merged it in yet. Great if it works for your situation, but be careful :-) |
There is a problem when fitWidth = 100.234. Round method make fitWidth equals to 100, but it should be 101 (to prevent black line appearing). #249
I don't know Go and how to run tests, so if you have ability to write tests, please, do it.