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

A new constant in adaptivethreshold is created to calculate #4149

Merged
merged 4 commits into from
Aug 5, 2015
Merged

A new constant in adaptivethreshold is created to calculate #4149

merged 4 commits into from
Aug 5, 2015

Conversation

LaurentBerger
Copy link
Contributor

A new constant ADAPTIVE_THRESH_GAUSSIAN_C_FLOAT in function adaptivethreshold is created to calculate gaussianBlur with CV_32F. Hence rouding error are avoided
problem using ADAPTIVE_THRESH_GAUSSIAN_C is described in this post http://answers.opencv.org/question/63304/results-stability-given-by-adaptivethreshold-with-adaptive_thresh_gaussian_c/

gaussianBlur with CV_32F. hence rouding error are avoided
kanster pushed a commit to kanster/opencv that referenced this pull request Jun 23, 2015
@vpisarev
Copy link
Contributor

@LaurentBerger, I think, this is not a good way to solve the problem. Either the accuracy of GaussianBlur, applied to 8u images, should be improved or the existing ADAPTIVE_THRESH_GAUSSIAN_C should be modified to use CV_32F internally.

@vpisarev vpisarev self-assigned this Jun 23, 2015
@LaurentBerger
Copy link
Contributor Author

First sorry for my bad english.
About problem I have post a message on answers.opencv.org. It seems that's not really a problem and there is no recorded bug about this. So I think that may be some people use ADAPTIVE_THRESH_GAUSSIAN_C as it is. Hence I think that add a new constant is better than modify existing code for ADAPTIVE_THRESH_GAUSSIAN_C.
I think that your idea to improve accuracy of GaussianBlur, applied to 8u images will have too much consequence (relative to speed) for people using GaussianBlur knowing this accuracy problem.
I think then "ADAPTIVE_THRESH_GAUSSIAN_C should be modified to use CV_32F internally." it's a better way.
So Must I go on this pull request? If yes I think replace line code for ADAPTIVE_THRESH_GAUSSIAN_C coull be replace with line code of ADAPTIVE_THRESH_GAUSSIAN_C_FLOAT and forget ADAPTIVE_THRESH_GAUSSIAN_C_FLOAT constant.

@vpisarev
Copy link
Contributor

@LaurentBerger, thanks! Basically, I'm ready to merge it. However, it would be very useful if you provide some test case that passes with this fix and fails without it. Otherwise some further changes in the library may break it again

@LaurentBerger
Copy link
Contributor Author

I have made a test program. This program use adaptiveThreshold with ADAPTIVE_THRESH_MEAN_C, ADAPTIVE_THRESH_GAUSSIAN_C with pull request and ADAPTIVE_THRESH_GAUSSIAN_C (old version) with Version control: 3.0.0-beta-1038-gdcf177e-dirty
Platform:
Host: Windows 6.2 AMD64
CMake: 3.3.0-rc1
CMake generator: Visual Studio 12 2013 Win64
CMake build tool: C:/Program Files (x86)/MSBuild/12.0/bin/MSBuild.exe
MSVC: 1800
Windows size ranging from 3 to 55 and program calculates ratio number of white pixel/ number of pixels in image
threshtestfruits. You can download test program and results perso.univ-lemans.fr/~berger/Afsd56/OCV_PR4149.zip

@vpisarev
Copy link
Contributor

@LaurentBerger, thank you! this is quite remarkable! May I ask you to add such a test (with some automatic analysis) to our regression tests, so that the problem can be verified and possibly eliminated later.

@LaurentBerger
Copy link
Contributor Author

Yes rounding errors are well known but such effect is very surprising.
About test you mean something like opencv/modules/imgproc/test/test_thresh.cpp

@vpisarev
Copy link
Contributor

@LaurentBerger, yes, something like that. There is no need to make fullscale test derived from CvArrTest, just a lightweight google-test that, for example, takes a certain image, computes GaussianBlur directly and via floating-point and then checks that the difference is within a certain threshold.

@LaurentBerger
Copy link
Contributor Author

Before starting google-test I have a simple program to understand. you can download at perso.univ-lemans.fr/~berger/Afsd56/OCV_PR4149_v2.zip
This program study gaussianBlur with float or unsigned char result for size window ranging from 3 to 55. Source image is randomize and experience is done 1000 times. Norm (mean of 1000 experience) between float and uchar result is used to plot a curve function of window size.
I think that threshold value for test could be 1.e-03 If you are agree with program and value where can I insert this test?

gaussianblurtestfloatchar

With this threshold value only results with circle are selected when adaptiveThreshold is used

threshfloatchar

@vpisarev
Copy link
Contributor

vpisarev commented Aug 5, 2015

👍

@Ashod
Copy link
Contributor

Ashod commented Aug 8, 2015

Thanks @LaurentBerger for the patch. There is a corner case that I fixed in #5155.

@LaurentBerger LaurentBerger deleted the ThreshGaussianFloat branch November 8, 2015 21:05
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.

4 participants