search

OpenDroneMap / OpenSfM

Open source Structure from Motion pipeline
GNU Affero General Public License v3.0
19 stars 38 forks source link

Fix DSPSIFT color pick #36

Closed originlake closed 2 months ago

originlake commented 2 months ago

Hi, this PR tries to fix the issue for picking color of detected features.

The previous update https://github.com/OpenDroneMap/OpenSfM/pull/33 seems to make it easier to detect features at the edge. For example, assuming an image sized 1000x1000, vlfeat can produce feature point at [500, 999.65], rounding this coordinate will lead to out of bounds error when picking color with it.

I think this is related to pixel center, vlfeat might use (0.5, 0.5) as the center, maybe a better fix would be minus 0.5 before rounding? Not sure how other feature detectors deal with coordinates related pixel center, no such issue observed from other feature detectors.

pierotofy commented 2 months ago

Hey @originlake thanks for looking into this; have you found a test case (test images) where this error happens? Can you share it/them? I'd be curious to look into it as well.

originlake commented 2 months ago

The datasets I have are under NDA, but let me see if I can pull some reproducible image data or find a public dataset has such issue.

originlake commented 2 months ago

I read the source code of VLFeat. There is no pixel center issue related. In the algorithm, it will check the boundary per octave level, https://github.com/vlfeat/vlfeat/blob/1b9075fc42fe54b42f0e937f8b9a230d8e2c7701/vl/covdet.c#L1312-L1314 and then rescale the points with respect to original image size (no boundary check after this step) https://github.com/vlfeat/vlfeat/blob/1b9075fc42fe54b42f0e937f8b9a230d8e2c7701/vl/covdet.c#L2044-L2045 For example, assuming an image with size w,h

  1. octave level -1, raw range [0, 2*w-1], [0,2*h-1], step=0.5, scaled range [0, w-0.5], [0,h-0.5]
  2. octave level 0, raw range [0, w-1], [0,h-1], step=1, scaled range [0, w-1], [0,h-1]
  3. octave level 1, raw range [0, w/2-1], [0,h/2-1], step=2, scaled range [0, w-2], [0,h-2]
  4. ......

I can tell the issue will only happen at octave level -1 and reaches the max width(w-0.5) or height(h-0.5), which will return w or h after rounding and hence causing error. So I think the clipping solution makes sense https://github.com/OpenDroneMap/OpenSfM/blob/097ef20e4fb29c97a46465ebae1993b6edef5ee5/opensfm/features.py#L636-L638

Let me know your thoughts.

pierotofy commented 2 months ago

I think the clipping solution is probably OK, but I don't fully understand why other detectors don't suffer from this issue.

originlake commented 2 months ago

why other detectors don't suffer from this issue.

From what I can tell from SIFT implementation in OpenCV, it will ignore the keypoints on the border, the width of border is defined to 5 here https://github.com/opencv/opencv/blob/93b607dc72e1d7953b17a58d8fb5f130b05c3d7a/modules/features2d/src/sift.simd.hpp#L92-L93 Orb also calls a function to remove keypoints on the border https://github.com/opencv/opencv/blob/93b607dc72e1d7953b17a58d8fb5f130b05c3d7a/modules/features2d/src/keypoint.cpp#L105-L117

originlake commented 2 months ago

We could implement the same border filter, but I don't know if it has good or bad effect.

pierotofy commented 2 months ago

Ah, interesting! If other detectors ignore keypoints near the border, we should probably do something similar with DSP SIFT.

originlake commented 2 months ago

I implemented the filter to remove border keypoints, also added one additional argument to control the border size, default is 5 as given in OpenCV's SIFT implementation.

I think the most optimized and identical to OpenCV's logic way is to modify the VLfeat source code, but that's kind of out of my scope. Let me know if you plan to go that way, I'll close this PR.

pierotofy commented 2 months ago

Awesome, thank you @originlake ! Modifying VLfeat is probably not needed for the scope of this.