Updating STAR detector and FREAK descriptor to work with large and/or 16-bit images #1932
Conversation
ghost
assigned 
|
ping? |
|
@seth-planet Could you please fix "CONFLICT (content): Merge conflict in modules/imgproc/src/sumpixels.cpp" and send the PR again? |
Conflicts: modules/imgproc/src/sumpixels.cpp
|
Conflict should be fixed now. pullrequest.opencv.org doesn't look like it has re-run, but otherwise everything is good. |
|
ping? |
|
@vpisarev Vadim, could you please review it? |
|
ping? |
|
echo!! echo! echo. (echo) |
|
hey all, just checking in... |
|
Hi all, just a friendly reminder that it would make our lives easier if we could get this merged now. I'm about to open source my code, and it's a bit of a pain to tell folks to download this specific fork in order for things to work correctly. |
|
@seth-planet Thanks for contribution, but we cannot merge without @vpisarev review. Vadim is quite busy, so let's wait for him. |
|
@seth-planet 10x for PR! unfortunately there are build errors: http://pullrequest.opencv.org |
|
Well, it worked three months ago... |
|
So, I've spent a few hours working on this, and I'm not near finishing. Seeing as I've already performed this merge several times (beyond what's listed here), I think it's fair for me to wait until you're ready to merge. So, please tell me when you all are ready for me to bring this code together. |
|
@seth-planet sorry we cant merge it until you fix all errors |
|
Yes, but if I fix the errors, will you merge it? via iPhone
|
|
@seth-planet I think yes |
Conflicts: modules/features2d/include/opencv2/features2d.hpp modules/features2d/src/freak.cpp modules/features2d/src/stardetector.cpp
|
Latest merge commit should be working fine. Pullrequest.opencv.org isn't picking it up in anything expect the OCL environment yet, though. (It's working fine in that.) |
|
@vpisarev Vadim please have a look - all green |
|
Hallo. |
| @@ -239,13 +239,129 @@ void FREAK::computeImpl( InputArray _image, std::vector<KeyPoint>& keypoints, Ou | |||
|
|
|||
| ((FREAK*)this)->buildPattern(); | |||
|
|
|||
| // Convert to gray if not already | |||
| Mat grayImage = image; | |||
| // if( image.channels() > 1 ) | |||
There was a problem hiding this comment.
Because they were causing one of the tests to fail. It's mentioned in here:
#1654
There was a problem hiding this comment.
So, may be they should be deleted or there should be some notes, why they are still be in request?
There was a problem hiding this comment.
There really wasn't any conclusion (or response) in #1654, so I just commented in order to pass the tests. However, keeping the lines in strikes me as correct behavior. So, the code is currently in logic purgatory.
|
Hi, Seth! Thanks for contributing and your patience. We need to wait for Vadim's approval. In this time could you please add some tests for new supported image data types? |
|
@vpisarev Vadim, please review this request. It waits a long time. |
|
@BelBES Sergei, please review this request. |
|
@seth-planet Lin2 builder has warnings, it's an accuracy issue. |
|
That's interesting. It was working 12 days ago. |
|
Looks like everything is green now. |
|
Still green across the board... |
|
@vpisarev Vadim, please, could you also check this request? |
|
sorry, terribly sorry for delay. the pull request is big, but the changes look pretty reasonable. Let's put it in. 👍 |
|
@seth-planet Finally we've got it! Thank you for the effort! |
A project that I've been working with involves large, 16-bit, images. The current implementations of the STAR feature detector and the FREAK feature descriptor don't support either large or 16-bit images. I've written a set of changes to support both. Much of the work goes into adding more bits to the integral image when appropriate. I'll discuss what needed doing:
In STAR, the method of detecting if the matrix was color didn't make sense. I'm using channels() instead of type() now.
In STAR, he integral image data type needed to be changed for two reasons. If each cell can have the value of 256, the integer type could overflow when the image is greater than 8 megapixels. If you are working with 16-bit imagery imagery instead of 8-bit, this will happen at much smaller image sizes. So, I've used templates to use a 32-bit integer under the normal case, and 64-bit double for large images and/or higher bit depth.
In STAR, SSE2 is only enabled if the data type is the original 32-bit integer. It was my aim to support the new data types (shorts & doubles) without removing the existing SSE usage.
In integral(), in sumpixels.cpp, I added templated support for working with 'short' & 'ushort' precision input. As mentioned before, it's appropriate to use double precision when working with 16-bit images (so that's what I did).
In FREAK, I broke most of computeImpl() into a new function, computeDescriptors(). I did this so I could use templates and thus keep all the data type permutations sane.
In FREAK, SSE2 is only used if the average value is expected to be an 8-bit number. This is the original usage, and it should be just as fast as before. 16-bit data falls back to the scalar code.
In FREAK, meanIntensity() now uses templates.
In FREAK, the return value of meanIntensity() is 'int' instead of 'uchar' for compatibility between possible data types. It should make no speed difference.