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 Macbuild_image:Custom=ubuntu-openvino-2021.2.0:20.04build_image:Custom Win=openvino-2021.1.0build_image:Custom Mac=openvino-2021.2.0test_modules:Custom=dnn,python2,python3,javatest_modules:Custom Win=dnn,python2,python3,javatest_modules:Custom Mac=dnn,python2,python3,javabuildworker:Custom=linux-1# disabled due high memory usage: test_opencl:Custom=ONtest_opencl:Custom=OFFtest_bigdata:Custom=1test_filter:Custom=*

This was referencedFeb 16, 2021

Added Steps support in DNN Slice layeropencv/opencv_extra#854

Merged

ONNX implementation does not support slice layer with steps != 1#18920

Closed

asmorkalov requested review fromdkurt andsl-sergei

February 17, 2021 06:39

sl-sergei approved these changes

Feb 20, 2021

View reviewed changes

alalek assignedsl-sergei and unassignedsl-sergei

Feb 20, 2021

alalek reviewed

Feb 20, 2021

View reviewed changes

modules/dnn/src/layers/slice_layer.cpp OutdatedShow resolvedHide resolved

dkurt reviewed

Feb 24, 2021

View reviewed changes

modules/dnn/src/layers/slice_layer.cpp OutdatedShow resolvedHide resolved

LupusSanctus force-pushed theam/slice_steps branch 2 times, most recently from96eaa99 toaf88ecfCompare

February 28, 2021 21:51

Copy link

Author

LupusSanctus commentedMar 3, 2021

@alalek, the code was corrected in accordance with comments. Is it needed to rebase this patch on 3.4 branch?

Copy link

Member

alalek commentedMar 3, 2021

Is it needed to rebase this patch on 3.4 branch

We will backport it separately later (there is some CUDA-specific code).

Copy link

Contributor

asmorkalov commentedMar 9, 2021

/cc@asenyaev

LupusSanctus force-pushed theam/slice_steps branch fromaf88ecf to105dfb0Compare

March 9, 2021 20:41

LupusSanctus changed the base branch frommaster to3.4

March 9, 2021 20:41

Copy link

Author

LupusSanctus commentedMar 9, 2021

@alalek, rebased on 3.4 branch.

alalek reviewed

Mar 10, 2021

View reviewed changes

Copy link

Member

alalek left a comment

Choose a reason for hiding this comment

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

Thank you for update!

modules/dnn/src/layers/slice_layer.cpp

		outputs[i][j] =normalize_axis_range(sliceRanges[i][j], inpShape[j]).size();

		if (! sliceSteps.empty() && sliceSteps[i][j] >1)
		outputs[i][j] = (outputs[i][j] + sliceSteps[i][j] -1) / sliceSteps[i][j];

Copy link

Member

Choose a reason for hiding this comment

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

Please add out of range checks forsliceSteps[i][j] access.

Copy link

Author

Choose a reason for hiding this comment

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

@alalek, checks were added.

modules/dnn/src/layers/slice_layer.cpp Outdated

Comment on lines 627 to 630

		if (dim +1 < dimsNum)
		getSliceRecursive(inpMat, inpIdx, sliceRanges, sliceSteps, dim +1, dimsNum, outputs, outIdx);
		else
		outputs.at<float>(outIdx.data()) = inpMat.at<float>(inpIdx.data());

Copy link

Member

Choose a reason for hiding this comment

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

IMHO, provided implementation is really slow.

I believe we should have optimized case for 2D (the last 2 dims) processing at least.

Copy link

Author

Choose a reason for hiding this comment

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

@alalek, yes, but if it's ok with you, let's separate this optimization activity and implement it not in this scope.

modules/dnn/src/onnx/onnx_importer.cpp Outdated

Comment on lines 651 to 656

		DictValuesteps_dict = layerParams.get("steps");
		for (int i =0; i <steps_dict.size(); ++i)
		{
		if (steps.get<int>(i) !=1)
		if (steps_dict.get<int>(i) !=1) {
		CV_Error(Error::StsNotImplemented,
		"Slice layer only supports steps = 1");

Copy link

Member

Choose a reason for hiding this comment

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

Do we want to support this case?

Copy link

Author

LupusSanctusMar 25, 2021•
edited
Loading

Choose a reason for hiding this comment

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

@alalek, this case could be useful for opset9, which doesn't support step!=1, however, it means that initially onnx model could not be generated with step value != 1, hence, this check is useless.
Just in case, I've checked the model from theissue (which relates to the PR, where this code block was added), there is no step parameter (in opset9 slice layer "no step parameter" => step=1). Removed this code block.

modules/dnn/test/test_onnx_importer.cpp Outdated

Comment on lines 626 to 627

		if (backend == DNN_BACKEND_OPENCV)
		applyTestTag(target == DNN_TARGET_OPENCL ? CV_TEST_TAG_DNN_SKIP_OPENCL : CV_TEST_TAG_DNN_SKIP_OPENCL_FP16);

Copy link

Member

alalekMar 10, 2021

Choose a reason for hiding this comment

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

Implementation should fallback on CPU code instead of test skipping.

Copy link

Author

LupusSanctusMar 25, 2021

Choose a reason for hiding this comment

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

@alalek, corrected.

Anastasia Murzova added2 commits

March 24, 2021 23:02

Added Steps support in DNN Slice layer

4fa115c

Added code corrections

303fcbc

LupusSanctus force-pushed theam/slice_steps branch from105dfb0 to303fcbcCompare

March 25, 2021 20:15

dnn(slice): fix OCL and OCL_FP16 processing

626eaef

alalek approved these changes

Mar 26, 2021

View reviewed changes

alalek assignedsl-sergei

Mar 26, 2021

alalek merged commit3e48a91 intoopencv:3.4

Mar 26, 2021

Copy link

Contributor

OlivierLDff commentedMar 26, 2021•
edited
Loading

Hi, is there any plan to merge this in master?
I merged it in my fork to try to load yolov5 dnn. This work but i have different result in i run the inference on cpu or with cuda backend.
Have you tried CUDA backend with this and yolov5 model?
The result are coherent with OPENCV backend, but with CUDA result are a bit "off" and are scaled x2.
Should i open an issue, or is this something known?
Thanks for the great work.

Copy link

Member

alalek commentedMar 26, 2021

This patch will be merged to master, but without the CUDA part (CUDA code path will fallback on CPU - it works, but slow)

Copy link

Contributor

OlivierLDff commentedMar 26, 2021

Ok, is there any issue i can follow about this?
Why no CUDA? is there any plan? As you said CPU backend doesn't fit real time application.

Copy link

Member

alalek commentedMar 26, 2021

Fallback CUDA-CPU-CUDA is slow due to data transfers and synchronization points.

Currently CUDA optimizations are maintained by community, so feel free to propose a patch after the merge to master (wait for "(4.x) Merge 3.4" regular PR).