Add read/write functions for features2d and normalize naming convention#20367
Add read/write functions for features2d and normalize naming convention#20367asmorkalov merged 3 commits intoopencv:4.xfrom
Conversation
|
@augustinmanecy, thank you for the patch! We appreciate the new functionality that you've added. However, the pull request does not pass our tests. Can you please retain the original names and also make the added read() & write() methods non-virtual? |
|
@vpisarev Ok I'm working on java test corrections, as suggested by @berak in the corresponding opencv_contrib PR and will commit a fix soon. |
|
FASTFeatureDetectorTest and ORBDescriptorExtractorTest in Java read Detector configuration from file. Please check changes there. |
|
Hi, I just pushed a new commit which fix Java tests. I also added additionnal check in read methods in order to avoid erasing current value of parameters which are not in the configuration file (if a paramater was missing in the configuration file, it was replaced by 0). |
|
Ok, the tests for BRIEF, STAR and SURF (from xfeatures2d) have failed because it was not build with my own opencv_contrib branch: (last commit of augustinmanecy/opencv_contrib referenced as PR opencv/opencv_contrib#2997) |
|
@augustinmanecy You need to create branch with the same name in contrib/extra and propose PRs there. PRs with the same branch name are tested together. |
3bf9cf4 to
f2b406e
Compare
|
Tests failed for windows 10 but the failling tests are relevant to opencv_test_videoio / opencv_test_videoio.videoio/videocapture_acceleration.read... I don't think this is due to my changes, what to do now? |
|
ignore |
|
Ok, so does it mean this PR can be merged or is there something else to do? |
modules/features2d/misc/java/test/SURFDescriptorExtractorTest.java
Outdated
Show resolved
Hide resolved
modules/features2d/misc/java/test/SURFDescriptorExtractorTest.java
Outdated
Show resolved
Hide resolved
54f234c to
574a6c8
Compare
|
I updated tests to check read parameters and I also added some missing tests (BRISK, FREAK, etc.). All builds are successful for me but the pipeline failed because I didn't created a fork for opencv_extra with branch features2d-rw, as I did not modify this part... Then I forked it now and created the features2d-rw branch. (I do not understand why previous builds succeed if this fork was mandatory...) |
|
I re-triggered build on pullrequest.opencv.org. |
3d4e9bf to
fae6831
Compare
|
Build for iOS failed due to inconsistence between naming conventions in generated codes. The following errors are due to the fact that the parameter structure for cv::SimpleBlobDetector::Params is not always defined using the same convention in different files:
I would suggest to generate this structure as cv::SimpleBlobDetector_Params in _build/precommit-contrib_ios/build/build_ios_contrib/build/build-armv7-iphoneos/modules/objc_bindings_generator/ios/gen/objc/features2d/SimpleBlobDetectorParams.h:28_to remains coherent with naming convention chosen in Java interface. Here is the compilation error obtained on the forge: |
|
I'm not sure how to manage "Compare ABI dumps" fails during build... These are due to new pure methods I added for getters/setters in some features2d base class. This getters/setters are implemented in there XXX_Impl derived class and are needed for Java tests... I can make this methods not pure (to not break derived class implementation based on previous version), as it is done for example with BRISK.: /** @brief Set detection threshold.
@param threshold AGAST detection threshold score.
*/
CV_WRAP virtual void setThreshold(int threshold) { CV_UNUSED(threshold); return; }
CV_WRAP virtual int getThreshold() const { return -1; }=>The problem with this solution, is that this is not the initial implementation chosen for base class of other features2d... Any suggestions? |
So, what should I do for this point? Dummy core as done in BRISK, change nothing, or something else? |
aab9d78 to
0d0f08b
Compare
|
Build for OpenCV CN Ubuntu 18.04 and 20.04 failed in the forge because DBUILD_opencv_xfeatures2d is set to OFF: But this define need to be set to ON to build features2d Java Tests (see below)... |
|
Hi, According to last build, there are 3 issues I can't solve without some help or guidelines:
@asmorkalov can you help me with this? |
asmorkalov
left a comment
There was a problem hiding this comment.
Hello @Palmitoxico Thanks a lot for hard work! The PR looks good to me in general. There are several things that can simplify things and improve the code:
- No need to cast floating point values, you can just add type literal like this (float)0.2 -> 0.2f.
- It make sense to use common base class with "not implemented" exceptions and inherit test classes from it.
- Please do not use (double)(float) casting. JUnit supports assertEqual for floats with epsilon
- write + writeYml test does not give much to test coverage. cv::FileStorage is used internally everywhere, so only one test case is enough.
modules/features2d/misc/java/test/AKAZEDescriptorExtractorTest.java
Outdated
Show resolved
Hide resolved
modules/features2d/misc/java/test/AKAZEDescriptorExtractorTest.java
Outdated
Show resolved
Hide resolved
| CV_WRAP virtual void setParams( SimpleBlobDetector::Params params ) { CV_UNUSED(params); return; } | ||
| CV_WRAP virtual SimpleBlobDetector::Params getParams() { return SimpleBlobDetector::Params(); } |
There was a problem hiding this comment.
The methods look confusing. Please remove them if they cannot implement parameters management.
There was a problem hiding this comment.
OK, I make them pure abstract methods, and let the derived class SimpleBlobDetectorImpl do the implementation in blobdetector.cpp
=> Concerning your suggestion (which is the same thing) about using exceptions in base class for new getters/setters, it could be for exemple for SIFT:
CV_WRAP virtual void setNFeatures(int maxFeatures) { CV_Error(Error::StsNotImplemented, "The method is not implemented, this must be done by derived classes."); }
CV_WRAP virtual int getNFeatures() const { CV_Error(Error::StsNotImplemented, "The method is not implemented, this must be done by derived classes."); }However I'm not sure this is the good choice. Indeed, I initially added cores to the new setters/getters to workaround the "Compare ABI dumps" fails during build. But it should be pure abstract methods because adding an implementation with exception in base class will leads to runtime exceptions if derived class does not overload theses methods...
So, I would prefer to make them pure abstract to yield to compilation errors if derived class does not implement them, but this will trigger "Compare ABI dumps" fails during build. So for the same SIFT example, I would prefer to use:
CV_WRAP virtual void setNFeatures(int maxFeatures) = 0;
CV_WRAP virtual int getNFeatures() const = 0;@asmorkalov is it possible to keep add new getters/setters as abstract methods and ignore "Compare ABI dumps" fails when merging the PR?
|
@asmorkalov I think you mean @augustinmanecy ? |
|
Thanks a lot! Sure. |
|
@asmorkalov |
|
I could help to complete this PR if needed. BTW what is your opinion about using only |
|
@sturkmen72, yes with pleasure, I don't really know what is missing now to complete this PR. |
7b72f6e to
43caf02
Compare
96d7b6d to
4fd8faa
Compare
8a18503 to
f9f7c6a
Compare
Update Java tests accrording to new features2d read/write functions. In read function, check before if node is empty to avoid erasing default value in case of missing parameters. fix whitespace Add check of read parameters in Java Tests. => add getters/setters to complete cpp/java/python API (needed for these tests.) Add new tests for AGAST, AKAZE, BEBLID, BRISK, DAISY, FREAK, KAZE, LATCH, LUCID and MSD features2d. fix warning override for gftt Remove warning for conversion from double to foat for akaze, and fix doc of BRISK Make new getters/setters not pure virtual method in base class of fetaures2d to pass "Compare ABI dumps" tests. revert getter/setter of AKAZE threshold parameter to double instead of double to ensure compatibility with previous version revert getters/setters of KAZE from float to double to ensure compatibility with previous version fix suspicious cast and use f suffix in JAVA tests to force assetEquals overload. Add valid() method to check parameters' validity of simpleBlob detector. Remove xfeatures2d tests and enpty test suites. Restored doubles in API. Reimplemented SimpleBlobDetector input validation. Resotred broken feature2d java tests. Exlcude SimpleBlobDetector::Params::blobColor from test for now as it cannot be automatically wrapped to java. Re-implemented new java tests to handle non-free part too. Got rid of double->text->double conversion issue in KAZE anr AKAZE tests.
f9f7c6a to
20a0562
Compare
|
@alalek Could you take a look again? Known issues:
Both of the issues coulld be solved by separate PR. Please add supressions to ABI checker. In case if it looks ok, I'll squash commits and merge the PR. |
|
@alalek Could you take a look on this? |
| String filename = OpenCVTestRunner.getTempFileName("xml"); | ||
|
|
||
| writeFile(filename, "%YAML:1.0\n---\nthreshold: 130\nnonmaxSuppression: 1\n"); | ||
| writeFile(filename, "<?xml version=\"1.0\"?>\n<opencv_storage>\n<name>Feature2D.FastFeatureDetector</name>\n<threshold>10</threshold>\n<nonmaxSuppression>1</nonmaxSuppression>\n<type>2</type>\n</opencv_storage>\n"); |
There was a problem hiding this comment.
Why is test code changed?
Are old configurations become broken?
There was a problem hiding this comment.
The PR adds adds more parameters to saved configuration. The YAML->XML replacement looks strange, but the line changes anyway. I decided not to rework original solution.
modules/features2d/misc/java/test/ORBDescriptorExtractorTest.java
Outdated
Show resolved
Hide resolved
| CV_WRAP static Ptr<SimpleBlobDetector> | ||
| create(const SimpleBlobDetector::Params ¶meters = SimpleBlobDetector::Params()); | ||
|
|
||
| CV_WRAP virtual void setParams(const SimpleBlobDetector::Params& params ) = 0; |
There was a problem hiding this comment.
Do we really need setParams() if we already have ctor/create?
Especially "virtual".
| "ManualFuncs" : { | ||
| "SimpleBlobDetector": { | ||
| "setParams": { "declaration" : [""], "implementation" : [""] }, | ||
| "getParams": { "declaration" : [""], "implementation" : [""] } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Yes. I checked objc code generation and this block just disables broken bindings.
There was a problem hiding this comment.
Perhaps disabling/ignoring should be done more obviously, e.g. through class_ignore_list approach (but for methods).
There was a problem hiding this comment.
It'll be replaced with appropriate solution with another PR.
…t's done incorrectly.
741e41c to
ee47082
Compare
|
@alalek Can we merge this? |
|
There is one build warning left on Win64 (MSVS 2015) configuration. |
|
@alalek ready for merge. |
|
@alalek may I merge the patch? |
| if (p.thresholdStep <= 0) | ||
| CV_Error(Error::StsBadArg, "thresholdStep>0"); | ||
|
|
||
| if (p.minThreshold > p.maxThreshold || p.minThreshold <= 0) |
There was a problem hiding this comment.
@augustinmanecy Should that be p.minThreshold < 0?
Some of my own internal tests are now failing against OpenCV master, and I think this is why.
**Merge with contrib**: opencv/opencv_contrib#3003 ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] 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 - [x] The PR is proposed to proper branch - [ ] There is reference to original bug report and related work - [x] 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
Merge with contrib: opencv/opencv_contrib#3003
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.