-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Add BAD descriptor to xfeatures2d module #3277
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
A missing file with pre-computed bad descriptors caused the tests to fail. I added it in a opencv_extra fork and made the necessary pull request. It should be fixed now. I would really appreciate it if you could rerun the GitHub actions. |
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.
Thank you for the great contribution!
Please take a look on comments below.
@@ -28,7 +28,8 @@ namespace xfeatures2d | |||
// Struct containing the 6 parameters that define an Average Box weak-learner | |||
struct ABWLParams | |||
{ | |||
int x1, y1, x2, y2, boxRadius, th; | |||
int x1, y1, x2, y2, boxRadius; | |||
float th; |
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 should keep integer-based computation of original BEBLID.
Could you please add separate struct type with floats? (and change BEBLID computation to use params from template type)
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.
Thank you very much @alalek for your feedback.
You are right. It is a better idea to preserve the original structure for BEBLID. In my last commit, I added a new ABWLParamsFloatTh for the new descriptor and undo the changes to the original ABWLParams.
I also made BEBLID_Impl a templated class that can be used with both structs.
|
||
The descriptor was trained in Liberty split of the UBC datasets \cite winder2007learning . | ||
*/ | ||
class CV_EXPORTS_W BAD : public Feature2D |
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.
BTW, BAD
is not a good name for googling. Could we add here some prefix/suffix?
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.
Done.
After discussing with the other authors, we think that a better name would be TEBLID (Triplet-based Efficient Binary Local Image Descriptor), which makes it clear that it is an improvement over BEBLID and the name is better suited for googling. In any case, in the documentation, we indicate that this descriptor appears as BAD in the paper.
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.
Thank you for the update!
It is able to describe keypoints from any detector just by changing the scale_factor parameter. | ||
TEBLID is as efficient as ORB, BEBLID or BRISK, but the triplet-based training objective selected more | ||
discriminative features that explain the accuracy gain. It is also more compact than BEBLID, | ||
when running the [AKAZE example](https://raw.githubusercontent.com/opencv/opencv/master/samples/cpp/tutorial_code/features2D/AKAZE_match.cpp) |
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.
Could you please use this link? https://github.com/opencv/opencv/blob/4.x/samples/cpp/tutorial_code/features2D/AKAZE_match.cpp
- master -> 4.x
- use GitHub UI instead of raw code (some browsers open "download" UI)
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.
Done!
modules/xfeatures2d/src/beblid.cpp
Outdated
if (n_bits == BEBLID::SIZE_512_BITS) | ||
{ | ||
#include "beblid.p512.hpp" | ||
begin_ptr = (WeakLearnerT *) std::begin(beblid_wl_params_512); | ||
end_ptr = (WeakLearnerT *) std::end(beblid_wl_params_512); |
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.
(WeakLearnerT *)
Using WeakLearnerT = ABWLParamsFloatTh
and BEBLID::SIZE_512_BITS)
is not a valid case and we should emit error.
Probably it makes sense to specialize ctor and/or add wl_params
parameter.
template <class WeakLearnerT>
BEBLID_Impl<WeakLearnerT>::BEBLID_Impl(float scale_factor, int n_bits, std::vector<WeakLearnerT>&& wl_params)
and move parameters vector initialization on upper (caller) level (::create()
calls).
Also these changes would not require special values like 102,103 for enum values.
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.
Thank you very much again for your feedback @alalek .
I implemented your suggestion in my last commit. By doing it we have one extra benefit, the enum with the descriptor size is no longer needed in the constructor. Also, I decided to use a const reference ( const std::vector<WeakLearnerT>& wl_params
) instead of a Rvalue reference because in this way we can keep the weights of the .h files constant and only one copy of the vector is done.
// Pre-trained parameters of TEBLID-256 trained in Liberty data set. 10K triplets are sampled per iteration. Each triplet | ||
// contains an anchor patch, a positive and a negative, selected as the hardest among 256 random negatives. | ||
static const std::vector<ABWLParamsFloatTh> teblid_wl_params_256 = { | ||
{25, 14, 13, 15, 6, 21.65}, {16, 15, 14, 11, 1, 5.65}, {14, 14, 7, 8, 6, 4.95}, |
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.
Could you please add f
suffixes to floating-point numbers?
-21.65
+21.65f
It is necessary to resolve build problem on Windows:
c:\build\precommit-contrib_windows64\4.x\opencv_contrib\modules\xfeatures2d\src\teblid.p512.hpp(187): error C2397: conversion from 'double' to 'float' requires a narrowing conversion (compiling source file C:\build\precommit-contrib_windows64\4.x\opencv_contrib\modules\xfeatures2d\src\beblid.cpp)
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.
Done!
|
||
// Pre-trained parameters of TEBLID-256 trained in Liberty data set. 10K triplets are sampled per iteration. Each triplet | ||
// contains an anchor patch, a positive and a negative, selected as the hardest among 256 random negatives. | ||
static const std::vector<ABWLParamsFloatTh> teblid_wl_params_256 = { |
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.
std::vector
Need to check generated binary code of this initializer. Array should be fine, but std::vector
- doesn't.
Could you please use something like this:
static const ABWLParamsFloatTh teblid_wl_params_256_[] = {...};
static const std::vector<ABWLParamsFloatTh> teblid_wl_params_256(teblid_wl_params_256_, teblid_wl_params_256 + sizeof(teblid_wl_params_256_) / sizeof(teblid_wl_params_256_[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.
Sorry for late reply due to vacations and thanks again for your comments.
I have fixed it in my last commit.
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.
Well done! Thank you for contribution 👍
Thank you @alalek for your support and for maintaining OpenCV! It's a wonderful library, and I am always happy to contribute 😃 |
Add BAD descriptor to xfeatures2d module * Adding new BAD descriptor to xfeatures2d module * Changing BAD name by TEBLID and using int threshold again for BEBLID * Changing link to AKAZE tutorial and moved parameters initialization to ::create() * Adding f suffixes to floating-point parameters and using arrays again
Merge with extra: opencv/opencv_extra#978
Box Average Difference (BAD) is a new descriptor published last year. The method improves efficient binary descriptors such as ORB or BEBLID with a new loss function and optimization method. It outperforms the other binary efficient descriptors in several tasks like image matching, structure from motion, or patch retrieval. It is also compact, being able to outperform BEBLID-512 with only 256 bits. Here we can see a comparison in HPatches dataset from the paper:
Since the implementation is quite similar to the one of BEBLID, the same source code can be used. It is only necessary to add the new pre-trained weights and some minor modifications. The original BAD source can be found here. More details in the paper and the project page.
I tried to make the code clear, tested, and documented. If you find some possible improvements, please just let me know.
Pull Request Readiness Checklist
Patch to opencv_extra has the same branch name.