From 2fa6d144491344775bf010e183db6387392bcc21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niklas=20Hamb=C3=BCchen?= Date: Mon, 14 Oct 2019 19:43:23 +0100 Subject: [PATCH 1/3] cmake: Fix fixing parallel builds for subprojects The direct call to `make` in `BUILD_COMMAND` used until now forced subprojects to build with `-j1`, generating the message: warning: jobserver unavailable: using -j1. Add '+' to parent make rule. See https://gitlab.kitware.com/cmake/cmake/issues/16273. --- elibs/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/elibs/CMakeLists.txt b/elibs/CMakeLists.txt index ad61888a..ab9d2fbb 100644 --- a/elibs/CMakeLists.txt +++ b/elibs/CMakeLists.txt @@ -36,7 +36,7 @@ externalproject_add(ext_mve UPDATE_COMMAND "" SOURCE_DIR ${CMAKE_SOURCE_DIR}/elibs/mve CONFIGURE_COMMAND "" - BUILD_COMMAND make -C libs/mve && make -C libs/util #not platform independent + BUILD_COMMAND $(MAKE) -C libs/mve && make -C libs/util #not platform independent BUILD_IN_SOURCE 1 INSTALL_COMMAND "" ) From 17142543ecdd0697f83060c457c18d049bfe54d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niklas=20Hamb=C3=BCchen?= Date: Mon, 14 Oct 2019 17:07:59 +0000 Subject: [PATCH 2/3] Fix segfault due to invalid masks generation. Fixes #125. Until now, it was possible that a mask with a 255 in its border was generated, later failing the `valid_mask` assertions, or, if assertions are disabled by the build, segementation faults due to invalid memory accesses. See the example in the added comment for a condition where this could happen. The key insight is that if a point lay exactly on the "border" between two pixels, say between pixel N and N+1, it counted as occupying pixel N+1. So the triangle { (1,1), (1,2), (2,1) } in a 3x3 image would result in mask 64 64 64 64 255 255 64 255 64 (notice the triangle of 255s), instead of the correct mask 64 64 64 64 255 64 64 64 64 This commit fixes it by adding the condition that the last `x = width-1` and `y = height-1` must not count as `inside` the triangle. It also improves related assertions in a few places. --- libs/tex/poisson_blending.cpp | 5 ++++- libs/tex/texture_patch.cpp | 21 ++++++++++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/libs/tex/poisson_blending.cpp b/libs/tex/poisson_blending.cpp index 2bed1da0..c378e447 100644 --- a/libs/tex/poisson_blending.cpp +++ b/libs/tex/poisson_blending.cpp @@ -20,7 +20,10 @@ typedef Eigen::SparseMatrix SpMat; math::Vec3f simple_laplacian(int i, mve::FloatImage::ConstPtr img){ const int width = img->width(); - assert(i > width + 1 && i < img->get_pixel_amount() - width -1); + // Check it's not in the top/bottom border (`texture_patch_border = 1`). + assert(i > width && i < img->get_pixel_amount() - width); + // Check it's not in the left/right border (`texture_patch_border = 1`). + assert(i % width != 0 && i % width != width-1); return -4.0f * math::Vec3f(&img->at(i, 0)) + math::Vec3f(&img->at(i - width, 0)) diff --git a/libs/tex/texture_patch.cpp b/libs/tex/texture_patch.cpp index f98bbbb2..7c2b4175 100644 --- a/libs/tex/texture_patch.cpp +++ b/libs/tex/texture_patch.cpp @@ -67,9 +67,28 @@ TexturePatch::adjust_colors(std::vector const & adjust_values) { for (int x = min_x; x < max_x; ++x) { math::Vec3f bcoords = tri.get_barycentric_coords(x, y); - bool inside = bcoords.minimum() >= 0.0f; + // The point (x, y) is inside the triangle if all of its + // barycentric coordinates are positive. + // Further, we need to check that (x, y) does not touch the + // upper `texture_patch_border` (width-1 and height-1). + // This isn't necessary for the lower boarders. + // Example: + // Consider the triangle { (0,0), (0,1), (1,0) } + // in a 1x1 pixel image. With 1-pixels borders added around it + // it becomes the triangle { (1,1), (1,2), (2,1) } + // in a 3x3 pixel image. + // The lower borders (rows x=0 and y=0) will be free as expected + // because no point has these coordinates. + // But the upper border x=2 is "just touched" by point (2,1), + // so it would be marked as occupied (and texture borders are + // intended to be not occupied). + // Thus we must exclude them explicitly in the `inside` + // condition. + bool inside = bcoords.minimum() >= 0.0f + && x != get_width() - 1 && y != get_height() - 1; if (inside) { assert(x != 0 && y != 0); + assert(x != get_width() - 1 && y != get_height() - 1); for (int c = 0; c < 3; ++c) { iadjust_values->at(x, y, c) = math::interpolate( adjust_values[i][c], adjust_values[i + 1][c], adjust_values[i + 2][c], From ea20335b4453735119e417f0002faf3a42f1df2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niklas=20Hamb=C3=BCchen?= Date: Mon, 14 Oct 2019 20:06:37 +0100 Subject: [PATCH 3/3] openmp: Switch from `critical` to `ordered` for determinism. When adding to vectors (e.g. using `push_back()`), `ordered` in contrast to `critical` ensures that ordering of operations is the same as if run single-threadedly, and it ensures that the result is the same on every run of the program. This allows to generate deterministic results: Run with same inputs, byte-identical outputs are to be produced, independent of threading. I have checked using `time` on a 6-core machine that the changes have no significant impact on performance; this is expected because the critical regions are very small (usually adding small pointers to vectors). One location remains that still uses `critical` because `omp ordered` cannot be used inside `pragma omp parallel`, only inside `pragma omp parallel for`; this is likely because of the thread-local variable `projected_face_view_infos` being intended as a per-thread intermediate buffer; more effort needs to be put into how that can be put back into order. I've added a TODO for this. I haven't yet observed nondeterminism due to this, but may as well have been lucky. --- libs/tex/calculate_data_costs.cpp | 1 + libs/tex/generate_texture_patches.cpp | 8 ++++---- libs/tex/generate_texture_views.cpp | 8 ++++---- libs/tex/global_seam_leveling.cpp | 6 +++--- 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/libs/tex/calculate_data_costs.cpp b/libs/tex/calculate_data_costs.cpp index d56cb3ef..8604ce1f 100644 --- a/libs/tex/calculate_data_costs.cpp +++ b/libs/tex/calculate_data_costs.cpp @@ -238,6 +238,7 @@ calculate_face_projection_infos(mve::TriangleMesh::ConstPtr mesh, //std::sort(projected_face_view_infos.begin(), projected_face_view_infos.end()); + // TODO: Make this part thread-ordered for determinism. #pragma omp critical { for (std::size_t i = projected_face_view_infos.size(); 0 < i; --i) { diff --git a/libs/tex/generate_texture_patches.cpp b/libs/tex/generate_texture_patches.cpp index a0967dd0..c84f6447 100644 --- a/libs/tex/generate_texture_patches.cpp +++ b/libs/tex/generate_texture_patches.cpp @@ -466,7 +466,7 @@ generate_texture_patches(UniGraph const & graph, mve::TriangleMesh::ConstPtr mes std::size_t num_patches = 0; std::cout << "\tRunning... " << std::flush; - #pragma omp parallel for schedule(dynamic) + #pragma omp parallel for ordered schedule(dynamic) for (std::size_t i = 0; i < texture_views->size(); ++i) { std::vector > subgraphs; @@ -511,7 +511,7 @@ generate_texture_patches(UniGraph const & graph, mve::TriangleMesh::ConstPtr mes for (; it != candidates.end(); ++it) { std::size_t texture_patch_id; - #pragma omp critical + #pragma omp ordered { texture_patches->push_back(it->texture_patch); texture_patch_id = num_patches++; @@ -542,7 +542,7 @@ generate_texture_patches(UniGraph const & graph, mve::TriangleMesh::ConstPtr mes std::vector > subgraphs; graph.get_subgraphs(0, &subgraphs); - #pragma omp parallel for schedule(dynamic) + #pragma omp parallel for ordered schedule(dynamic) for (std::size_t i = 0; i < subgraphs.size(); ++i) { std::vector const & subgraph = subgraphs[i]; @@ -556,7 +556,7 @@ generate_texture_patches(UniGraph const & graph, mve::TriangleMesh::ConstPtr mes num_patches += 1; } else { if (settings.keep_unseen_faces) { - #pragma omp critical + #pragma omp ordered unseen_faces.insert(unseen_faces.end(), subgraph.begin(), subgraph.end()); } diff --git a/libs/tex/generate_texture_views.cpp b/libs/tex/generate_texture_views.cpp index 9993aa31..ad426638 100644 --- a/libs/tex/generate_texture_views.cpp +++ b/libs/tex/generate_texture_views.cpp @@ -109,7 +109,7 @@ from_images_and_camera_files(std::string const & path, } ProgressCounter view_counter("\tLoading", files.size() / 2); - #pragma omp parallel for + #pragma omp parallel for ordered for (std::size_t i = 0; i < files.size(); i += 2) { view_counter.progress(); const std::string cam_file = files[i]; @@ -168,7 +168,7 @@ from_images_and_camera_files(std::string const & path, mve::image::save_png_file(image, image_file); } - #pragma omp critical + #pragma omp ordered texture_views->push_back(TextureView(i / 2, cam_info, image_file)); view_counter.inc(); @@ -184,7 +184,7 @@ from_nvm_scene(std::string const & nvm_file, mve::Bundle::Cameras& cameras = bundle->get_cameras(); ProgressCounter view_counter("\tLoading", cameras.size()); - #pragma omp parallel for + #pragma omp parallel for ordered for (std::size_t i = 0; i < cameras.size(); ++i) { view_counter.progress(); mve::CameraInfo& mve_cam = cameras[i]; @@ -208,7 +208,7 @@ from_nvm_scene(std::string const & nvm_file, ); mve::image::save_png_file(image, image_file); - #pragma omp critical + #pragma omp ordered texture_views->push_back(TextureView(i, mve_cam, image_file)); view_counter.inc(); diff --git a/libs/tex/global_seam_leveling.cpp b/libs/tex/global_seam_leveling.cpp index 7fa60f39..a71512d6 100644 --- a/libs/tex/global_seam_leveling.cpp +++ b/libs/tex/global_seam_leveling.cpp @@ -254,7 +254,7 @@ global_seam_leveling(UniGraph const & graph, mve::TriangleMesh::ConstPtr mesh, util::WallTimer timer; std::cout << "\tCalculating adjustments:"<< std::endl; - #pragma omp parallel for + #pragma omp parallel for ordered for (std::size_t channel = 0; channel < 3; ++channel) { /* Prepare solver. */ Eigen::ConjugateGradient cg; @@ -276,11 +276,11 @@ global_seam_leveling(UniGraph const & graph, mve::TriangleMesh::ConstPtr mesh, /* Subtract mean because system is underconstrained and we seek the solution with minimal adjustments. */ x = x.array() - x.mean(); - #pragma omp critical + #pragma omp ordered std::cout << "\t\tColor channel " << channel << ": CG took " << cg.iterations() << " iterations. Residual is " << cg.error() << std::endl; - #pragma omp critical + #pragma omp ordered for (std::size_t i = 0; i < num_vertices; ++i) { for (std::size_t j = 0; j < labels[i].size(); ++j) { std::size_t label = labels[i][j];