Skip to content

Fixing dnn Resize layer for variable input size#18708

Closed
JulienMaille wants to merge 0 commit intoopencv:3.4from
JulienMaille:patch-2
Closed

Fixing dnn Resize layer for variable input size#18708
JulienMaille wants to merge 0 commit intoopencv:3.4from
JulienMaille:patch-2

Conversation

@JulienMaille
Copy link
Copy Markdown
Contributor

@JulienMaille JulienMaille commented Oct 30, 2020 •
edited by alalek
Loading

resolves #18695
merge with extra opencv/opencv_extra#816

opencv_extra=patch-1

force_builders_only=linux,docs

Xforce_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=*

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Nov 16, 2020

@JulienMaille Looks good. Could you add simple test for the issue?

@asmorkalov asmorkalov added category: dnn pr: needs test New functionality requires minimal tests set labels Nov 16, 2020
@JulienMaille JulienMaille force-pushed the patch-2 branch 6 times, most recently from 2ce5db3 to f02c0e4 Compare November 16, 2020 21:20
sl-sergei
sl-sergei approved these changes Nov 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@sl-sergei sl-sergei left a comment

Choose a reason for hiding this comment

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

This duplicates part of my changes from #18145 :(
Anyway, looks good, thank you!
The only thing I want to change there is s target branch, I think it should be 3.4 instead of master

@JulienMaille
Copy link
Copy Markdown
Contributor Author

JulienMaille commented Nov 16, 2020

