Fix bug at blobFromImagesWithParams by CSBVision · Pull Request #24128 · opencv/opencv

CSBVision · 2023-08-08T12:18:35Z

`blobFromImagesWithParams`(image-s) apply swap scalefactor for every images when swapRB is true. This patch moves the swap scalefactor code out of the loop, that fix the above issue.

We would like to improve the recently introduced function blobFromImagesWithParams with respect to three minor issues that we noticed:

Currently, mean and scalefactor are recreated for every image, even though they should be reused. Maybe the compiler is smart enough to optimize this, still we think the variable should be created / swapped before the main loop.
Next, we would like to improve the function's robustness with respect to inconsistent arguments, i.e. if a single-channel image is passed but swapRB is true, as explained below.
Finally, we made some minor modifications to follow the coding guidelines (if - else if - else instead of nested else).

Function wise, there are two improvements from this:

Currently, if the user passes a single-channel image in combination with mean and scalefactor values, but unintentionally sets swapRB to true, the whole result can be silently zeroed. The mean and scalefactor arrays will only contain a single value (whereas the other indices' values remain 0), but swapRB will swap out the true mean / scalefactor values and swap in zeros for these instead. There is no warning or assertion, only the whole image will be zeroed by the following multiplication. Therefore, we suggest to only swap these values if the image is not a single-channel image, i.e. mean and scalefactor contain correct values for all channels, therefore add if (nch > 2).
Next, a related issue exists in combination with DNN_LAYOUT_NHWC. If the user passes this in combination with a single-channel image and swapRB is true, the function produces the following error:

OpenCV\modules\core\src\copy.cpp:320: error: (-215:Assertion failed) channels() == CV_MAT_CN(dtype) in function 'cv::Mat::copyTo'

This results from first converting the image to RGB (even though it is grayscale instead of the expected BGR layout) and, thereafter, copying the resulting three-channel image into the single-channel blob. This is avoided by checking nch > 2 here, too, which is also consistent to the function's implementation of the DNN_LAYOUT_NCHW case.

Both changes can be summarized such that swapRB only has an effect if the image has at least three channels. Most importantly, nothing changes at all if the arguments are consistent.

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

CSBVision · 2023-08-08T12:51:37Z

Furthermore, we think there is also a bug in the current implementation that is fixed by our changes. If the user passes multiple three-channel images in combination with swapRB == true and RGB scales, scalefactor will be swapped during every iteration of the main loop. Consequently, for images with even indices, the correct scales will be used, while for all images with odd indices, the red and blue factors are incorrectly swapped.

LaurentBerger · 2023-08-08T13:25:54Z

Furthermore, we think there is also a bug in the current implementation that is fixed by our changes.

I don't undestand. Image2BlobParams is const in blobFromImageWithParams

I tried VS2022 + windows11

    Mat image = imread(samples::findFile("lena.jpg"));
        Image2BlobParams paramBlob;
        paramBlob.datalayout = DNN_LAYOUT_NCHW;
        paramBlob.ddepth = CV_32F;
        paramBlob.mean = Scalar(4, 5, 6);
        paramBlob.scalefactor = Scalar(1, 2, 3);
        paramBlob.size = Size(1024, 1024);
        paramBlob.swapRB = true;
        paramBlob.paddingmode = DNN_PMODE_NULL;
        cout << "Init " << paramBlob.scalefactor << endl;
        Mat blobOpencv = blobFromImageWithParams(image, paramBlob);
        cout << "First call " << paramBlob.scalefactor << endl;
        blobOpencv = blobFromImageWithParams(image, paramBlob);
        cout << "Second call " << paramBlob.scalefactor << endl;

and result is

Init [1, 2, 3, 0]
Init [4, 5, 6, 0]
First call [1, 2, 3, 0]
First call [4, 5, 6, 0]
Second call [1, 2, 3, 0]
Second call [4, 5, 6, 0]
[```

CSBVision · 2023-08-08T13:34:26Z

Correct, but the actually applied weights are those defined by

Scalar scalefactor = param.scalefactor;

which is a non-const local variable inside blobFromImagesWithParams. Here, red and blue values are swapped during each iteration, which is only relevant if you pass a collection of images instead of a single one. If you only use blobFromImageWithParams (notice the missing 's' - blobFromImage and blobFromImages), your input is forwarded to blobFromImagesWithParams, but the main loop is only executed once.
Inside each iteration we currently have

Scalar mean = param.mean;
if (param.swapRB)
{
    std::swap(mean[0], mean[2]);
    std::swap(scalefactor[0], scalefactor[2]);
}

which recreates mean (i.e. no issue here) but reuses scalefactor and keeps swapping.

LaurentBerger · 2023-08-08T14:04:00Z

Ok only scaleFactor variables is outside the loop. yes there is a problem :

{
        Mat image = imread(samples::findFile("lena.jpg"));
        Mat roi(image(Rect(0, 0, 4, 2)));
        vector<Mat> tab = { roi.clone(), roi.clone(), roi.clone() };
        Image2BlobParams paramBlob;
        paramBlob.datalayout = DNN_LAYOUT_NHWC;
        paramBlob.ddepth = CV_32F;
        paramBlob.mean = Scalar(4, 5, 6);
        paramBlob.scalefactor = Scalar(1, 2, 3);
        paramBlob.size = Size(4, 2);
        paramBlob.swapRB = true;
        paramBlob.paddingmode = DNN_PMODE_NULL;
        cout << "Init " << paramBlob.scalefactor << endl;
        cout << tab[0] << endl;
        Mat blobOpencv = blobFromImagesWithParams(tab, paramBlob);
        displayBlobSize(blobOpencv, "blobOpencv");
        Mat x;
        x = blobOpencv.reshape(3, vector<int> {3,8});
        cout << "Original " << tab[2] << endl;
        for (int i = 0; i < tab.size(); i++)
        {
            cout << "blob " << i << endl;
            cout << x.row(i).reshape(0, 2) << endl;
        }

result is

Original
 [128, 138, 225, 127, 137, 224, 126, 136, 224, 125, 135, 223;
 127, 137, 224, 127, 137, 224, 127, 137, 224, 126, 136, 224]
blob 0
[221, 266, 366, 220, 264, 363, 220, 262, 360, 219, 260, 357;
 220, 264, 363, 220, 264, 363, 220, 264, 363, 220, 262, 360]
blob 1
[663, 266, 122, 660, 264, 121, 660, 262, 120, 657, 260, 119;
 660, 264, 121, 660, 264, 121, 660, 264, 121, 660, 262, 120]
blob 2
[221, 266, 366, 220, 264, 363, 220, 262, 360, 219, 260, 357;
 220, 264, 363, 220, 264, 363, 220, 264, 363, 220, 262, 360]

zihaomu

LGTM! 👍 Thanks for your contribution.

CSBVision · 2023-08-08T15:20:35Z

Thanks for confirming - but this bug will be fixed by the PR as well 👍

zihaomu · 2023-08-08T23:37:22Z

Hi @CSBVision, can you add a test at https://github.com/opencv/opencv/blob/4.x/modules/dnn/test/test_misc.cpp#L66 so that we will not miss this case again in the future?

CSBVision · 2023-08-09T09:25:15Z

This is our proposal for a test:

Mat blobs = blobFromImagesWithParams(std::vector<Mat> { img, img }, param);
vector<Range> ranges;
ranges.push_back(Range(0, 1));
ranges.push_back(Range(0, blobs.size[1]));
ranges.push_back(Range(0, blobs.size[2]));
ranges.push_back(Range(0, blobs.size[3]));
Mat blob0 = blobs(ranges);
ranges[0] = Range(1, 2);
Mat blob1 = blobs(ranges);
Mat diff = blob0 - blob1;
diff = diff.reshape(1, 1);
double minVal, maxVal;
minMaxLoc(diff, &minVal, &maxVal);
EXPECT_EQ(minVal, maxVal);

Is this test fine for you?
If so, we would update the PR.

zihaomu · 2023-08-09T09:48:00Z

Hi @CSBVision, how about adding a new test case for this issue?
It's a good time to add the test for blobFromImagesWithParams(image-s).

TEST(blobFromImagesWithParams_4ch, multi_image)
{
    Mat img(10, 10, CV_8UC4, cv::Scalar(0,1,2,3));
    ...
}

CSBVision · 2023-08-09T10:01:44Z

Then we a arrive at the following additional function:

TEST(blobFromImagesWithParams_4ch, multi_image)
{
    Mat img(10, 10, CV_8UC4, cv::Scalar(0, 1, 2, 3));
    std::vector<double> factorVec = { 0.1, 0.2, 0.3, 0.4 };

    Scalar scalefactor(factorVec[0], factorVec[1], factorVec[2], factorVec[3]);

    Image2BlobParams param;
    param.scalefactor = scalefactor;
    param.datalayout = DNN_LAYOUT_NHWC;

    Mat blobs = blobFromImagesWithParams(std::vector<Mat> { img, img }, param);
    vector<Range> ranges;
    ranges.push_back(Range(0, 1));
    ranges.push_back(Range(0, blobs.size[1]));
    ranges.push_back(Range(0, blobs.size[2]));
    ranges.push_back(Range(0, blobs.size[3]));
    Mat blob0 = blobs(ranges);
    ranges[0] = Range(1, 2);
    Mat blob1 = blobs(ranges);
    Mat diff = blob0 - blob1;
    diff = diff.reshape(1, 1);
    double minVal, maxVal;
    minMaxLoc(diff, &minVal, &maxVal);
    EXPECT_EQ(minVal, maxVal);
}

Should we add this to the PR?

zihaomu · 2023-08-09T11:48:31Z

Hi @CSBVision, thanks for update. We can use the following code the check if the two Mat are the same. Other part looks good for me.

 EXPECT_EQ(0, cvtest::norm(image0, image1, NORM_INF));

CSBVision · 2023-08-09T12:23:55Z

Alright, just updated the PR.

asmorkalov · 2023-08-10T06:37:15Z

modules/dnn/src/dnn_utils.cpp

+    {
+        std::swap(mean[0], mean[2]);
+        std::swap(scalefactor[0], scalefactor[2]);
    }


I propose to add CV_LOG_WARNING for the else case to highlight the issue.

What do you think about that?

if (param.swapRB) { if (nch > 2) { std::swap(mean[0], mean[2]); std::swap(scalefactor[0], scalefactor[2]); } else { CV_LOG_WARNING(NULL, "Red/blue color swapping requires at least three image channels."); } }

asmorkalov · 2023-08-10T06:42:54Z

modules/dnn/src/dnn_utils.cpp

    {
        CV_Assert(scalefactor == Scalar::all(1.0) && "Scaling is not supported for CV_8U blob depth");
-        CV_Assert(param.mean == Scalar() && "Mean subtraction is not supported for CV_8U blob depth");
+        CV_Assert(mean == Scalar() && "Mean subtraction is not supported for CV_8U blob depth");


I propose to revert param.mean here. In case if assert user should see function parameter name, but not some internal variable,

We used this for consistency with the scalefactor check. If param.mean should be used, we should also use param.scalefactor, making both checks more explicit. Or what do you think?

you are right, both checks.

LaurentBerger · 2023-08-10T08:47:23Z

May it can be usefull to define in doc if mean and scalefactor are applied before channel are swap

CSBVision · 2023-08-10T09:03:28Z

May it can be usefull to define in doc if mean and scalefactor are applied before channel are swap

In the blobFromImage doc, it is already explained:

mean - scalar with mean values which are subtracted from channels. Values are intended to be in (mean-R, mean-G, mean-B) order if image has BGR ordering and swapRB is true.

zihaomu · 2023-08-10T09:46:39Z

Hi @CSBVision，

/home/ci/opencv/modules/dnn/src/dnn_utils.cpp:122:13: error: 'CV_LOG_WARNING' was not declared in this scope
  122 |             CV_LOG_WARNING(NULL, "Red/blue color swapping requires at least three image channels.");
      |             ^~~~~~~~~~~~~~

Please add the header file #include <opencv2/core/utils/logger.hpp>

CSBVision · 2023-08-10T11:02:08Z

Thanks for pointing out. We just added the lines from the comments without recompiling. Should be fine now 👍

asmorkalov

👍
I changed test a bit to have slightly different images in batch.

CSBVision · 2023-09-06T07:23:46Z

Alright, looks good👍
Btw: Would be great if you could fine the time to approve our two years old JPEG2000 PR (not linked here as it has nothing to do with this PR), too, such that we can soon delete all these old/merged branches on our side.

asmorkalov requested review from fengyuentau and zihaomu August 8, 2023 12:21

asmorkalov added the category: dnn label Aug 8, 2023

zihaomu approved these changes Aug 8, 2023

View reviewed changes

zihaomu changed the title ~~Update dnn_utils.cpp~~ Fix bug at blobFromImagesWithParams Aug 8, 2023

zihaomu added the bug label Aug 8, 2023

CSBVision force-pushed the CSBVision-patch-1 branch from 426a303 to cce0634 Compare August 9, 2023 12:23

asmorkalov reviewed Aug 10, 2023

View reviewed changes

CSBVision force-pushed the CSBVision-patch-1 branch 2 times, most recently from 7c91620 to 2b0ebfa Compare August 10, 2023 09:28

CSBVision force-pushed the CSBVision-patch-1 branch from 2b0ebfa to 01a7e0a Compare August 10, 2023 09:50

zihaomu requested a review from asmorkalov September 6, 2023 05:41

Update dnn_utils.cpp

674c618

asmorkalov force-pushed the CSBVision-patch-1 branch from 01a7e0a to 674c618 Compare September 6, 2023 07:01

asmorkalov approved these changes Sep 6, 2023

View reviewed changes

asmorkalov merged commit 5350fba into opencv:4.x Sep 6, 2023

asmorkalov self-assigned this Sep 6, 2023

asmorkalov added this to the 4.9.0 milestone Sep 6, 2023

asmorkalov mentioned this pull request Sep 11, 2023

(5.x) Merge 4.x #24254

Merged

Uh oh!

Conversation

CSBVision commented Aug 8, 2023 • edited by zihaomu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

blobFromImagesWithParams(image-s) apply swap scalefactor for every images when swapRB is true. This patch moves the swap scalefactor code out of the loop, that fix the above issue.

Pull Request Readiness Checklist

Uh oh!

CSBVision commented Aug 8, 2023

Uh oh!

LaurentBerger commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CSBVision commented Aug 8, 2023

Uh oh!

LaurentBerger commented Aug 8, 2023

Uh oh!

zihaomu left a comment

Choose a reason for hiding this comment

Uh oh!

CSBVision commented Aug 8, 2023

Uh oh!

zihaomu commented Aug 8, 2023

Uh oh!

CSBVision commented Aug 9, 2023

Uh oh!

zihaomu commented Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CSBVision commented Aug 9, 2023

Uh oh!

zihaomu commented Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CSBVision commented Aug 9, 2023

Uh oh!

asmorkalov Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

CSBVision Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

asmorkalov Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

CSBVision Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

asmorkalov Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

LaurentBerger commented Aug 10, 2023

Uh oh!

CSBVision commented Aug 10, 2023

Uh oh!

zihaomu commented Aug 10, 2023

Uh oh!

CSBVision commented Aug 10, 2023

Uh oh!

asmorkalov left a comment

Choose a reason for hiding this comment

Uh oh!

CSBVision commented Sep 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CSBVision commented Aug 8, 2023 •

edited by zihaomu

Loading

`blobFromImagesWithParams`(image-s) apply swap scalefactor for every images when swapRB is true. This patch moves the swap scalefactor code out of the loop, that fix the above issue.

LaurentBerger commented Aug 8, 2023 •

edited

Loading

zihaomu commented Aug 9, 2023 •

edited

Loading

zihaomu commented Aug 9, 2023 •

edited

Loading