Skip to content
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

xfeatures2d BriefDescriptorExtractor not implemented #1112

Closed
Cartermel opened this issue Oct 16, 2023 · 4 comments
Closed

xfeatures2d BriefDescriptorExtractor not implemented #1112

Cartermel opened this issue Oct 16, 2023 · 4 comments

Comments

@Cartermel
Copy link
Contributor

The BriefDescriptorExtractor class isn't yet implemented, and looks like it may introduce many breaking changes to implement.

Description

I'd like to use BRIEF for computing descriptors on keypoints received from any keypoint alg (fast, sift, surf etc). I currently have code that does this in C# using emgu cv, where as far as I can tell they leave the vector of keypoints on the c++ side instead of copying like gocv does. The breaking change I'm thinking of is to change the copying of keypoints to leaving the vector on the c++ side, just like a Mat. This would allow usage of those keypoints easily in any descriptor algorithm.

Since that would negatively affect anyone using the current keypoint implementation, we could add them being on the c++ side in addition to the existing? Or copy the keypoints back over to c++ from go, which seems redundant, but would be much easier. Not sure, hopefully someone has a better approach!

@Cartermel
Copy link
Contributor Author

I noticed the keypoints are being copied back over to c++ in a couple of functions:

func DrawKeyPoints(src Mat, keyPoints []KeyPoint, dst *Mat, color color.RGBA, flag DrawMatchesFlag) {

func DrawMatches(img1 Mat, kp1 []KeyPoint, img2 Mat, kp2 []KeyPoint, matches1to2 []DMatch, outImg *Mat, matchColor color.RGBA, singlePointColor color.RGBA, matchesMask []byte, flags DrawMatchesFlag) {

Should be trivial to implement for BRIEF, I was originally worried about the performance impact of copying the keypoints since, for my use case at least, I'll be running this against thousands of images. But I ran some benchmarks and copying the struct between c++ / go is extremely lightweight, which makes sense since the keypoint struct is very small.

Here's some data for anyone curious:
(I only used time.Now() and time.Since() for measuring elapsed, there's probably a better way but I didn't search)

elapsed times are the average of 3 runs rounded to nearest 100ms

5000 iterations:
No copy: 26200ms
Copy:    27800ms
No copy 1600ms faster 
~5.9% difference

10000 iterations:
No copy: 52000ms
Copy:    55400ms
No copy 3400ms faster
~6.3% difference

This was using this function for computing the descriptors using BRIEF and copying from go (my vscode syntax highlighting is broken...) :
image

Where VectorOfKeyPoint is a custom struct that's just a ptr to a vectorcv::KeyPoint for testing. And the c++ implementation was:
image

And the native c++ implementation without copying was this:
image

Where I passed in the same FastFeatureDector from golang. Not shown is the code for creating the BriefDescriptorExtractor, but its the same as all the other feature2d alg initialization.

This is running on a Ryzen 7 5800X.

TLDR

Copying the keypoints from golang for use in BRIEF results in about a 6% time loss (on my minimal testing), but leaves all the existing keypoint implementation intact. Personally I think this is a fine trade off, I'm interested in knowing what everyone thinks!

In the meantime I'll create a PR for BRIEF and copy the golang keypoints, unless someone's currently building this!

@Cartermel
Copy link
Contributor Author

Forgot to add, this was the simple benchmark function:
image

and the native c++ call was basically exactly the same but with the c call.
And it just uses the lenna photo from opencv.

@deadprogram
Copy link
Member

Keeping the same patterns as used by the other wrappers is the best bet. But I see from the PR that you already also came to that same conclusion 😸

Thanks for working on this @Cartermel

@deadprogram
Copy link
Member

Released as part of 0.36 so now closing. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants