[G-API]: Eliminates detail::ParamDesc mention from public tests by OrestChura · Pull Request #18913 · opencv/opencv

OrestChura · 2020-11-24T21:01:06Z

This change:

replaces explicit ParamDesc objects declarations by brace-enclosed initializer lists
has to add constructors for ParamDesc
unifies tests appearance as possible

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

force_builders=Custom,Custom Win,Custom Mac
build_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
build_gapi_standalone:Mac=ade-0.1.1f
build_gapi_standalone:Linux x64 Debug=ade-0.1.1f

build_image:Custom=centos:7
buildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

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=gapi
test_modules:Custom Win=gapi
test_modules:Custom Mac=gapi

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

alalek · 2020-11-24T21:48:48Z

modules/gapi/include/opencv2/gapi/infer/ie.hpp

+        ParamDesc(std::string model, std::string weights, std::string device,
+                  std::vector<std::string> inputNames, std::vector<std::string> outputNames,
+                  std::unordered_map<std::string, ConstInput> constInputs,
+                  std::size_t numIn, std::size_t numOut, Kind kind_, bool isGen, IEConfig config_)


Use const reference as input parameters

Of course. Thanks!
Fixed

rolled back and added std::move in initializations to make less copies if rvalues are passed

@OrestChura Nope, that won't work. All of those objects will be copied to the function's arguments. You should use const reference or universal reference (aka && + std::forward)

@TolyaTalamanov can you take a look?

On the one hand no needed implement both overloads (const& and &&)
On the other hand In case copy, ParamsDesc(const& T) make only one copy but ParamDesc(T t) make copy + move

I'm ok with both of them

I'd prefer to leave it as is now as it has some advantages and, actually, no so important because it's not the public code

because it's not the public code

Actually, it is a public header:

modules/gapi/include/opencv2/gapi/infer/ie.hpp

"copy + move" vs "const reference + copy"

The second one is preferable.
Copying in the first case is done on a caller side. Such copying significantly increases binary code size in cases of multiple use (default).
Copying after "const reference" is limited to 1 instance of binary code.

I see! OK then, thanks. I've returned the const references already.

TolyaTalamanov · 2020-11-30T09:30:35Z

modules/gapi/include/opencv2/gapi/infer/ie.hpp

        bool is_generic;
        IEConfig config;
+
+        ParamDesc(const std::string& model, const std::string& weights, const std::string& device,


Why is it needed ?

Understood, but you make copy here

fixed, pls check if OK

returned to const references #18913 (comment)

TolyaTalamanov · 2020-11-30T09:34:27Z

modules/gapi/test/infer/gapi_infer_ie_test.cpp


 struct ROIList: public ::testing::Test {
-    cv::gapi::ie::detail::ParamDesc params;
+    std::string model_path   = "", weights_path = "", device_id    = "";


Such kind of initialization isn't necessary, because std::string has default ctor

Oh, OK, thanks! removed

modules/gapi/test/infer/gapi_infer_ie_test.cpp

asmorkalov · 2020-12-09T14:26:23Z

@OrestChura @TolyaTalamanov Friendly reminder.

alalek · 2020-12-10T07:47:15Z

Conflicting files
modules/gapi/test/infer/gapi_infer_ie_test.cpp

Rebase is required.

- replaces explicit ParamDesc objects declarations by brace-enclosed initializer lists - has to add constructors for ParamDesc

- remove initializations for std::string variables - reduce copying in ParamDesc constructors

OrestChura · 2020-12-11T16:30:18Z

@TolyaTalamanov please look one more time as there are new tests from the recent PR which have required fixes
/cc @smirnov-alexey as backup

smirnov-alexey

Please, fix copying

OrestChura · 2020-12-14T15:04:48Z

@alalek Let's merge this please

modules/gapi/include/opencv2/gapi/infer/ie.hpp

- hardcoded key values replaced by IE::PluginConfigParams:: references

OrestChura · 2020-12-14T17:20:21Z

@alalek should I squash?

alalek

should I squash?

Not required. There is no mess on GitHub's diff page.

modules/gapi/include/opencv2/gapi/infer/ie.hpp

dmatveev

I don't understand what part of what is eliminated, tbh..

There's more new lines than deleted lines, too.

modules/gapi/include/opencv2/gapi/infer/ie.hpp

dmatveev · 2020-12-14T17:54:03Z

modules/gapi/include/opencv2/gapi/infer/ie.hpp

+                  const std::vector<std::string>& inputNames,
+                  const std::vector<std::string>& outputNames,
+                  const std::unordered_map<std::string, ConstInput>& constInputs,
+                  std::size_t numIn, std::size_t numOut, Kind kind_, bool isGen,
+                  const IEConfig& config_)


Why do we need this constructor at all?

I mean, the {}-thing should work?

Overall - I would not expect any changes at the module API level when there is some test work happening.

This constructor is needed exactly not to change API anywhere else.
I needed the other constructor for possibility to forward ParamDesc in functions via {model, weights, device} statement. But in Params<Net> here the full constructor is used, so I had to add the full constructor explicitly

Let's make a call

dmatveev · 2020-12-14T17:55:50Z

modules/gapi/test/infer/gapi_infer_ie_test.cpp

-        params.model_path, params.weights_path, params.device_id
-    }.cfgOutputLayers({ "age_conv3", "prob" });
+    auto pp = cv::gapi::ie::Params<AgeGender> {model_path, weights_path, device_id
+                                              }.cfgOutputLayers({ "age_conv3", "prob" });


