Conversation
|
@vpisarev Could you look at it? |
|
@Yosshi999, thank you for the patch! |
| @param sigma The sigma of the Gaussian applied to the input image at the octave \#0. If your image | ||
| is captured with a weak camera with soft lenses, you might want to reduce the number. | ||
|
|
||
| @param useUcharDescriptors If true, the type of descriptors will be CV_8U. If false (default), it |
There was a problem hiding this comment.
Could you please update the parameter description as well
| return makePtr<SIFT_Impl>(_nfeatures, _nOctaveLayers, _contrastThreshold, _edgeThreshold, _sigma); | ||
|
|
||
| CV_Assert(_descriptorType == CV_32F || _descriptorType == CV_8U); | ||
| bool useUcharDescriptors = (_descriptorType == CV_8U); |
There was a problem hiding this comment.
IMO it make sense to propagate the type info deeper to SIFT_Impl itself to simplify descriptorType() and allocation of _descriptors. I also suggest providing a comment why _descriptorType is restricted to 8U and 32F since it's not obvious here.
|
@vpisarev The build is failed due to ABI change introduced to |
| CV_WRAP static Ptr<SIFT> create(int nfeatures = 0, int nOctaveLayers = 3, | ||
| double contrastThreshold = 0.04, double edgeThreshold = 10, | ||
| double sigma = 1.6); | ||
| double sigma = 1.6, int descriptorType = CV_32F); |
There was a problem hiding this comment.
Could you please add one more create function instead. If there are two create functions: one with the previous set of parameters and one with extended set of parameters(without default values so all of them are mandatory) it should solve ABI compatibility check issue.
By the way there is no ABI compatibility requirement on master branch, so such a change should pass CI tests there.
|
Is 8-bit implementation bit-exact? (with BE resize?) |
|
@alalek No. Its calculation part is same as 32-bit implementation's one. |
|
@terfendail Please confirm that bit-exact results are not expected by this patch. |
|
Internal SIFT descriptor implementation was actually developed in accordance with Lowe paper so the result is always integer values in range 0-255. However evaluation is implemented using floating point arithmetic. The purpose of the patch is to allow return of descriptor data in a more reasonable format. Bit-exact implementation of descriptor require essentially more efforts and will be prepared as a separate patch. |
I added an option so that SIFT can return 8-bit descriptors instead of 32-bit floating-point descriptors.
(David Lowe in his paper uses 8-bit descriptors)
These 8-bit descriptors must be equal to 32-bit floating-point ones because the current implementation applies
saturate_cast<uchar>to the descriptors. (The testFeatures2d_SIFT.descriptor_typeensures this.)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.