| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
Fix rect_nfa (lsd) #23210
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
Fix rect_nfa (lsd) #23210
Conversation
Comparing the nfa function with the function in the binomial_nfa repository (https://github.com/rafael-grompone-von-gioi/binomial_nfa/blob/main/C99/log_binomial_nfa.c#L152), the first log_gamma call is missing.
modules/imgproc/src/lsd.cpp
Outdated
|
|
||
| // function to get the slope of the rectangle for a specific row | ||
| inline double get_slope(cv::Point2d p1, cv::Point2d p2) { | ||
| return (p2.y != p1.y) ? (p2.x - p1.x) / (p2.y - p1.y) : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p2.y != p1.y
This check is not enough to avoid NaN / Inf results of this expression.
: 0;
Why is 0 here?
Both horizontal and vertical lines has the same value from this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alalek, thank you for reviewing the PR.
The main reason for that function is that I moved the old code logic (lines below) to a function. There was a mistake because I forgot that I switched to Point2d, so I changed the check to use integer values instead.
opencv/modules/imgproc/src/lsd.cpp
Lines 1024 to 1032 in cb2052d
| double flstep = (min_y->p.y != leftmost->p.y) ? | |
| (min_y->p.x - leftmost->p.x) / (min_y->p.y - leftmost->p.y) : 0; //first left step | |
| double slstep = (leftmost->p.y != tailp->p.x) ? | |
| (leftmost->p.x - tailp->p.x) / (leftmost->p.y - tailp->p.x) : 0; //second left step | |
| double frstep = (min_y->p.y != rightmost->p.y) ? | |
| (min_y->p.x - rightmost->p.x) / (min_y->p.y - rightmost->p.y) : 0; //first right step | |
| double srstep = (rightmost->p.y != tailp->p.x) ? | |
| (rightmost->p.x - tailp->p.x) / (rightmost->p.y - tailp->p.x) : 0; //second right step |
But the reality is that due to the way the vertices are ordered (the image below shows the possible cases), and the way the 2 regions are defined for the slope calculation, the y value of both points should only be the same when the rectangle is horizontal. In that case, in the calculation for the second left region (vertices 1 and 2), and the first right region (vertices 0 and 3) the y value of the points used for the slope calculation will be the same.
And because of the checks inside the for loop (code below), the slope in those cases will not be used. So, in theory, it does not matter what the slope is when the y value is the same. But actually, returning 0 is a safe option, in case there is a mistake (like the one I just fixed with the left limit check).
opencv/modules/imgproc/src/lsd.cpp
Lines 988 to 996 in cec7cc4
| if(y <= int(ceil(ordered_y[1].y))) | |
| left_limit = get_limit(ordered_y[0], y, flstep); | |
| else | |
| left_limit = get_limit(ordered_y[1], y, slstep); | |
| if(y < int(ceil(ordered_y[3].y))) | |
| right_limit = get_limit(ordered_y[0], y, frstep); | |
| else | |
| right_limit = get_limit(ordered_y[3], y, srstep); |
However, despite all that (sorry for the long response btw), I can make other changes to the function if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thank you for contribution!
Fix rect_nfa (lsd) * Fix missing log_gamma in nfa() Comparing the nfa function with the function in the binomial_nfa repository (https://github.com/rafael-grompone-von-gioi/binomial_nfa/blob/main/C99/log_binomial_nfa.c#L152), the first log_gamma call is missing. * Fix rect_nfa pixel index * Replace std::rotate * Rename tmp to v_tmp * Replace auto and std::min_element * Change slope equality check to int * Fix left limit check
This is a replacement for #23052, where I made some mistakes with the branches at the end.
With the changes to the rect_nfa function, now it checks the same pixels inside the rectangle as the original paper. In the image below I fill a rotated rectangle using the original paper method (blue channel), the old OpenCV method (green channel), and the fixed code (red channel). As you can see, the old calculation had some logic issues.
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.