Skip to content

fix meanStdDev overflow for large images#26867

Merged
asmorkalov merged 1 commit intoopencv:4.xfrom
shyama7004:fix-meanStdDev
Feb 7, 2025
Merged

fix meanStdDev overflow for large images#26867
asmorkalov merged 1 commit intoopencv:4.xfrom
shyama7004:fix-meanStdDev

Conversation

@shyama7004
Copy link
Copy Markdown
Contributor

Fixes : #26861

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@chacha21
Copy link
Copy Markdown
Contributor

chacha21 commented Feb 2, 2025

Isn't there a preparatory step to foresee so that (int)src.total() can be src.total() instead, in case of an hal implementation ? (I think there is none for now)

if (src.isContinuous() && mask.isContinuous())
    {
        CALL_HAL(meanStdDev, cv_hal_meanStdDev, src.data, 0, (int)src.total(), 1, src.type(),
                 _mean.needed() ? mean_mat.ptr<double>() : nullptr,
                 _sdv.needed() ? stddev_mat.ptr<double>() : nullptr,
                 mask.data, 0);
    }

@shyama7004
Copy link
Copy Markdown
Contributor Author

Isn't there a preparatory step to foresee so that (int)src.total() can be src.total() instead, in case of an hal implementation ? (I think there is none for now)

if (src.isContinuous() && mask.isContinuous())
    {
        CALL_HAL(meanStdDev, cv_hal_meanStdDev, src.data, 0, (int)src.total(), 1, src.type(),
                 _mean.needed() ? mean_mat.ptr<double>() : nullptr,
                 _sdv.needed() ? stddev_mat.ptr<double>() : nullptr,
                 mask.data, 0);
    }

Your concern is valid, can do the following

if (src.isContinuous() && mask.isContinuous())
{
    const size_t tpix = src.total();
    CALL_HAL(meanStdDev, cv_hal_meanStdDev, src.data, 0, tpix, 1, src.type(),
             _mean.needed() ? mean_mat.ptr<double>() : nullptr,
             _sdv.needed() ? stddev_mat.ptr<double>() : nullptr,
             mask.data, 0);
}

@chacha21
Copy link
Copy Markdown
Contributor

chacha21 commented Feb 2, 2025

The prototype of cv_hal_meanStdDev is currently cv_hal_meanStdDev(..., int width, int height, ...), so just using a size_t tpix for width and 1 for height won't solve anything.
Either the prototype of cv_hal_meanStdDev() must change to accept size_t arguments, or it can only be used if width*height does not overflow.

@shyama7004 shyama7004 marked this pull request as draft February 2, 2025 16:34
@shyama7004 shyama7004 marked this pull request as ready for review February 3, 2025 06:43
@vpisarev
Copy link
Copy Markdown
Contributor

vpisarev commented Feb 5, 2025

@shyama7004, the added test requires 16Gb of RAM. That's a huge number, quite inconvenient for our CI. I suggest to, first of all, consider 8U type for array, not 32F and then reduce size of the array to a smaller number of elements, ~2 billions or so (e.g. (1<<16) x ((1<<15) + 10)). It will still cause 32-bit integer overflow, but it will take 8x less space than the current version

@shyama7004
Copy link
Copy Markdown
Contributor Author

@shyama7004, the added test requires 16Gb of RAM. That's a huge number, quite inconvenient for our CI. I suggest to, first of all, consider 8U type for array, not 32F and then reduce size of the array to a smaller number of elements, ~2 billions or so (e.g. (1<<16) x ((1<<15) + 10)). It will still cause 32-bit integer overflow, but it will take 8x less space than the current version

Ok I will implement this

@asmorkalov asmorkalov force-pushed the fix-meanStdDev branch 2 times, most recently from 99d13df to 16fd7a7 Compare February 7, 2025 07:17
@asmorkalov asmorkalov self-requested a review February 7, 2025 07:18
@asmorkalov asmorkalov self-assigned this Feb 7, 2025
@asmorkalov asmorkalov added this to the 4.12.0 milestone Feb 7, 2025
@asmorkalov asmorkalov merged commit 740388b into opencv:4.x Feb 7, 2025
27 of 28 checks passed
@shyama7004 shyama7004 deleted the fix-meanStdDev branch February 7, 2025 10:53
@asmorkalov asmorkalov mentioned this pull request Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cv::meanStdDev could not compute values for large images.

4 participants