Fixes matrix expression bug which happens when Scalar in addition pas…

[saskatchewancatch]

…ses isReal test
Fixes #16538
Unit test is under work. Coming shortly.

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 OpenCV (BSD) License.
• To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
• The PR is proposed to proper branch
• There is reference to 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

[saskatchewancatch]

I missed the superres test on my VM, seems it was disabled because of long-ness.

Problem is here where SSIM is calculated for the test case.

I'll take a look.

[asmorkalov]

@saskatchewancatch Could add test for the bug to prove the solution?

[alalek]

Some code for tests:

    Mat m0(Size(5, 3), CV_8UC3, Scalar(1,2,3));

    {
        Mat m = m0 + 5;
        cout << m << endl;
    }
    {
        Mat m = 2 * m0 + 5;
        cout << m << endl;
    }
    {
        Mat m = m0 + 5 * Scalar(2, 1, 0);
        cout << m << endl;
    }
#if 0 // can't compile
    {
        Mat m = 2 * m0 + 5 * Scalar(2, 1, 0);
        cout << m << endl;
    }
#endif

Current 3.4.9/4.2.0 output

[  6,   2,   3,   6,   2,   3,   6,   2,   3,   6,   2,   3,   6,   2,   3;
   6,   2,   3,   6,   2,   3,   6,   2,   3,   6,   2,   3,   6,   2,   3;
   6,   2,   3,   6,   2,   3,   6,   2,   3,   6,   2,   3,   6,   2,   3]
[  7,   9,  11,   7,   9,  11,   7,   9,  11,   7,   9,  11,   7,   9,  11;
   7,   9,  11,   7,   9,  11,   7,   9,  11,   7,   9,  11,   7,   9,  11;
   7,   9,  11,   7,   9,  11,   7,   9,  11,   7,   9,  11,   7,   9,  11]
[ 11,   7,   3,  11,   7,   3,  11,   7,   3,  11,   7,   3,  11,   7,   3;
  11,   7,   3,  11,   7,   3,  11,   7,   3,  11,   7,   3,  11,   7,   3;
  11,   7,   3,  11,   7,   3,  11,   7,   3,  11,   7,   3,  11,   7,   3]

[saskatchewancatch]

core/test/test_operations.cpp's CV_OperationsTest::TestMat() is a good place to put the test. Thoughts?

I propose integrating alalek's suggestion like such:

diff --git a/modules/core/test/test_operations.cpp b/modules/core/test/test_operations.cpp
index dd91617537..d2ff870559 100644
--- a/modules/core/test/test_operations.cpp
+++ b/modules/core/test/test_operations.cpp
@@ -292,6 +292,16 @@ bool CV_OperationsTest::TestMat()
         CHECK_DIFF(4.0 * (maskMat1 | maskMat1), maskMat4);
         CHECK_DIFF((maskMat4 | maskMat4)/4.0, maskMat1);

+        //Multi-channel Scalar addition tests
+        Mat multiChannel_2x2__1_2_3(Size(2, 2), CV_8UC3, Scalar(1, 2, 3));
+        Mat multiChannel_2x2__6_2_3(Size(2, 2), CV_8UC3, Scalar(6, 2, 3));
+        Mat multiChannel_2x2__7_9_11(Size(2, 2), CV_8UC3, Scalar(7, 9, 11));
+        Mat multiChannel_2x2__11_7_3(Size(2, 2), CV_8UC3, Scalar(11, 7, 3));
+
+        CHECK_DIFF(multiChannel_2x2__6_2_3, multiChannel_2x2__1_2_3 + 5);
+        CHECK_DIFF(multiChannel_2x2__7_9_11, 2 * multiChannel_2x2__1_2_3 + 5);
+        CHECK_DIFF(multiChannel_2x2__11_7_3, multiChannel_2x2__1_2_3 + 5 * Scalar(2, 1, 0));
+
 #if !MSVC_OLD
         CHECK_DIFF(2.0 * (maskMat1 * 2.0) , maskMat4);
 #endif

[alalek]

test_operations.cpp

Right.

It is better to create separate test case (it is hard to maintain or debug huge tests), like:

TEST(Core_MatExpr, issue_16538)
{
    ...
}

[saskatchewancatch]

Thank you for test suggestions @alalek

Between the tests you provided and the ones that already exist in test_operations.cpp, the optimization branches that were touched in this PR are tested.

[alalek]

Both code branches are equal to:

cv::add(dst, e.s[0], dst);

so proposed condition doesn't make sense.

The real problem is how input "double" is stored through Scalar:

• 5 is Scalar(5, 0, 0, 0)
• or 5 is Scalar:all(5)
• also tests should properly handle + Scalar(5, 0, 0, 0) and Scalar:all(5)

.isReal() check is not correct if we try to use Scalar::all(5) approach.
Probably we should raise exceptions on operations like multichannel_mat + just scalar.

[saskatchewancatch]

@alalek Thanks for review, to address your feedback:

• Yes, that if/else is redundant. I will just replace with cv::add(dst, e.s, dst); This is what will properly handle OP's multi-channel Mat + multi-channel scalar issue.

• Raising exception in case of mulitchannel_mat + just_scalar is fine imo but I think it's better handled in a separate PR, where isReal() issue can be fully sorted out. That may take longer as the solution cannot be implemented inside matrix_expressions.cpp and probably requires handling inside lower level arithmetic operation handlers.

• As for this PR, I will change the tests above so that the just scalar is changed to the Scalar form: ex) res = multiChannel_2x2__1_2_3 + Scalar(5, 0, 0);, and then add the tests Scalar::all test you suggested.

This PR at least will fix OP's alpha blending issue and strengthen against regressions with the added tests. Is this fine?

[alalek]

Patch is fine.
But it explodes another problem: multiChannel + 5.0 result is not well defined.
Need to take a look on this in details.

[saskatchewancatch]

The superres test is the problem. In the test, a multichannel matrix is being added to a double constant. As per issue 16739, and above discussion, this is a grey area. I propose this adjustment for test_superres.cpp to make the addition with the pure scalar explicit.

diff --git a/modules/superres/test/test_superres.cpp b/modules/superres/test/test_superres.cpp
index b0d4122f6e..b98258484f 100644
--- a/modules/superres/test/test_superres.cpp
+++ b/modules/superres/test/test_superres.cpp
@@ -154,8 +154,8 @@ void DegradeFrameSource::reset()

 double MSSIM(cv::InputArray _i1, cv::InputArray _i2)
 {
-    const double C1 = 6.5025;
-    const double C2 = 58.5225;
+    const Scalar C1(6.5025, 6.5025, 6.5025);
+    const Scalar C2(58.5225, 58.5225, 58.5225);

     const int depth = CV_32F;

This makes all tests pass.
Until we figure out how to deal with issue 16739, this seems sane to me.

[alalek]

The superres test is the problem

Right.
Unfortunately, similar users code may exist too, but there is no way to fix that (and probably properly warn about that).

[asenyaev]

jenkins cn please retry a build