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

wip: fix: resize with interpolationType nearest #453

Closed
wants to merge 11 commits into from

Conversation

tpoisseau
Copy link
Contributor

Closes: #452

@tpoisseau tpoisseau linked an issue Apr 26, 2024 that may be closed by this pull request
@targos
Copy link
Member

targos commented Apr 29, 2024

@stropitek wdyt? Maybe we should remove the custom code and use transform with a scaling matrix?

@stropitek
Copy link
Contributor

stropitek commented Apr 29, 2024

I think it would make sense to use transform to have a consistent implementation of methods like resize, cropRectangle, rotate which all just iterate over a target image, calculate a position on the source image and then interpolate the value.

@tpoisseau
Copy link
Contributor Author

I would like to try to use transform matrix instead but I don't know how it work

@targos
Copy link
Member

targos commented Apr 30, 2024

https://en.wikipedia.org/wiki/Scaling_(geometry)

With a 2x3 matrix, it should be

[
  [ factor   0    0 ]
  [   0    factor 0 ]
]

@tpoisseau
Copy link
Contributor Author

It's the best I can do.
Nearest is working.
Bilinear is very close from the opencv result.

I let you review before resetting snapshots to let the tests pass.

@tpoisseau
Copy link
Contributor Author

tpoisseau commented Apr 30, 2024

Bilinear:

PR OpenCV main
result resize_bilinear testResizeBilinear resize_bilinear
substract resize_bilinear_substraction resize_bilinear_substraction

Nearest:

PR OpenCV main
result resize_nearest testResizeNearest resize_nearest
substract resize_nearest_substraction resize_nearest_substraction

@targos
Copy link
Member

targos commented Apr 30, 2024

Do we still need custom implementation of the interpolation functions?

@targos
Copy link
Member

targos commented Apr 30, 2024

Bilinear is known to give different result compared to OpenCV. Solution is to add a margin of error with comment like here:

// OpenCV bilinear interpolation is less precise for speed.
expect(transformed).toMatchImage('opencv/testAffineTransform.png', {
error: 3,
});

Use the smallest possible error margin

@tpoisseau
Copy link
Contributor Author

Do we still need custom implementation of the interpolation functions?

I removed It because I do not use it anymore.

Bilinear is known to give different result compared to OpenCV.

Ok I thought we should be one to one with OpenCV (And I found his less precise algorithm more smoother).


If you agree with theses code changes I think we can reset snapshots and merge

@targos
Copy link
Member

targos commented Apr 30, 2024

I found his less precise algorithm more smoother

Really? The difference is barely visible in the other tests (you have to quickly change from one image to the other to see that the colors change, but they are basically the same).

src/utils/interpolatePixel.ts Outdated Show resolved Hide resolved
src/utils/interpolatePixel.ts Show resolved Hide resolved
@tpoisseau
Copy link
Contributor Author

tpoisseau commented Apr 30, 2024

The difference is barely visible in the other tests

I did not check other tests, but take a lot of time to tweak trying to get result most similar result with opencv.
Comparing result with substraction, zoom-in and pixel grid ^^'

@tpoisseau
Copy link
Contributor Author

After snaptshots reset, some tests did not pass yet

@targos
Copy link
Member

targos commented Apr 30, 2024

There are still substantial changes to interpolateNeighbourBilinear which may explain it.

@tpoisseau
Copy link
Contributor Author

Yes, I'll adapt tests data

@targos
Copy link
Member

targos commented May 2, 2024

But are you sure about the change to the algorithm? It worked fine until now.

@tpoisseau
Copy link
Contributor Author

I'm not sure.
I found bilinear for the small image better and more similar to opencv. but for largeur image, black and white (like letters with rotation and matching) it seems more blurry. rotation seems to produce some color shifting with triangles.

Maybe I should rollback the bilinear but keep nearest.

const {
interpolationType = 'bilinear',
borderType = 'constant',
borderType = interpolationType === 'bilinear' ? 'replicate' : 'constant',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Is it OpenCV behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know but bilinear was better with replicate by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably not the right way to fix it.

Comment on lines +42 to +43
// OpenCV bilinear interpolation is less precise for speed.
expect(resized).toMatchImage('opencv/testResizeBilinear.png', { error: 25 });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error: 25 is too much (that's 10% of the range of values 0-255)

tpoisseau added a commit that referenced this pull request May 2, 2024
chirurgical fix, rework of #453

Closes: #452
@tpoisseau tpoisseau mentioned this pull request May 2, 2024
tpoisseau added a commit that referenced this pull request May 2, 2024
chirurgical fix, rework of #453

Refs: #452
@tpoisseau
Copy link
Contributor Author

This PR was in a dead end I preferred to restart from scratch and only fix nearest resize.
#454

This PR will be usefull for future as exploration of this topic.

@tpoisseau tpoisseau closed this May 2, 2024
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.

resize method doesn't work properly
3 participants