flanggut / smvs

Shading-aware Multi-view Stereo
BSD 3-Clause "New" or "Revised" License
250 stars 75 forks source link

Possible Bug: vector out of range in fill_values_at_pixels #37

Closed xubury closed 3 years ago

xubury commented 3 years ago

So I try to run this project on windows compiled by MinGW-GCC. But I keep getting a vector out of range error when running DepthOptimizer's optimize() method. (Doesn't have this problem on Linux) So I dig into the function and trace the crash to this functionpatch->fill_values_at_pixels(&pixels, &depths, &depth_derivatives,&depth_2nd_derivatives, &pids, sampling); It is called by jacobian_entries_for_patch in gauss_newton_step.construct step At first I thought it was caused by the rounding in pixels->resize(MATH_POW2(this->size) / MATH_POW2(subsample));. But It is not. Then I just force break the loop.

if (id == pixels->size()) 
{
   break;
}

Altough the outcome doesn't seem to differ much to the one I did on Linux, I am still wondering what could cause this crash?

xubury commented 3 years ago

I ran a simple test with subsample = 2 and size = 15, so the pixels->size() should be 225 / 4 = 56. But the final output is pid: 223 id: 59. So it's maybe a bug? But I didn't get this error on Linux. Weird..

    int subsample = 2;
    int size = 15;
    int pid = 0;
    int id = 0;
    while (pid < size * size) {
        std::cout << "pid: " << pid << " ";
        std::cout << "id: " << id << "\n";
        pid += subsample;
        id += 1;
        if ((pid / size) % (subsample) == 1) pid += size * (subsample - 1);
    }
    std::flush(std::cout);
xubury commented 3 years ago

I suspect https://github.com/flanggut/smvs/issues/36 also crashed here because fill_values_at_pixels try to access out of range vector index.

xubury commented 3 years ago

ok, I found the solution here. After digging into the code, I realize patch size has to be a even value. Because it was calculate by 2^size. fill_values_at_pixels is correct when the patch size is even. But my compiler did a funny thing here, it return 15 calling std::pow(2, 4) which messed out everything. Simliar to the question in stackoverflow. https://stackoverflow.com/questions/15851636/why-is-my-integer-math-with-stdpow-giving-the-wrong-answer Don't know if this issue is related to MinGW-gcc only though. However, there's some closed issues report SF that I think may be caused by this std::pow inaccuracy. https://github.com/flanggut/smvs/issues/14 https://github.com/flanggut/smvs/issues/22 https://github.com/flanggut/smvs/issues/36 Here's my temporary patch:

Index: lib/surface.cc
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- lib/surface.cc  (revision 7cf59084329d494068c67bd57bfeae5660584ad3)
+++ lib/surface.cc  (date 1603953213188)
@@ -16,6 +16,11 @@

 SMVS_NAMESPACE_BEGIN

+constexpr int int_pow(int b, int e)
+{
+    return (e == 0) ? 1 : b * int_pow(b, e - 1);
+}
+
 Surface::Surface (mve::Bundle::ConstPtr bundle, StereoView::Ptr main_view,
     int scale, mve::FloatImage::ConstPtr init_depth)
     : pixel_width(main_view->get_width())
@@ -25,7 +30,7 @@
     int const width = this->pixel_width;
     int const height = this->pixel_height;

-    this->patchsize = std::pow(2, this->scale);
+    this->patchsize = int_pow(2, this->scale);
     this->num_patches_x = (width - 2) / this->patchsize - 1;
     this->num_patches_y = (height - 2) / this->patchsize - 1;
     this->node_stride = this->num_patches_x + 1;
@@ -59,7 +64,7 @@
     int const height = this->pixel_height;

     this->depth = mve::FloatImage::create(width, height, 1);
-    this->patchsize = std::pow(2, this->scale);
+    this->patchsize = int_pow(2, this->scale);
     this->num_patches_x = (width - 2) / this->patchsize;
     this->num_patches_y = (height - 2) / this->patchsize;
     this->node_stride = this->num_patches_x + 1;
@@ -984,7 +989,7 @@
 Surface::subdivide_patches (void)
 {
     this->scale = this->scale - 1;
-    this->patchsize = std::pow(2, this->scale);
+    this->patchsize = int_pow(2, this->scale);

     int new_num_patches_x = (this->pixel_width - 2) / this->patchsize;
     int new_num_patches_y = (this->pixel_height - 2) / this->patchsize;
xubury commented 3 years ago

this->patchsize = std::ceil(std::pow(2, this->scale)); also works too. Just tested on Ubuntu 20.04 gcc 9.3.0 and windows 10 mingw-gcc 10.2.0

flanggut commented 3 years ago

Thanks for looking into this! Looks like it is indeed a floating point issue. Considering we're only ever looking for powers of 2

this->patchsize = 1 << this->scale;

would probably be the best fix. I'll prepare a patch.

flanggut commented 3 years ago

4f1e28b8a444cedd7040f08c92b7a0960e0efd84 should fix the issue.

xubury commented 3 years ago

4f1e28b should fix the issue.

yes, it works great now. Thanks!