fbs disparity filtering fix #1877
Conversation
modules/ximgproc/src/fbs_filter.cpp
Outdated
| uint16_t *pftar = (uint16_t*) output.data; | ||
| for (int i = 0; i < int(splat_idx.size()); i++) | ||
| { | ||
| pftar[i] = uint16_t(y(splat_idx[i]) * 65535.0f); |
There was a problem hiding this comment.
Please use cv::saturate_cast<ushort>(...) here and below.
modules/ximgproc/src/fbs_filter.cpp
Outdated
| xw = x.mul(w); | ||
| cv::ximgproc::fastGlobalSmootherFilter(guide, xw, filtered_xw, fgs_spatialSigma, fgs_colorSigma); | ||
| cv::ximgproc::fastGlobalSmootherFilter(guide, w, filtered_w, fgs_spatialSigma, fgs_colorSigma); | ||
| cv::divide(filtered_xw, filtered_w, filtered_disp, 1.0f, CV_32FC1); | ||
| cv::divide(filtered_xw, filtered_w+EPS, filtered_disp, 1.0f, CV_32FC1); |
There was a problem hiding this comment.
Please add some simple regression test to for this issue into test_fbs.cpp file.
New test images can be added into opencv_extra/testdata (but prefer to reuse existed).
Is it related for master branch only? Or 3.4 too? OpenCV handles division by zero differently for floating-point numbers (3.4 has zero pre-check, master uses IEEE 754 rules).
There was a problem hiding this comment.
This was related to master branch only as I was originally working with 3.4 and noticed the issue while switching to master. Like you pointed out, I had already a suspicion that the 3.4 handles division by zero differently but didn't check this.
Would it be better to remove the Fast Global Filtering because it is not used in the original paper for initializing the Fast Bilateral Solver? In fbs_filter.cpp, the parameters for FGS are also fixed. I would prefer to remove the FGS part and maybe add it to disparity filtering demo for comparison.
In the case, you think its better to keep the FGS in fbs_filter.cpp, I can add the requested test.
There was a problem hiding this comment.
it is not used in the original paper
It make sense to remove that.
From user/applications side it is more easy to add pre-filter than to remove extra stage.
@berak Do you have any objections?
There was a problem hiding this comment.
BTW, This code is not a part of any OpenCV release yet - you can work on 3.4 branch (3.4.4 expected on this week).
There was a problem hiding this comment.
no objections, wise decision ;)
There was a problem hiding this comment.
In its present state, the fbs filter expects float confidence map values to be in [0,255] range like in disparity_filters.cpp. Would it be better to change the float confidence values to [0,1] range instead?
Besides this issue my revision should be complete.
There was a problem hiding this comment.
float confidence values to [0,1] range
Sounds good to me.
There was a problem hiding this comment.
Now the FGS (i.e. WLS) filtering is moved from fbs_filter.cpp to disparity_filtering.cpp as a preprocessing option. Consequently, there is no division by zero issue even between master and 3.4 branches.
There was a problem hiding this comment.
@alalek I added the fixed FBS filter to 3.4 branch (hopefully wasn't too late for 3.4.4).
There was a problem hiding this comment.
Please add some simple regression test to for this issue into test_fbs.cpp file.
New test images can be added into opencv_extra/testdata (but prefer to reuse existed).
I checked the test_fbs.cpp as well. It is comparing the output to a reference image produced with a different filter (FGS) with a very loose error threshold. I updated the test with a new reference image produced with the same filter and will commit it soon.
This pullrequest changes
int16disparity imagesfbs_filter.cpptodisparity_filtering.cppdemo code