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

Replace Round function with Ceil in fit method (Black line) #250

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lastonoga
Copy link

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.

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)
@Dynom
Copy link
Collaborator

Dynom commented Feb 20, 2019

Not too unsurprisingly, two tests fail because of this change. The question is, what is correct:

--- FAIL: TestCalculateDestinationFitDimension (0.00s)
    image_test.go:104: Fit dimensions calculation failure
        Expected : 710/555 (width/height)
        Actual   : 710/556 (width/height)
        {imageWidth:1279 imageHeight:1000 optionWidth:710 optionHeight:9999 fitWidth:710 fitHeight:555}
    image_test.go:104: Fit dimensions calculation failure
        Expected : 312/173 (width/height)
        Actual   : 312/174 (width/height)
        {imageWidth:900 imageHeight:500 optionWidth:312 optionHeight:312 fitWidth:312 fitHeight:173}

/cc #240 (comment)

@lastonoga
Copy link
Author

lastonoga commented Feb 20, 2019

@Dynom Ceil should be only for fitWidth var. I changed it.

@lastonoga
Copy link
Author

lastonoga commented Feb 20, 2019

We need to add new test

imageWidth: 851
imageHeight: 1024

fitWidth: 3000
fitHeight: 100

Expected : 83/100 (width/height)
Actual   : 84/100 (width/height) 

So ceil only for fitWidth will crash some vertical images. Looks like we need to change algorithm.
Or do something like this 😂
fitWidth -= 0.2

There is a problem that we can't have "half of the pixel", so i think there are 2 ways:

  1. Do hack like fitWidth -= 0.2
  2. Make image a little bit bigger where dimension is the best fitted (where Round works fine)

@Dynom
Copy link
Collaborator

Dynom commented Feb 20, 2019

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.

@evrenkutar
Copy link

@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 pushed a commit to photoslurp/imaginary that referenced this pull request Jul 24, 2019
@Dynom
Copy link
Collaborator

Dynom commented Aug 6, 2019

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

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

Successfully merging this pull request may close these issues.

None yet

3 participants