| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
[G-API]: Adding reshape for CNN input. #18240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[G-API]: Adding reshape for CNN input. #18240
Conversation
|
@OrestChura, @aDanPin, please review. |
mpashchenkov
force-pushed
the
mp/ocv-gapi-input-cnn-reshape
branch
from
20d59f4 to
d1887e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks!
But please check if PR causes IE tests failure
So what is |
|
..also, if this function is not called for all N inputs, then I'd limit it to configuring a single input only. |
| Params<Net>& cfgInputReshape(const std::map<std::string, std::vector<std::size_t>> &r) { | ||
| desc.reshape_inputs = r; | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really I'd add an overload with const std::string, const std::vector<size_t>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpashchenkov what about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider to add rvalue implementation, because it will be used in most of cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added overloads.
| // cv::Mat-wrapped blob there to support IE's optimal "GetBlob idiom" | ||
| // Still, constant data is to set only once. | ||
| this_request.SetBlob(p.first, wrapIE(p.second.first, p.second.second)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did it go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a duplicate code. Already fixed.
| // Reshape for configured map with layers' names and their new shapes | ||
| if (!params.reshape_inputs.empty()) { | ||
| GAPI_Assert(params.reshape_inputs.size() <= params.num_in); | ||
| net.reshape(params.reshape_inputs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So how it works for your purpose? Why we're not looking at the input data dimensions? Isn't this the case?
|
Another question - how does this reshape thing co-exists with the automatic preprocessing we set? |
|
@mpashchenkov Please take a look on new comments and merge conflict. |
mpashchenkov
force-pushed
the
mp/ocv-gapi-input-cnn-reshape
branch
from
991afe9 to
1189a29
Compare
| Params<Net>& cfgInputReshape(const std::map<std::string, std::vector<std::size_t>> &r) { | ||
| desc.reshape_inputs = r; | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpashchenkov what about this?
| size_t to_hw_shift = 0; | ||
| switch (layout) { | ||
| case InferenceEngine::NHWC: | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need {/} as you don't declare local variables here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| const auto input_name = ii->name(); | ||
| const auto idx = getIdxByName(uu->params.reshape_in_names, input_name); | ||
| uu->params.reshape_in_shapes[uu->params.reshape_in_names.at(idx)] = input_dims; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain what happens here in this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments about steps.
| @@ -619,8 +653,11 @@ struct InferROI: public cv::detail::KernelTag { | |||
| // 0th is ROI, 1st is input image | |||
| auto &&ii = uu.inputs.at(uu.params.input_names.at(0)); | |||
| auto &&mm = in_metas.at(1u); | |||
| configureInputInfo(ii, mm); | |||
| configureInputInfo(ii, const_cast<IEUnit*>(&uu), mm); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, const_cast is not good. It breaks the above contracts where we declare our inputs as const.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added FIXME.
| if (!uu->params.reshape_in_names.empty()) { | ||
| configureReshape(ii, uu, {meta.size.height, meta.size.width}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this logic is all correct.
Hypothetically, one kind of reshape shouldn't exclude the other one - and so for an N-input network the user could specify particular shapes for layers X and Y and keep it dynamic (based on input) for Z.
Also, is {W,H} the right shape? Does it work well with SetResizeAlgorithm() / SetBlob thing we also use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New behavior:
You can call reshape by shapes parameter for some layers and call reshape by image size for another layers.
If both calls contain same layers names, then reshaping by shapes is preferred.
|
@TolyaTalamanov please have a look on this. |
| Params<Net>& cfgInputReshape(const std::map<std::string, std::vector<std::size_t>> &r) { | ||
| desc.reshape_inputs = r; | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider to add rvalue implementation, because it will be used in most of cases
mpashchenkov
changed the title
|
Updated magic commands (openvino-2021.1.0 version). Old version doesn't support some blob functionality. @alalek, please restart build for this PR, it should work correctly. |
You should add
P.S. OV 2020.3 LTS should be supported too (with partially disabled functionality) /cc @opencv/intel-gapi |
mpashchenkov
force-pushed
the
mp/ocv-gapi-input-cnn-reshape
branch
from
a705a8a to
7bb1fe3
Compare
mpashchenkov
force-pushed
the
mp/ocv-gapi-input-cnn-reshape
branch
from
7bb1fe3 to
1e65dae
Compare
|
Good job, thanks! 👍 |
|
@alalek, CN Jenkins can't reach https://github.com/opencv/opencv.git, but build bot's build is correct. Do need to restart CN Jenkins or can merge it by build bot's result? |
|
Ignore, these failures are not related to any patch. |
|
@alalek, can you merge this PR? |
…nn-reshape [G-API]: Adding reshape for CNN input. * Added CNN input IE reshape * rbs * Added unordered_set instead vector * Alignment
Adding reshape for CNN input
Reshape changes shape for CNN input dims.
To use input reshape, you should call function
cfgInputReshapeforParams:.cfgInputReshape( { {layer1_name, shape1}, {layer3_name, shape3} } )- for some layers;.cfgInputReshape(layer1_name, shape1)- for single layer;.cfgInputReshape( { layer1_name, layer3_name } )- for reshaping by input image.layer..._name- name of input layer, which should to be reshaped.shape...- vector of dimensions, which reshape will use (1,3,300,300 for example).Behavior:
input_reshape_tablefrom IEUnit contains pairs layer's name --- dimensions for reshape;cfgInputReshapethen IE throw error message;cfgInputReshapeare constructing element ofinput_reshape_tableby current CNN dimensions and height and width input image;.cfgInputReshapeat the same time then reshape by dimensions is preferred then by input image;InferenceEngine::CNNNetwork::reshapefunction is located inOutMeta()of Infer, InferROI, InferList and InferList2.Tests:
New fixture InferWithReshape:
.cfgInputReshapeat the same time;.cfgInputReshape;New fixture InferWithReshapeNV12:
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.
Magic commands