Skip to content

Added edge input feature to fast_line_detector #2716

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

Merged
merged 8 commits into from
Oct 22, 2020

Conversation

tsukada-cs
Copy link
Contributor

Fixed issue #2667 in opencv/opencv_contrib

Added the is_edge option to the detect method in fast_line_detector.

The Fast Line Detector used to use the canny method for edge detection internally, but is_edge=true makes it possible to input the edge image directly. Parameters for the canny method are ignored. The default value is false, so it won't break anyone's code.

This feature will provide users with additional flexibility and versatility in FastLineDetector.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@tsukada-cs tsukada-cs changed the title Feature/fld is edge option Added is_edge option on the detect method in fast_line_detector Oct 16, 2020
*/
CV_WRAP virtual void detect(InputArray _image, OutputArray _lines) = 0;
CV_WRAP virtual void detect(InputArray _image, OutputArray _lines,
bool is_edge = false) = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

detect() is not a right place for algorithm parameters tuning. See createFastLineDetector()

Can we check some canny parameters to handle that without touching API?
For example, canny_aperture_size == 0

Deleted is_edge option from detect().
@tsukada-cs
Copy link
Contributor Author

tsukada-cs commented Oct 19, 2020

@alalek
Thanks for the comment.
I tried changing the internal assert statement and then canny_aperture_size = 0, canny_th1=0, canny_th2=0, etc., but it didn't work as expected.
So I removed the is_edge option of detect() and added the input_edge option to createFastLineDetector().

@tsukada-cs tsukada-cs changed the title Added is_edge option on the detect method in fast_line_detector Added input_edge option on the createFastLineDetector() in fast_line_detector Oct 19, 2020
@alalek
Copy link
Member

alalek commented Oct 19, 2020

but it didn't work as expected

Try to split this assertion check first.

BTW, There is no sense to define canny_* parameters if we don't use that.

@tsukada-cs
Copy link
Contributor Author

tsukada-cs commented Oct 19, 2020

@alalek

Try to split this assertion check first.

I failed to state the situation correctly.
I split the assertion check and tried canny_appature_size=0, but the canny method was applied as a result (The edges of the input image were detected using the canny method).
Also, the documentation says that the ksize of Sobel() must be 1, 3, 5 or 7, so we are not supposed to enter canny_appature_size = 0 (ksize=0).

BTW, There is no sense to define canny_* parameters if we don't use that.

I agree.
Should we change the implementation?

@alalek
Copy link
Member

alalek commented Oct 19, 2020

and tried canny_appature_size=0, but the canny method was applied as a result

You should add this check before Canny() call to bypass it.

Removed the input_edge option from createFastLineDetector().
@tsukada-cs
Copy link
Contributor Author

@alalek

Thanks for the kind comments.
I changed the implementation to use the input image as an edge image when canny_aperture_size=0.

@tsukada-cs tsukada-cs changed the title Added input_edge option on the createFastLineDetector() in fast_line_detector Added edge input feature to fast_line_detector Oct 20, 2020
@@ -544,7 +545,8 @@ void FastLineDetectorImpl::lineDetection(const Mat& src, std::vector<SEGMENT>& s
std::vector<Point2i> points;
std::vector<SEGMENT> segments, segments_tmp;
Mat canny;
Canny(src, canny, canny_th1, canny_th2, canny_aperture_size);
if (canny_aperture_size == 0) canny = src;
else Canny(src, canny, canny_th1, canny_th2, canny_aperture_size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using dedicated lines for if/else bodies.
It is necessary to see real picture from "code coverage" line-based reports (example of weekly report).

* @param _do_merge false - If true, incremental merging of segments
will be perfomred
* will be perfomred
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be performed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out. Fixed it.

@tsukada-cs
Copy link
Contributor Author

@alalek

Consider using dedicated lines for if/else bodies.
It is necessary to see real picture from "code coverage" line-based reports (example of weekly report).

I tried, but is this what you mean?

Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! Thank you 👍

@alalek alalek merged commit d8197c6 into opencv:master Oct 22, 2020
@tsukada-cs
Copy link
Contributor Author

Thank you for accepting it. Thanks for all the kind comments.

@tsukada-cs tsukada-cs deleted the feature/fld-is_edge-option branch October 22, 2020 23:24
@alalek alalek mentioned this pull request Nov 27, 2020
@sturkmen72 sturkmen72 mentioned this pull request Apr 6, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants