Added TF add behaviour alignment by LupusSanctus · Pull Request #19477 · opencv/opencv

LupusSanctus · 2021-02-07T22:03:51Z

Merge with extra: opencv/opencv_extra#849

Fixes #17364: inappropriate OpenCV processing of specific tensor summation case in LaneNet BiseNetv2 front end.

Added tf.math.add and OCV eltwise_layer behaviour alignment.

The initial problem problem is incorrect processing of the case, when, for example, there are two tensors: a (shape: [1,1,1,n] ) and b (shape: [1,<some_val_1>,<some_val_2>,n] ). tf.math.add returns the result c (shape: [1,<some_val_1>,<some_val_2>,n] ), whereas OpenCV compares 1 vs <some_val_1> and 1 vs <some_val_2>, throws exception in getMemoryShapes() and that's all.
The solution for such cases (for example, for tensor shape 1x10x1x1 (NxCxHxW) +1x10x5x5 (NxCxHxW)) was:

choose the correct output resultant shape: 1x10x5x5 (NxCxHxW)
expand 1x10x1x1 (NxCxHxW) for further correct summation with 1x10x5x5 (NxCxHxW).
The initial problem problem is incorrect processing of the case, when, for example, there are two tensors: a (shape: [1,1,1,n] ) and b (shape: [1,<some_val_1>,<some_val_2>,n] ). tf.math.add returns the result c (shape: [1,<some_val_1>,<some_val_2>,n] ), whereas OpenCV compares 1 vs <some_val_1> and 1 vs <some_val_2>, throws exception in getMemoryShapes() and that's all.
The solution for such cases (for example, for tensor shape 1x10x1x1 (NxCxHxW) +1x10x5x5 (NxCxHxW)) was:
choose the correct output resultant shape: 1x10x5x5 (NxCxHxW)
expand 1x10x1x1 (NxCxHxW) for further correct summation with 1x10x5x5 (NxCxHxW).

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 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

force_builders=Custom,Custom Win,Custom Mac
build_image:Custom=ubuntu-openvino-2021.1.0:20.04
build_image:Custom Win=openvino-2021.1.0
build_image:Custom Mac=openvino-2021.1.0

test_modules:Custom=dnn,python2,python3,java
test_modules:Custom Win=dnn,python2,python3,java
test_modules:Custom Mac=dnn,python2,python3,java

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

dkurt · 2021-02-08T07:53:16Z

