Skip to content

Remove asymmetric padding in Conv layer since it is supported in CPU backend#22362

Merged
asmorkalov merged 3 commits intoopencv:4.xfrom
fengyuentau:conv_asym_pad_fuse
Aug 29, 2022
Merged

Remove asymmetric padding in Conv layer since it is supported in CPU backend#22362
asmorkalov merged 3 commits intoopencv:4.xfrom
fengyuentau:conv_asym_pad_fuse

Conversation

@fengyuentau
Copy link
Copy Markdown
Member

@fengyuentau fengyuentau commented Aug 10, 2022 •
edited by asmorkalov
Loading

In #21910, fast convolution is introduced with support for asymmetric padding in calculation. So there is no need to keep a separate padding layer for asymmetric padding in convolution layer.

Regression test for convolution layer with asymmetric padding:

opencv/modules/dnn/test/test_onnx_importer.cpp

Lines 118 to 122 in d09cc0f

TEST_P(Test_ONNX_layers, Convolution)
{
testONNXModels("convolution");
testONNXModels("conv_asymmetric_pads");
}

OpenVINO, Default CPU (CPU, OCL, Tengine), Vulkan, CUDA, WebNN are registered for this test.

Checks on different backends

  • OpenCV CPU
  • OpenCL
  • OpenVINO (should be good since it builds its padding from pads_begin & pads_end, see code here and here)
  • CUDA
  • Vulkan I am getting segmentation fault regardless my changes
  • WebNN (should be good since it builds its padding from pads_begin & pads_end, see code here)
  • Tengine (failed at Test_ONNX_layers.Convolution_variable_weight_bias/0 but should not be related to this patch)

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

@fengyuentau fengyuentau changed the title Remove asymmetric padding checks in Conv layer since it is supported in CPU backend Remove asymmetric padding in Conv layer since it is supported in CPU backend Aug 10, 2022
@asmorkalov asmorkalov requested a review from rogday August 10, 2022 15:18
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Aug 26, 2022

I tried the branch with TEngine enabled on my Jetson TK1 (armv7 with neon, Ubuntu 14.04) and minimal dnn test suite has a failure:

[ SKIPSTAT ] 947 tests skipped
[ SKIPSTAT ] TAG='mem_2gb' skip 21 tests
[ SKIPSTAT ] TAG='verylong' skip 1 tests
[ SKIPSTAT ] TAG='dnn_skip_opencv_backend' skip 57 tests
[ SKIPSTAT ] TAG='dnn_skip_onnx_conformance' skip 0 tests (616 times in extra skip list)
[ SKIPSTAT ] TAG='dnn_skip_parser' skip 559 tests
[ SKIPSTAT ] TAG='skip_other' skip 309 tests
[==========] 2619 tests from 80 test cases ran. (4368 ms total)
[  PASSED  ] 2618 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] Test_ONNX_layers.Convolution_variable_weight_bias/0, where GetParam() = OCV/CPU

I'll check, if it relates somehow to the patch and let you know.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Aug 26, 2022

The same issue in 4.x with TEngine:

[----------] Global test environment tear-down
[ SKIPSTAT ] 947 tests skipped
[ SKIPSTAT ] TAG='mem_2gb' skip 21 tests
[ SKIPSTAT ] TAG='verylong' skip 1 tests
[ SKIPSTAT ] TAG='dnn_skip_opencv_backend' skip 57 tests
[ SKIPSTAT ] TAG='dnn_skip_onnx_conformance' skip 0 tests (616 times in extra skip list)
[ SKIPSTAT ] TAG='dnn_skip_parser' skip 559 tests
[ SKIPSTAT ] TAG='skip_other' skip 309 tests
[==========] 2619 tests from 80 test cases ran. (4250 ms total)
[  PASSED  ] 2618 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] Test_ONNX_layers.Convolution_variable_weight_bias/0, where GetParam() = OCV/CPU

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Aug 26, 2022

The issue is TEngine specific and is not reproducible without it:

[----------] Global test environment tear-down
[ SKIPSTAT ] 947 tests skipped
[ SKIPSTAT ] TAG='mem_2gb' skip 21 tests
[ SKIPSTAT ] TAG='verylong' skip 1 tests
[ SKIPSTAT ] TAG='dnn_skip_opencv_backend' skip 57 tests
[ SKIPSTAT ] TAG='dnn_skip_onnx_conformance' skip 0 tests (616 times in extra skip list)
[ SKIPSTAT ] TAG='dnn_skip_parser' skip 559 tests
[ SKIPSTAT ] TAG='skip_other' skip 309 tests
[==========] 2619 tests from 80 test cases ran. (3999 ms total)
[  PASSED  ] 2619 tests.

@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented Aug 26, 2022 •
edited
Loading

@asmorkalov Thank you for the test! I will take a look at the problem. But let me clarify, do you mean Tengine (Build with WITH_TENGINE) by TEngine?

opencv/CMakeLists.txt

Lines 453 to 455 in 3651831

OCV_OPTION(WITH_TENGINE "Include Arm Inference Tengine support" OFF
VISIBLE_IF (ARM OR AARCH64) AND (UNIX OR ANDROID) AND NOT IOS
VERIFY HAVE_TENGINE)

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Aug 26, 2022

Yes, I mean WITH_TENGINE option:

cmake -DWITH_TENGINE=ON -DCPU_BASELINE_REQUIRE=NEON -DBUILD_opencv_gapi=OFF ../opencv

@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented Aug 29, 2022 •
edited
Loading

@asmorkalov Convolution with asymmetric padding in Tengine is now supported. However, I did not find the root problem in Test_ONNX_layers.Convolution_variable_weight_bias/0. The model in this unit test has no padding, and I can confirm the weight & bias are all correct before they are converted to Tengine tensors.

Hello @liqi-c , we find the tengine convolution failed at Test_ONNX_layers.Convolution_variable_weight_bias/0 in OpenCV DNN. Could you take a look at this issue?

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Aug 29, 2022

Previous work and tests for asymmetric padding support: #20964

@asmorkalov asmorkalov removed the pr: needs test New functionality requires minimal tests set label Aug 29, 2022
rogday
rogday approved these changes Aug 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@rogday rogday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Aug 29, 2022

Tested with the last 2 releases of OpenVINO. No new regressions in comparison to 4.x.

@asmorkalov asmorkalov merged commit 1fd45a1 into opencv:4.x Aug 29, 2022
@alalek alalek mentioned this pull request Jan 8, 2023
Merged
@asmorkalov asmorkalov added this to the 4.7.0 milestone Jan 23, 2023
@fengyuentau fengyuentau deleted the conv_asym_pad_fuse branch March 14, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@rogday rogday rogday approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rogday rogday

Labels

category: dnn feature

Projects

None yet

Milestone

4.7.0

Development

Successfully merging this pull request may close these issues.

4 participants

@fengyuentau @asmorkalov @rogday @alalek