image-rs / imageproc

Image processing operations
MIT License
736 stars 144 forks source link

`Laplacian` is incorrect #585

Closed cospectrum closed 4 months ago

cospectrum commented 4 months ago

The actual Laplacian can have negative values, and for a GrayImage the max value can be much higher than 255. But the merged version https://github.com/image-rs/imageproc/pull/516 doesn't care about it. So I want to remove the detector and at least fix the filter. Also approximation of Laplacian is not documented, and OpenCV uses [0, 1, 0, 1, -4, 1, 0, 1, 0] by default.

thecodergus commented 4 months ago

I read your points, reviewed the literature I consulted a year ago (Digital Image Processing, Global Edition - Rafael C. Gonzalez and Richard E. Woods) and I say that it is correct as it is. The default value of OpenCV is the adjacency transform, if you read my previous constructor, the use of [0, 1, 0, 1, -4, 1, 0, 1, 0] was present as an optional situation (The result is "dirtier"), if OpenCV uses it as default, it was a decision of the developers, in the literature this is a secondary formula. As for what you mentioned about the real Laplacian being able to have negative values, I didn't understand your point very well, in a GrayImage the resulting value will never be greater than 255 or negative. ovo1_laplace2

cospectrum commented 4 months ago

In GrayImage they will not be negative or larger than 255, since in this format the pixels are stored as u8. But it doesn't mean that values are correct.

cospectrum commented 4 months ago

If the input values are in [0, 255], then after Laplacian (with [0, 1, 0, 1, -4, 1, 0, 1, 0] kernel) they can be in [-1020, 1020].

cospectrum commented 4 months ago

See horizontal_sobel, for example. The input is GrayImage, but the output is Image<Luma<i16>> to make sure we don't lose the value.

thecodergus commented 4 months ago

See horizontal_sobel, for example. The input is GrayImage, but the output is Image<Luma<i16>> to make sure we don't lose the value.

I understand your point, but for what the algorithm proposes and does is correct, no information will be lost

cospectrum commented 4 months ago

Do you think laplacian_filter is correct? It's definitely not. Because I have variance of laplacian and laplacian of gaussian in the production, and current implementation from imageproc master just not gonna work for me, so I will continue to use my own implementation, which does not lose value.

cospectrum commented 4 months ago

https://docs.opencv.org/4.9.0/d5/db5/tutorial_laplace_operator.html

laplace
thecodergus commented 4 months ago

https://docs.opencv.org/4.9.0/d5/db5/tutorial_laplace_operator.html

laplace

I understand, I removed my suggestion about this, the only point that really bothered me was the removal of the function