Please choose 3.4 as a target branch (more details in #19474 (comment) )

modules/dnn/src/layers/eltwise_layer.cpp

alalek

Could we add explicit input parameter which changes operating mode?

We need to ensure that "layers fusing" is still properly handled (fused code may not call the ".forward()" method).

modules/dnn/src/layers/eltwise_layer.cpp

alalek · 2021-02-19T07:15:59Z

modules/dnn/src/layers/eltwise_layer.cpp

+                            for (size_t x = 0; x < xSize; x++)
+                            {
+                                outIdx[2] = x;
+                                inputs[i].at<float>(outIdx.data()) = tmpInput.at<float>(idx.data());


IMHO, generating duplicated data doesn't look good from performance perspective.

@alalek, there are two options to resolve the issue:

support Mat (shape: [1, m, k, n] ) + Vec (shape: [1, 1, 1, n]) operation in multiple implementations (ex.: OCL), which is quite complex and error-prone

expand Vec (shape: [1, 1, 1, n]) to Mat to support a new case using an existing implementation at the expense of extra memory usage

alalek · 2021-02-19T07:21:16Z

modules/dnn/src/layers/eltwise_layer.cpp

+                if (!allOnes && !isVecFound)
+                {
+                    vecIdx = i;
+                    isVecFound = true;


IMHO, such stuff should be explicitly specified through parameters instead of implicit internal handling.

@alalek, it seems that ''explicit input parameter'' is an extra option. This PR adds support Mat (shape: [1, m, k, n] ) + Vec (shape: [1, 1, 1, n]). This support was implemented with Vec (shape: [1, 1, 1, n]) to Mat expansion to avoid possible reconstruction of eltwise_layer.cpp summation core. Thus, the current approach should not lead to further corruptions:

We need to ensure that "layers fusing" is still properly handled (fused code may not call the ".forward()" method).

Main concern here is about correct and accurate implementation of supportBackend() callback.

Only OpenCV/CPU backend supports new mode (CUDA, Halide, Vulkan, even OpenCV/OpenCL doesn't support that).
Unsupported backends should be filtered out in the right way.

@alalek, could you, please, clarify the following: if we don't change the computation core (forward(...) and run(...) functions were not changed) and modify only one of the inputs' shape, expanding it in getMemoryShapes(...), we should not impact on the chosen backend. Thus, do we really need supportBackend() callback corrections here?

forward() is modified below, see if (channelsModeInput == ELTWISE_CHANNNELS_SAME && inputs[0].dims > 2)

Tests filter out non-working "OpenCL" cases (as mentioned there this should not be done for OpenCV/OCL code path)

modify only one of the inputs' shape

Probably we can't do that. Input is always a some output of another layer.

Thus, do we really need supportBackend() callback corrections here?

supportBackend() must be revised if we add support for new mode.

@alalek, thank you for the review, added corrections for supportBackend().

modules/dnn/src/layers/eltwise_layer.cpp

asmorkalov · 2021-03-19T05:45:41Z

@dkurt @alalek Please take a look.

alalek · 2021-03-23T09:12:32Z

modules/dnn/src/layers/eltwise_layer.cpp

        CV_Assert(outputs.size() == 1);
        const int nstripes = getNumThreads();
+
+        if (channelsModeInput == ELTWISE_CHANNNELS_SAME && inputs[0].dims > 2)


Please add bailout code in forward_ocl() (line 546) for new functionality:

if ((inputs_.depth() == CV_16S && op != SUM) || (channelsMode != ELTWISE_CHANNNELS_SAME)) return false; + if (hasVecInput) + return false; // TODO not implemented yet: https://github.com/opencv/opencv/pull/19477

@alalek, forward_ocl() was corrected.

Support Mat (shape: [1, m, k, n] ) + Vec (shape: [1, 1, 1, n]) operation by vec to mat expansion

alalek

Looks good to me! Thank you 👍

asmorkalov added the category: dnn label Feb 8, 2021

LupusSanctus force-pushed the am/eltwice_vec branch from 17f89b3 to 8bbd282 Compare February 8, 2021 10:29

LupusSanctus changed the base branch from master to 3.4 February 8, 2021 10:30

LupusSanctus force-pushed the am/eltwice_vec branch from d2d4c19 to fe28986 Compare February 8, 2021 15:53

asmorkalov requested a review from dkurt February 10, 2021 08:14

dkurt reviewed Feb 10, 2021

View reviewed changes

modules/dnn/src/layers/eltwise_layer.cpp Show resolved Hide resolved

alalek reviewed Feb 19, 2021

View reviewed changes

LupusSanctus force-pushed the am/eltwice_vec branch from 9077b10 to c496eba Compare February 28, 2021 20:41

asmorkalov requested review from alalek and dkurt March 10, 2021 06:20

alalek reviewed Mar 23, 2021

View reviewed changes

Anastasia Murzova added 2 commits March 23, 2021 23:04

Aligned OpenCV DNN and TF sum op behaviour

ca050a9

Support Mat (shape: [1, m, k, n] ) + Vec (shape: [1, 1, 1, n]) operation by vec to mat expansion

Added code corrections: backend, minor refactoring

5718f8a

LupusSanctus force-pushed the am/eltwice_vec branch from fea5e3d to 5718f8a Compare March 23, 2021 20:09

alalek approved these changes Mar 23, 2021

View reviewed changes

alalek merged commit 551d4a8 into opencv:3.4 Mar 23, 2021

This was referenced Mar 23, 2021

Corrected DNN elementwise multiplication #19765

Merged

(4.x) Merge 3.4 #19775

Merged

alalek mentioned this pull request Apr 9, 2021

(5.x) Merge 4.x #19885

Merged

Uh oh!

Conversation

LupusSanctus commented Feb 7, 2021 • edited by asenyaev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

dkurt commented Feb 8, 2021

Uh oh!

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alalek Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

LupusSanctus Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalek Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

LupusSanctus Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalek Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

LupusSanctus Mar 19, 2021

Choose a reason for hiding this comment

Uh oh!

alalek Mar 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LupusSanctus Mar 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asmorkalov commented Mar 19, 2021

Uh oh!

alalek Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

LupusSanctus Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LupusSanctus commented Feb 7, 2021 •

edited by asenyaev

Loading

LupusSanctus Feb 28, 2021 •

edited

Loading

LupusSanctus Feb 28, 2021 •

edited

Loading

alalek Mar 20, 2021 •

edited

Loading