Something strange happened to this formatting.

it is not supposed to look like this.

I can format to the indentation like it was before in this snippet

@dmatveev please check if OK now

alalek · 2020-12-14T22:38:29Z

modules/gapi/include/opencv2/gapi/infer/ie.hpp

+              num_out(numOut), kind(kind_), is_generic(isGen), config(config_) {};
+
+        ParamDesc(const std::string& model, const std::string& weights, const std::string& device)
+            : model_path(model), weights_path(weights), device_id(device) {};


Uninitialized fields (static code scans will come here):

num_in

num_out

kind

is_generic

OrestChura · 2020-12-23T14:56:37Z

After a discussion, it is decided to leave ::ie::detail::ParamDesc as is not to add these huge constructors. Overall, the problem is more complex and requires deeper refactoring than covered in this PR.
Have to close this and open a new one to avoid misunderstanding of a branch

dmatveev · 2020-12-24T07:21:29Z

Thanks Orest!

OrestChura added bug category: g-api / gapi labels Nov 24, 2020

OrestChura requested a review from TolyaTalamanov November 24, 2020 21:01

alalek reviewed Nov 24, 2020

View reviewed changes

OrestChura added the test label Nov 25, 2020

TolyaTalamanov reviewed Nov 30, 2020

View reviewed changes

modules/gapi/test/infer/gapi_infer_ie_test.cpp Outdated Show resolved Hide resolved

TolyaTalamanov approved these changes Dec 10, 2020

View reviewed changes

OrestChura added 4 commits December 10, 2020 14:43

Eliminates detail::ParamDesc mention from public tests

d2ee41f

- replaces explicit ParamDesc objects declarations by brace-enclosed initializer lists - has to add constructors for ParamDesc

Addressing comments: const reefrences

ce125ca

Fixing CPU config tests

d1fac57

Addressing comments

c84ad3b

- remove initializations for std::string variables - reduce copying in ParamDesc constructors

OrestChura force-pushed the oc/fix_paramdesc branch from 42ceb67 to 7829bed Compare December 11, 2020 16:27

Rebase & fix new tests

cfb2ce9

OrestChura requested a review from TolyaTalamanov December 11, 2020 16:28

OrestChura force-pushed the oc/fix_paramdesc branch from 7829bed to cfb2ce9 Compare December 11, 2020 16:34

smirnov-alexey requested changes Dec 11, 2020

View reviewed changes

smirnov-alexey approved these changes Dec 14, 2020

View reviewed changes

OrestChura assigned TolyaTalamanov Dec 14, 2020

alalek reviewed Dec 14, 2020

View reviewed changes

modules/gapi/include/opencv2/gapi/infer/ie.hpp Outdated Show resolved Hide resolved

modules/gapi/include/opencv2/gapi/infer/ie.hpp Show resolved Hide resolved

- const references

adac399

- hardcoded key values replaced by IE::PluginConfigParams:: references

alalek approved these changes Dec 14, 2020

View reviewed changes

modules/gapi/include/opencv2/gapi/infer/ie.hpp Show resolved Hide resolved

dmatveev reviewed Dec 14, 2020

View reviewed changes

Fix formatting

e546566

alalek reviewed Dec 14, 2020

View reviewed changes

OrestChura closed this Dec 23, 2020

OrestChura mentioned this pull request Dec 23, 2020

[G-API]: IE tests pluginConfig fix & refactoring #19206

Merged

3 tasks

Uh oh!

Conversation

OrestChura commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smirnov-alexey Dec 11, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

TolyaTalamanov Dec 14, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asmorkalov commented Dec 9, 2020

Uh oh!

alalek commented Dec 10, 2020

Uh oh!

OrestChura commented Dec 11, 2020

Uh oh!

smirnov-alexey left a comment

Choose a reason for hiding this comment

Uh oh!

OrestChura commented Dec 14, 2020

Uh oh!

Uh oh!

Uh oh!

OrestChura commented Dec 14, 2020

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OrestChura commented Nov 24, 2020 •

edited

Loading

smirnov-alexey Dec 11, 2020 •

edited

Loading

TolyaTalamanov Dec 14, 2020 •

edited

Loading