From 938a3a169e66adb71949907fcf3ebca2089727a9 Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Wed, 6 Mar 2024 07:58:20 -0800 Subject: [PATCH 1/6] Fix `-Wimplicit-const-int-float-conversion` violation in mapillary/opensfm/opensfm/src/features/src/matching.cc Summary: # Overview LLVM has detected a bad _implicit constant integer to float conversion_ in mapillary/opensfm/opensfm/src/features/src/matching.cc. Almost all such cases lead to very subtle bugs, so we are making them a compiler error by default. # Reproducer The fix can be checked using: ``` buck2 build --config cxx.extra_cxxflags=-Wimplicit-const-int-float-conversion ``` # Description of the problem What value do you think this produces? ``` static_cast(static_cast(std::numeric_limits::max())) ``` You'd hope it would produce UINT64_MAX (18446744073709551615), but it doesn't. In debug mode it produces 9223372036854775808 (half of the above). In opt mode it produces "random" numbers such as 140726009322632 (which is 130,000x smaller than UINT64_MAX). Comparisons between floats and maximum integer values are also scary: ``` constexpr double x = std::numeric_limits::max(); constexpr auto kMax64 = std::numeric_limits::max(); std::cout << (x > kMax64 ? kMax64 : static_cast(x)) << std::endl; ``` produces garbage: 140731185888920 The reason this happens is because floating-point types can only represent integers sparsely in the regions of the maximum integers (generated with P1188399781): ``` delta = 128 2147483264 f 2147483392 f 2147483520 f 2147483647 <- Largest integer 2147483648 f 2147483904 f 2147484160 f delta = 256 4294966528 f 4294966784 f 4294967040 f 4294967295 <- Largest integer 4294967296 f 4294967808 f 4294968320 f delta = 1024 9223372036854772736 d 9223372036854773760 d 9223372036854774784 d 9223372036854775807 <- Largest integer 9223372036854775808 d 9223372036854777856 d 9223372036854779904 d delta = 2048 18446744073709545472 d 18446744073709547520 d 18446744073709549568 d 18446744073709551615 <- Largest integer 18446744073709551616 d 18446744073709555712 d 18446744073709559808 d ``` # Fixing the Problem See example fixes in D54119898, D54119901, D54119899. ## Careful Clamping The fix will be situation dependent, `folly::constexpr_clamp_cast` is _very_ useful: ``` // Include the target `//folly:constexpr_math` #include #include #include int main() { constexpr auto kMax32 = std::numeric_limits::max(); std::cout << folly::constexpr_clamp_cast(static_cast(kMax32)) << std::endl; return 0; } ``` ## Use infinity In some cases we want want something like ``` double smallest_value = BIG_NUMBER; for (const auto &x : values) { smallest_value = std::min(smallest_value, x); } ``` If `BIG_NUMBER` is an integer this code could be incorrect! Instead we use ``` double smallest_value = std::numeric_limits::infinity(); ``` ## Make the conversion explicit For situations in which we are dividing by a very large integer, we can use `static_cast(LARGE_INT)` to make the conversion from int to float explicit. The conversion is _not_ exact, but our hope is that the precision we've lost is small enough that it doesn't matter for the problem being solved. Reviewed By: YanNoun Differential Revision: D54586513 fbshipit-source-id: 87fba3097e3822eb68660e0f2ab947eaf6450698 --- opensfm/src/features/src/matching.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/opensfm/src/features/src/matching.cc b/opensfm/src/features/src/matching.cc index b25ed46ca..26e7e4339 100644 --- a/opensfm/src/features/src/matching.cc +++ b/opensfm/src/features/src/matching.cc @@ -41,8 +41,8 @@ void MatchUsingWords(const cv::Mat &f1, const cv::Mat &w1, const cv::Mat &f2, } std::vector best_match(f1.rows, -1), second_best_match(f1.rows, -1); - std::vector best_distance(f1.rows, 99999999), - second_best_distance(f1.rows, 99999999); + std::vector best_distance(f1.rows, std::numeric_limits::infinity()); + std::vector second_best_distance(f1.rows, std::numeric_limits::infinity()); *matches = cv::Mat(0, 2, CV_32S); cv::Mat tmp_match(1, 2, CV_32S); for (unsigned int i = 0; i < w1.rows; ++i) { From 424ecc210ee468c0eab46a019d484ceaf4893a21 Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Wed, 6 Mar 2024 08:05:42 -0800 Subject: [PATCH 2/6] Fix `-Wimplicit-const-int-float-conversion` violation in mapillary/opensfm/opensfm/src/dense/src/depthmap.cc Summary: # Overview LLVM has detected a bad _implicit constant integer to float conversion_ in mapillary/opensfm/opensfm/src/dense/src/depthmap.cc. Almost all such cases lead to very subtle bugs, so we are making them a compiler error by default. # Reproducer The fix can be checked using: ``` buck2 build --config cxx.extra_cxxflags=-Wimplicit-const-int-float-conversion ``` # Description of the problem What value do you think this produces? ``` static_cast(static_cast(std::numeric_limits::max())) ``` You'd hope it would produce UINT64_MAX (18446744073709551615), but it doesn't. In debug mode it produces 9223372036854775808 (half of the above). In opt mode it produces "random" numbers such as 140726009322632 (which is 130,000x smaller than UINT64_MAX). Comparisons between floats and maximum integer values are also scary: ``` constexpr double x = std::numeric_limits::max(); constexpr auto kMax64 = std::numeric_limits::max(); std::cout << (x > kMax64 ? kMax64 : static_cast(x)) << std::endl; ``` produces garbage: 140731185888920 The reason this happens is because floating-point types can only represent integers sparsely in the regions of the maximum integers (generated with P1188399781): ``` delta = 128 2147483264 f 2147483392 f 2147483520 f 2147483647 <- Largest integer 2147483648 f 2147483904 f 2147484160 f delta = 256 4294966528 f 4294966784 f 4294967040 f 4294967295 <- Largest integer 4294967296 f 4294967808 f 4294968320 f delta = 1024 9223372036854772736 d 9223372036854773760 d 9223372036854774784 d 9223372036854775807 <- Largest integer 9223372036854775808 d 9223372036854777856 d 9223372036854779904 d delta = 2048 18446744073709545472 d 18446744073709547520 d 18446744073709549568 d 18446744073709551615 <- Largest integer 18446744073709551616 d 18446744073709555712 d 18446744073709559808 d ``` # Fixing the Problem See example fixes in D54119898, D54119901, D54119899. ## Careful Clamping The fix will be situation dependent, `folly::constexpr_clamp_cast` is _very_ useful: ``` // Include the target `//folly:constexpr_math` #include #include #include int main() { constexpr auto kMax32 = std::numeric_limits::max(); std::cout << folly::constexpr_clamp_cast(static_cast(kMax32)) << std::endl; return 0; } ``` ## Use infinity In some cases we want want something like ``` double smallest_value = BIG_NUMBER; for (const auto &x : values) { smallest_value = std::min(smallest_value, x); } ``` If `BIG_NUMBER` is an integer this code could be incorrect! Instead we use ``` double smallest_value = std::numeric_limits::infinity(); ``` ## Make the conversion explicit For situations in which we are dividing by a very large integer, we can use `static_cast(LARGE_INT)` to make the conversion from int to float explicit. The conversion is _not_ exact, but our hope is that the precision we've lost is small enough that it doesn't matter for the problem being solved. Reviewed By: YanNoun Differential Revision: D54586527 fbshipit-source-id: 6ac8812158f204457e002f2c3ec15b55d8fcb538 --- opensfm/src/dense/src/depthmap.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/opensfm/src/dense/src/depthmap.cc b/opensfm/src/dense/src/depthmap.cc index a665d9296..57ef85b6d 100644 --- a/opensfm/src/dense/src/depthmap.cc +++ b/opensfm/src/dense/src/depthmap.cc @@ -126,7 +126,8 @@ cv::Vec3f PlaneFromDepthAndNormal(float x, float y, const cv::Matx33d &K, } float UniformRand(float a, float b) { - return a + (b - a) * float(rand()) / RAND_MAX; + // Note that float(RAND_MAX) cannot be exactly represented as a float. We ignore the small inaccuracy here; this is already a bad way to get random numbers. + return a + (b - a) * float(rand()) / static_cast(RAND_MAX); } DepthmapEstimator::DepthmapEstimator() From afb0c15beacbf68890c3ddcadbb33186cc03f379 Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Thu, 7 Mar 2024 08:17:38 -0800 Subject: [PATCH 3/6] Fix `-Wimplicit-const-int-float-conversion` violation in mapillary/opensfm/opensfm/src/geometry/relative_pose.h Summary: # Overview LLVM has detected a bad _implicit constant integer to float conversion_ in mapillary/opensfm/opensfm/src/geometry/relative_pose.h. Almost all such cases lead to very subtle bugs, so we are making them a compiler error by default. # Reproducer The fix can be checked using: ``` buck2 build --config cxx.extra_cxxflags=-Wimplicit-const-int-float-conversion ``` # Description of the problem What value do you think this produces? ``` static_cast(static_cast(std::numeric_limits::max())) ``` You'd hope it would produce UINT64_MAX (18446744073709551615), but it doesn't. In debug mode it produces 9223372036854775808 (half of the above). In opt mode it produces "random" numbers such as 140726009322632 (which is 130,000x smaller than UINT64_MAX). Comparisons between floats and maximum integer values are also scary: ``` constexpr double x = std::numeric_limits::max(); constexpr auto kMax64 = std::numeric_limits::max(); std::cout << (x > kMax64 ? kMax64 : static_cast(x)) << std::endl; ``` produces garbage: 140731185888920 The reason this happens is because floating-point types can only represent integers sparsely in the regions of the maximum integers (generated with P1188399781): ``` delta = 128 2147483264 f 2147483392 f 2147483520 f 2147483647 <- Largest integer 2147483648 f 2147483904 f 2147484160 f delta = 256 4294966528 f 4294966784 f 4294967040 f 4294967295 <- Largest integer 4294967296 f 4294967808 f 4294968320 f delta = 1024 9223372036854772736 d 9223372036854773760 d 9223372036854774784 d 9223372036854775807 <- Largest integer 9223372036854775808 d 9223372036854777856 d 9223372036854779904 d delta = 2048 18446744073709545472 d 18446744073709547520 d 18446744073709549568 d 18446744073709551615 <- Largest integer 18446744073709551616 d 18446744073709555712 d 18446744073709559808 d ``` # Fixing the Problem See example fixes in D54119898, D54119901, D54119899. ## Careful Clamping The fix will be situation dependent, `folly::constexpr_clamp_cast` is _very_ useful: ``` // Include the target `//folly:constexpr_math` #include #include #include int main() { constexpr auto kMax32 = std::numeric_limits::max(); std::cout << folly::constexpr_clamp_cast(static_cast(kMax32)) << std::endl; return 0; } ``` ## Use infinity In some cases we want want something like ``` double smallest_value = BIG_NUMBER; for (const auto &x : values) { smallest_value = std::min(smallest_value, x); } ``` If `BIG_NUMBER` is an integer this code could be incorrect! Instead we use ``` double smallest_value = std::numeric_limits::infinity(); ``` ## Make the conversion explicit For situations in which we are dividing by a very large integer, we can use `static_cast(LARGE_INT)` to make the conversion from int to float explicit. The conversion is _not_ exact, but our hope is that the precision we've lost is small enough that it doesn't matter for the problem being solved. Reviewed By: dmm-fb Differential Revision: D54586517 fbshipit-source-id: b03d0e05edc5dccff17811e8419548cfb72759b0 --- opensfm/src/geometry/relative_pose.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/opensfm/src/geometry/relative_pose.h b/opensfm/src/geometry/relative_pose.h index 997875764..7fb759fd9 100644 --- a/opensfm/src/geometry/relative_pose.h +++ b/opensfm/src/geometry/relative_pose.h @@ -87,7 +87,8 @@ struct RelativePoseCost { std::srand(42); const int count = end_ - begin_; for (int i = 0; i < MAX_ERRORS; ++i) { - const int index = (float(std::rand()) / RAND_MAX) * count; + // Note that float(RAND_MAX) cannot be exactly represented as a float. We ignore the small inaccuracy here; this is already a bad way to get random numbers. + const int index = (float(std::rand()) / static_cast(RAND_MAX)) * count; picked_errors_.push_back(begin_ + index); } } From 78b795c58a4684195c6c7ab5e7d531ee484f32c6 Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Thu, 7 Mar 2024 10:57:56 -0800 Subject: [PATCH 4/6] Remove unused variables in mapillary/opensfm/opensfm/src/third_party/akaze/lib/AKAZE.cpp Summary: LLVM-15 has a warning `-Wunused-but-set-variable` which we treat as an error because it's so often diagnostic of a code issue. Unused variables can compromise readability or, worse, performance. This diff either (a) removes an unused variable and, possibly, it's associated code, or (b) qualifies the variable with `[[maybe_unused]]`, mostly in cases where the variable _is_ used, but, eg, in an `assert` statement that isn't present in production code. - If you approve of this diff, please use the "Accept & Ship" button :-) Reviewed By: dmm-fb Differential Revision: D54378379 fbshipit-source-id: 0a23b525ffbe0862e32609ce45af4c2cd7794551 --- opensfm/src/third_party/akaze/lib/AKAZE.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/opensfm/src/third_party/akaze/lib/AKAZE.cpp b/opensfm/src/third_party/akaze/lib/AKAZE.cpp index 5d42f251d..e49b6f1c2 100644 --- a/opensfm/src/third_party/akaze/lib/AKAZE.cpp +++ b/opensfm/src/third_party/akaze/lib/AKAZE.cpp @@ -269,7 +269,7 @@ void AKAZE::Find_Scale_Space_Extrema(std::vector& kpts) { double t1 = 0.0, t2 = 0.0; float value = 0.0; float dist = 0.0, ratio = 0.0, smax = 0.0; - int npoints = 0, id_repeated = 0; + int id_repeated = 0; int sigma_size_ = 0, left_x = 0, right_x = 0, up_y = 0, down_y = 0; bool is_extremum = false, is_repeated = false, is_out = false; cv::KeyPoint point; @@ -357,7 +357,6 @@ void AKAZE::Find_Scale_Space_Extrema(std::vector& kpts) { point.pt.x *= ratio; point.pt.y *= ratio; kpts_aux.push_back(point); - npoints++; } else { point.pt.x *= ratio; From 4b562c6ab178b60d1f167dc2ec24d6dad8bb7aa6 Mon Sep 17 00:00:00 2001 From: oafolabi Date: Mon, 18 Mar 2024 10:02:32 -0700 Subject: [PATCH 5/6] =?UTF-8?q?returning=20norm=5Fcoords=20instead=20of=20?= =?UTF-8?q?px=5Fcoords=20in=20pixelToNormalizedCoordi=E2=80=A6=20(#1031)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: …natesMany Attempting to fix issue create here: [https://github.com/mapillary/OpenSfM/issues/1030#issue-2046628484](https://github.com/mapillary/OpenSfM/issues/1030#issue-2046628484) Pull Request resolved: https://github.com/mapillary/OpenSfM/pull/1031 Reviewed By: paulinus Differential Revision: D55006764 Pulled By: fabianschenk fbshipit-source-id: 506078187c0f2387535521f0068c3322f2465b22 --- opensfm/src/geometry/src/camera.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opensfm/src/geometry/src/camera.cc b/opensfm/src/geometry/src/camera.cc index 45619f6cf..d864fa743 100644 --- a/opensfm/src/geometry/src/camera.cc +++ b/opensfm/src/geometry/src/camera.cc @@ -366,7 +366,7 @@ MatX2d Camera::PixelToNormalizedCoordinatesMany(const MatX2d& px_coords, norm_coords.row(i) = PixelToNormalizedCoordinates(px_coords.row(i), width, height); } - return px_coords; + return norm_coords; } Vec2d Camera::NormalizedToPixelCoordinates(const Vec2d& norm_coord) const { From c72419228fb8c86f2c367989abd64f784ec684b3 Mon Sep 17 00:00:00 2001 From: generatedunixname89002005307016 Date: Thu, 21 Mar 2024 18:37:26 -0700 Subject: [PATCH 6/6] upgrade pyre version in `fbcode/mapillary` - batch 1 Differential Revision: D55201655 fbshipit-source-id: a1eb370d0b39457e3f59614a093a50022869eca9 --- opensfm/commands/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/opensfm/commands/__init__.py b/opensfm/commands/__init__.py index 97a2f32fe..da12e64dc 100644 --- a/opensfm/commands/__init__.py +++ b/opensfm/commands/__init__.py @@ -24,6 +24,7 @@ reconstruct_from_prior, undistort, ) +# pyre-fixme[21]: Could not find module `opensfm.commands.command_runner`. from .command_runner import command_runner