@sl-sergei :( did I waste my time? Can you check your own PR passes the tests I have added?

@sl-sergei
Copy link
Copy Markdown
Contributor

sl-sergei commented Nov 16, 2020

@sl-sergei :( did I waste my time? Can you check your own PR passes the tests I have added?

No, you didn't waste time, because my changes are conditioned on ONNX "dynamic_axes" option (thus your tests will pass only under this condition). I planned to extend my work further to make the fixes unconditional.
So, obviously, this PR will be merged first, you did good job 👍

@alalek
Copy link
Copy Markdown
Member

alalek commented Nov 16, 2020

@JulienMaille
This patch should go into 3.4 branch first.
We will merge changes from 3.4 into master regularly (weekly/bi-weekly).

So, please:

  • change "base" branch of this PR: master => 3.4 (use "Edit" button near PR title)
  • rebase your commits from master onto 3.4 branch. For example:
    git rebase -i --onto upstream/3.4 upstream/master
    (check list of your commits, save and quit (Esc + "wq" + Enter)
    where upstream is configured by following this GitHub guide and fetched (git fetch upstream).
  • push rebased commits into source branch of your fork (with --force option)

Note: no needs to re-open PR, apply changes "inplace".

@JulienMaille
Copy link
Copy Markdown
Contributor Author

JulienMaille commented Nov 16, 2020

I will rebase as soon as I can fix the failing tests. Handling all cases is harder than expected

@l-bat l-bat mentioned this pull request Nov 17, 2020
Merged
l-bat
l-bat approved these changes Nov 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@l-bat l-bat 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 Nov 17, 2020

@JulienMaille The Solution introduces several test failures on all platforms. Please take a look: https://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/28872

@JulienMaille
Copy link
Copy Markdown
Contributor Author

JulienMaille commented Nov 17, 2020 •
edited
Loading

@JulienMaille The Solution introduces several test failures on all platforms. Please take a look: https://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/28872

Yes, I know, and I'm a bit struggling with them. For instance I get this error on 'Upsample' which has not been changed by my PR:

[ERROR:0] onnx_importer.cpp (1798) handleNode DNN/ONNX: ERROR during processing node with 2 inputs and 1 outputs: [Upsample]:(10)
[ INFO:0] onnx_importer.cpp (1801) handleNode     Input[0] = 'input'
[ INFO:0] onnx_importer.cpp (1801) handleNode     Input[1] = '16'
[ INFO:0] onnx_importer.cpp (1805) handleNode     Output[0] = '10'
unknown file: Failure
C++ exception onnx_importer.cpp:1807: error: (-2:Unspecified error) in function 'handleNode'
> Node [Upsample]:(10) parse error: onnx_importer.cpp:330: error: (-5:Bad argument) Blob 16 not found in const blobs in function 'getBlob'

on this model
image

@JulienMaille
Copy link
Copy Markdown
Contributor Author

JulienMaille commented Nov 17, 2020

@l-bat Can you explain me what is this assert doing? Thanks in advance

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

Lines 1674 to 1677 in b5c1621

else if (layer_type == "Resize")
{
for (int i = 1; i < node_proto.input_size(); i++)
CV_Assert(layer_id.find(node_proto.input(i)) == layer_id.end());

@sl-sergei
Copy link
Copy Markdown
Contributor

sl-sergei commented Nov 17, 2020 •
edited
Loading

@l-bat Can you explain me what is this assert doing? Thanks in advance

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

Lines 1674 to 1677 in b5c1621

else if (layer_type == "Resize")
{
for (int i = 1; i < node_proto.input_size(); i++)
CV_Assert(layer_id.find(node_proto.input(i)) == layer_id.end());

This assert checks if the input tensors (outputs of the previous layers) are already added to the graph

@JulienMaille JulienMaille force-pushed the patch-2 branch 4 times, most recently from 43c765d to 7bfa053 Compare November 17, 2020 13:21
@JulienMaille
Copy link
Copy Markdown
Contributor Author

JulienMaille commented Nov 17, 2020

@l-bat Can you explain me what is this assert doing? Thanks in advance
This assert checks if the input tensors (outputs of the previous layers) are already added to the graph

How do you handle if they're not? This assert is triggered on one of the new tests

@l-bat
Copy link
Copy Markdown
Contributor

l-bat commented Nov 17, 2020

This assert checks that all inputs of node are constant and we can get them using getBlob. You can remove it and handle cases with variable inputs.

@sl-sergei
Copy link
Copy Markdown
Contributor

sl-sergei commented Nov 17, 2020

@l-bat Can you explain me what is this assert doing? Thanks in advance
This assert checks if the input tensors (outputs of the previous layers) are already added to the graph

How do you handle if they're not? This assert is triggered on one of the new tests

The answer above is correct, sorry for my mistake(

@JulienMaille JulienMaille force-pushed the patch-2 branch 2 times, most recently from 4e16909 to 95ed5a1 Compare November 17, 2020 14:14
@JulienMaille
Copy link
Copy Markdown
Contributor Author

JulienMaille commented Nov 17, 2020

@l-bat @sl-sergei I'm sorry I'm getting lost, specifically with input existing but not being accesible to getBlob()

[ERROR:0] onnx_importer.cpp (1798) handleNode DNN/ONNX: ERROR during processing node with 2 inputs and 1 outputs: [Upsample]:(10)
[ INFO:0] onnx_importer.cpp (1801) handleNode     Input[0] = 'input'
[ INFO:0] onnx_importer.cpp (1801) handleNode     Input[1] = '16'
[ INFO:0] onnx_importer.cpp (1805) handleNode     Output[0] = '10'
unknown file: Failure
C++ exception with description "OpenCV(4.5.0-dev) onnx_importer.cpp:1807: error: (-2:Unspecified error) in function 'handleNode'
> Node [Upsample]:(10) parse error: OpenCV(4.5.0-dev) onnx_importer.cpp:330: error: (-5:Bad argument) Blob 16 not found in const blobs in function 'getBlob'

Can you help?

@JulienMaille JulienMaille changed the base branch from master to 3.4 November 17, 2020 20:12
@JulienMaille JulienMaille mentioned this pull request Nov 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@l-bat l-bat l-bat approved these changes

+1 more reviewer

@sl-sergei sl-sergei sl-sergei approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug category: dnn

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

onnx 12: Inconsistent shape for ConcatLayer in function 'cv::dnn::ConcatLayerImpl::getMemoryShapes'

5 participants

@JulienMaille @asmorkalov @sl-sergei @alalek @l-bat