CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
Reworked findContours to reduce C-API usage #25146
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
Conversation
24b2a2e
to
82f0b1d
Compare
@asmorkalov , I've addressed some of the comments, but still not sure what to do with others:
|
2609d74
to
ae87c6e
Compare
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.
π
|
|
modules/imgproc/src/contours_new.cpp
Outdated
static const int MASK_FLAGS = 0xC0000000; // right + new | ||
static const int MASK_VAL = 0x3FFFFFFF; // ~flags - pixel label | ||
static const int MASK_VAL = 0x3FFFFFFF; // ~flags - pixel label |
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.
Is there clang-format option 'do not touch'?
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.
We can enclosure a block of code between // clang-format on
- // clang-format off
comments to ignore it.
But I believe in general it is better to avoid alignment of values or comments in such blocks to simplify future merges. Thus I chose to disable these rules: AlignConsecutiveAssignments: None
and other AlignConsecutive*
options (see https://releases.llvm.org/12.0.0/tools/clang/docs/ClangFormatStyleOptions.html for details)
- added new tests for findContours - rewritten findContours and TC89 approximation without using C-API data structures - enabled chain code output (method=0) - moved link runs algorithm to separate function (does not copy image, limited hierarchy) - temporarily added findContours_old function to test accuracy
Would also be nice to have 4 connected and 8 connected contour detection... |
Reworked findContours to reduce C-API usage opencv#25146 What is done: * rewritten `findContours` and `icvApproximateChainTC89` using C++ data structures * extracted LINK_RUNS mode to separate new public functions - `findContoursLinkRuns` (it uses completely different algorithm) * ~added new public `cv::approximateChainTC89`~ - **:x: decided to hide it** * enabled chain code output (method = 0, no public enum value for this in C++ yet) * kept old function as `findContours_old` (exported, but not exposed to user) * added more tests for findContours (`test_contours_new.cpp`), some tests compare results of old function with new one. Following tests have been added: * contours of random rectangle * contours of many small (1-2px) blobs * contours of random noise * backport of old accuracy test * separate test for LINK RUNS variant What is left to be done (can be done now or later): * improve tests: * some tests have limited verification (e.g. only verify contour sizes) * perhaps reference data can be collected and stored * maybe more test variants can be added (?) * add enum value for chain code output and a method of returning starting points (e.g. first 8 elements of returned `vector<uchar>` can represent 2 int point coordinates) * add documentation for new functions - **:heavy_check_mark: DONE** * check and improve performance (my experiment showed 0.7x-1.1x some time ago) * remove old functions completely (?) * change contour return order (BFS) or allow to select it (?) * return result tree as-is (?) (new data structures should be exposed, bindings should adapt)
Reworked findContours to reduce C-API usage opencv#25146 What is done: * rewritten `findContours` and `icvApproximateChainTC89` using C++ data structures * extracted LINK_RUNS mode to separate new public functions - `findContoursLinkRuns` (it uses completely different algorithm) * ~added new public `cv::approximateChainTC89`~ - **:x: decided to hide it** * enabled chain code output (method = 0, no public enum value for this in C++ yet) * kept old function as `findContours_old` (exported, but not exposed to user) * added more tests for findContours (`test_contours_new.cpp`), some tests compare results of old function with new one. Following tests have been added: * contours of random rectangle * contours of many small (1-2px) blobs * contours of random noise * backport of old accuracy test * separate test for LINK RUNS variant What is left to be done (can be done now or later): * improve tests: * some tests have limited verification (e.g. only verify contour sizes) * perhaps reference data can be collected and stored * maybe more test variants can be added (?) * add enum value for chain code output and a method of returning starting points (e.g. first 8 elements of returned `vector<uchar>` can represent 2 int point coordinates) * add documentation for new functions - **:heavy_check_mark: DONE** * check and improve performance (my experiment showed 0.7x-1.1x some time ago) * remove old functions completely (?) * change contour return order (BFS) or allow to select it (?) * return result tree as-is (?) (new data structures should be exposed, bindings should adapt)
lower_run = rns[rns[lower_run].next].next; | ||
continue; | ||
} | ||
rns[rns[lower_run].next] = rns[rns[lower_run].next]; |
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.
@mshabunin , why assign it to itself? Is it a bug?
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.
Yes, this is a problem, but not serious - there should be no operation, like in the original code:
opencv/modules/imgproc/src/contours.cpp
Lines 1636 to 1664 in f4ebf7c
for( ; n < lower_total/2; n++ ) | |
{ | |
if( connect_flag != ICV_SINGLE ) | |
{ | |
prev_point->link = lower_run->next; | |
connect_flag = ICV_SINGLE; | |
lower_run = lower_run->next->next; | |
continue; | |
} | |
lower_run->link = lower_run->next; | |
//First point of contour | |
CV_WRITE_SEQ_ELEM( lower_run, writer_ext ); | |
lower_run = lower_run->next->next; | |
} | |
for( ; k < upper_total/2; k++ ) | |
{ | |
if( connect_flag != ICV_SINGLE ) | |
{ | |
upper_run->next->link = prev_point; | |
connect_flag = ICV_SINGLE; | |
upper_run = upper_run->next->next; | |
continue; | |
} | |
upper_run->next->link = upper_run; | |
upper_run = upper_run->next->next; | |
} |
I'll fix it soon.
Reworked findContours to reduce C-API usage opencv#25146 What is done: * rewritten `findContours` and `icvApproximateChainTC89` using C++ data structures * extracted LINK_RUNS mode to separate new public functions - `findContoursLinkRuns` (it uses completely different algorithm) * ~added new public `cv::approximateChainTC89`~ - **:x: decided to hide it** * enabled chain code output (method = 0, no public enum value for this in C++ yet) * kept old function as `findContours_old` (exported, but not exposed to user) * added more tests for findContours (`test_contours_new.cpp`), some tests compare results of old function with new one. Following tests have been added: * contours of random rectangle * contours of many small (1-2px) blobs * contours of random noise * backport of old accuracy test * separate test for LINK RUNS variant What is left to be done (can be done now or later): * improve tests: * some tests have limited verification (e.g. only verify contour sizes) * perhaps reference data can be collected and stored * maybe more test variants can be added (?) * add enum value for chain code output and a method of returning starting points (e.g. first 8 elements of returned `vector<uchar>` can represent 2 int point coordinates) * add documentation for new functions - **:heavy_check_mark: DONE** * check and improve performance (my experiment showed 0.7x-1.1x some time ago) * remove old functions completely (?) * change contour return order (BFS) or allow to select it (?) * return result tree as-is (?) (new data structures should be exposed, bindings should adapt)
What is done:
findContours
andicvApproximateChainTC89
using C++ data structuresfindContoursLinkRuns
(it uses completely different algorithm)added new public- β decided to hide itcv::approximateChainTC89
findContours_old
(exported, but not exposed to user)test_contours_new.cpp
), some tests compare results of old function with new one. Following tests have been added:What is left to be done (can be done now or later):
vector<uchar>
can represent 2 int point coordinates)