diff --git a/modules/imgproc/include/opencv2/imgproc.hpp b/modules/imgproc/include/opencv2/imgproc.hpp index 5d6416debb..ae46e278d2 100644 --- a/modules/imgproc/include/opencv2/imgproc.hpp +++ b/modules/imgproc/include/opencv2/imgproc.hpp @@ -4208,6 +4208,9 @@ The function calculates and returns the minimal up-right bounding rectangle for non-zero pixels of gray-scale image. @param array Input gray-scale image or 2D point set, stored in std::vector or Mat. + +@note Point coordinates that the resulting Rect cannot represent - outside of the int range, +or infinite - are saturated to INT_MIN / INT_MAX, and its width and height are clamped to INT_MAX. */ CV_EXPORTS_W Rect boundingRect( InputArray array ); diff --git a/modules/imgproc/src/geometry.cpp b/modules/imgproc/src/geometry.cpp index 020a636835..33097a4c19 100644 --- a/modules/imgproc/src/geometry.cpp +++ b/modules/imgproc/src/geometry.cpp @@ -703,6 +703,24 @@ static Rect maskBoundingRect( const Mat& img ) return Rect(xmin, ymin, xmax - xmin + 1, ymax - ymin + 1); } +// Converts an extremum of a CV_32F point set to an int coordinate of a Rect, flooring it and +// clamping it to the range a Rect can represent. cvFloor() is undefined outside of that range, so +// a coordinate beyond it used to leak the raw conversion result into the rectangle: 1e10 floored +// to INT_MIN, and +inf/-inf gave INT_MIN and INT_MAX, inverting it. +// See https://github.com/opencv/opencv/issues/29837 +static inline int floorSaturate( float value ) +{ + // the clamp is in double, where the bounds are exact - INT_MAX is not representable in float - + // and it cannot be left to saturate_cast(), which rounds but does not clip 32-bit + // integers. Reversing the first comparison keeps a NaN out of the undefined conversion too. + const double v = std::floor((double)value); + if( !(v > (double)INT_MIN) ) + return INT_MIN; + if( v >= (double)INT_MAX ) + return INT_MAX; + return saturate_cast(v); +} + // Calculates bounding rectangle of a point set or retrieves already calculated static Rect pointSetBoundingRect( const Mat& points ) { @@ -767,8 +785,9 @@ static Rect pointSetBoundingRect( const Mat& points ) const float* pts = points.ptr(); int64_t firstval = 0; std::memcpy(&firstval, pts, sizeof(pts[0]) * 2); - xmin = xmax = cvFloor(pts[0]); - ymin = ymax = cvFloor(pts[1]); + // the extrema are searched for in floats and converted to int once, below + float xmin_f = pts[0], xmax_f = pts[0]; + float ymin_f = pts[1], ymax_f = pts[1]; #if CV_SIMD || CV_SIMD_SCALABLE v_float32 minval, maxval; minval = maxval = v_reinterpret_as_f32(vx_setall_s64(firstval)); //min[0]=pt.x, min[1]=pt.y, min[2]=pt.x, min[3]=pt.y @@ -791,29 +810,35 @@ static Rect pointSetBoundingRect( const Mat& points ) vx_store(arr_maxval, maxval); for (int j = 0; j < nlanes; j++) { - int _xmin = cvFloor(arr_minval[2*j]), _ymin = cvFloor(arr_minval[2*j+1]); - int _xmax = cvFloor(arr_maxval[2*j]), _ymax = cvFloor(arr_maxval[2*j+1]); - xmin = std::min(xmin, _xmin); - ymin = std::min(ymin, _ymin); - xmax = std::max(xmax, _xmax); - ymax = std::max(ymax, _ymax); + xmin_f = std::min(xmin_f, arr_minval[2*j]); + ymin_f = std::min(ymin_f, arr_minval[2*j+1]); + xmax_f = std::max(xmax_f, arr_maxval[2*j]); + ymax_f = std::max(ymax_f, arr_maxval[2*j+1]); } #endif for( ; i < npoints; i++ ) { - // because right and bottom sides of the bounding rectangle are not inclusive - // (note +1 in width and height calculation below), cvFloor is used here instead of cvCeil - int pt_x = cvFloor(pts[2*i]); - int pt_y = cvFloor(pts[2*i+1]); + float pt_x = pts[2*i]; + float pt_y = pts[2*i+1]; - xmin = std::min(xmin, pt_x); - xmax = std::max(xmax, pt_x); - ymin = std::min(ymin, pt_y); - ymax = std::max(ymax, pt_y); + xmin_f = std::min(xmin_f, pt_x); + xmax_f = std::max(xmax_f, pt_x); + ymin_f = std::min(ymin_f, pt_y); + ymax_f = std::max(ymax_f, pt_y); } + + // because right and bottom sides of the bounding rectangle are not inclusive + // (note +1 in width and height calculation below), the extrema are floored, not rounded + xmin = floorSaturate(xmin_f); + xmax = floorSaturate(xmax_f); + ymin = floorSaturate(ymin_f); + ymax = floorSaturate(ymax_f); } - return Rect(xmin, ymin, xmax - xmin + 1, ymax - ymin + 1); + // xmax - xmin overflows int once the extrema saturate, so the sides are computed in int64 + const int64_t width = (int64_t)xmax - (int64_t)xmin + 1; + const int64_t height = (int64_t)ymax - (int64_t)ymin + 1; + return Rect(xmin, ymin, (int)std::min(width, INT_MAX), (int)std::min(height, INT_MAX)); } diff --git a/modules/imgproc/test/test_boundingrect.cpp b/modules/imgproc/test/test_boundingrect.cpp index cf70b66e04..af7f89165f 100644 --- a/modules/imgproc/test/test_boundingrect.cpp +++ b/modules/imgproc/test/test_boundingrect.cpp @@ -96,4 +96,42 @@ TEST(Imgproc_BoundingRect, bug_24217) } } +// See https://github.com/opencv/opencv/issues/29837 +// A CV_32F coordinate outside of the int range used to reach cvFloor(), which is undefined +// there: points spanning 1e10 collapsed to a 1x1 rect at INT_MIN, and +inf/-inf gave +// INT_MIN/INT_MAX, an inverted rectangle. They are saturated to the representable bounds now. +TEST(Imgproc_BoundingRect, out_of_int_range_29837) +{ + const int imin = std::numeric_limits::min(); + const int imax = std::numeric_limits::max(); + const float inf = std::numeric_limits::infinity(); + + // in-range point sets are unaffected + std::vector in_range { Point2f(1e4f, 1e4f), Point2f(2e4f, 2e4f) }; + EXPECT_EQ(boundingRect(in_range), Rect(10000, 10000, 10001, 10001)); + + // out of range: saturated, orientation preserved + std::vector above { Point2f(1e10f, 1e10f), Point2f(2e10f, 2e10f) }; + EXPECT_EQ(boundingRect(above), Rect(imax, imax, 1, 1)); + + std::vector below { Point2f(-1e10f, -1e10f), Point2f(-2e10f, -2e10f) }; + EXPECT_EQ(boundingRect(below), Rect(imin, imin, 1, 1)); + + // infinities saturate the same way, and the sides are clamped instead of overflowing + std::vector plus_inf { Point2f(inf, 0.f), Point2f(1.f, 1.f), Point2f(2.f, 0.f) }; + EXPECT_EQ(boundingRect(plus_inf), Rect(1, 0, imax, 2)); + + std::vector minus_inf { Point2f(-inf, 0.f), Point2f(1.f, 1.f), Point2f(2.f, 0.f) }; + EXPECT_EQ(boundingRect(minus_inf), Rect(imin, 0, imax, 2)); + + // long enough to go through the SIMD path as well + std::vector simd(64, Point2f(5.f, 5.f)); + simd[37] = Point2f(1e10f, -1e10f); + EXPECT_EQ(boundingRect(simd), Rect(5, imin, imax - 4, imax)); + + // CV_32S needs no conversion, but its sides can overflow int just the same + std::vector ints { Point(imin, imin), Point(imax, imax) }; + EXPECT_EQ(boundingRect(ints), Rect(imin, imin, imax, imax)); +} + }} // namespace