[G-API] Fix bugs in GIEBackend #20918

TolyaTalamanov · 2021-10-21T08:11:15Z

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
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

Bugs overview

Outputs map are filled only for LoadNetwork case.
These maps: https://github.com/opencv/opencv/blob/master/modules/gapi/src/backends/ie/giebackend.cpp#L221-L222 are filled only in case LoadNetwork: https://github.com/opencv/opencv/blob/master/modules/gapi/src/backends/ie/giebackend.cpp#L241-L242, but are used by both of them:
https://github.com/opencv/opencv/blob/master/modules/gapi/src/backends/ie/giebackend.cpp#L953
cv::gapi::ie::Params::pluginConfig(IEConfig) set config globally to IE::Core, but should be set locally within ExecutableNetwork: https://github.com/opencv/opencv/blob/master/modules/gapi/src/backends/ie/giebackend.cpp#L244-L245
In case ImportNetwork preprocessing should be passed as ExecutableNetwork::SetBlob argument, now preprocessing for NV12 color format is missed.

Build configuration

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
Xbuild_image:Custom=centos:7
Xbuildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f
build_image:Custom=ubuntu-openvino-2021.4.1:20.04
build_image:Custom Win=openvino-2021.4.1
build_image:Custom Mac=openvino-2021.4.1
test_modules:Custom=gapi,python2,python3,java
test_modules:Custom Win=gapi,python2,python3,java
test_modules:Custom Mac=gapi,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=*

TolyaTalamanov · 2021-10-21T09:14:47Z

modules/gapi/src/backends/ie/giebackend.cpp

@@ -218,8 +218,6 @@ struct IEUnit {

    cv::gapi::ie::detail::ParamDesc params;
    IE::CNNNetwork net;
-    IE::InputsDataMap inputs;


I believe it's better to removed them not get confused because they're relevant only in case LoadNetwork

TolyaTalamanov · 2021-10-25T11:22:49Z

modules/gapi/src/backends/ie/giebackend.cpp


    IE::ExecutableNetwork this_network;
    cv::gimpl::ie::wrap::Plugin this_plugin;

    InferenceEngine::RemoteContext::Ptr rctx = nullptr;
+    using PreProcMap = std::unordered_map<std::string, IE::PreProcessInfo>;
+    mutable cv::util::optional<PreProcMap> preproc_map;


Again, relevant only for ImportNetwork case, but should be stored anywhere after it's collected in outMeta method.

TolyaTalamanov · 2021-10-26T09:49:50Z

modules/gapi/src/backends/ie/giebackend.cpp

+static IE::PreProcessInfo configurePreProcInfo(const IE::InputInfo::CPtr& ii,
+                                               const cv::GMetaArg&        mm) {
+
+    IE::PreProcessInfo info;


In case code won't go to none of these if's, IE::PreProcessInfo is empty, I believe that passing that to ExecutableNetwork::SetBlob is OK

Doesn't IE find preprocessing by name because info is empty?

Looks like code comment. API may be changed in the future.

No, I meant for some inputs preprocessing isn't needed, in that case map will contain empty IE::PreProcessInfo.

mpashchenkov

I will be back bit later.

modules/gapi/src/backends/ie/giebackend.cpp

modules/gapi/src/backends/ie/giebackend/giewrapper.hpp

modules/gapi/test/infer/gapi_infer_ie_test.cpp

mpashchenkov · 2021-10-26T12:41:45Z

modules/gapi/test/infer/gapi_infer_ie_test.cpp

+    initDLDTDataPath();
+
+    cv::gapi::ie::detail::ParamDesc params;
+    params.model_path= compileAgeGenderBlob();


Suggested change

params.model_path= compileAgeGenderBlob();

params.model_path = compileAgeGenderBlob();

modules/gapi/test/infer/gapi_infer_ie_test.cpp

modules/gapi/src/backends/ie/giebackend.cpp

mpashchenkov · 2021-10-26T13:04:46Z

modules/gapi/src/backends/ie/giebackend.cpp

+static IE::PreProcessInfo configurePreProcInfo(const IE::InputInfo::CPtr& ii,
+                                               const cv::GMetaArg&        mm) {
+
+    IE::PreProcessInfo info;


Doesn't IE find preprocessing by name because info is empty?

Looks like code comment. API may be changed in the future.

Co-authored-by: Maxim Pashchenkov <maxim.pashchenkov@intel.com>

TolyaTalamanov · 2021-10-27T11:43:14Z

modules/gapi/src/backends/ie/giebackend.cpp

-            const IE::DataPtr& ie_out = uu.outputs.at(out_name);
-            const IE::SizeVector dims = ie_out->getTensorDesc().getDims();
+            const auto& desc =
+                uu.params.kind == cv::gapi::ie::detail::ParamDesc::Kind::Load


Honestly speaking looks awful, does anyone know how these different interfaces can be hidden? Should it be hidden at all?

TolyaTalamanov · 2021-10-27T12:01:31Z

modules/gapi/src/backends/ie/giebackend.cpp

        } else if (params.kind == cv::gapi::ie::detail::ParamDesc::Kind::Import) {
            this_plugin = cv::gimpl::ie::wrap::getPlugin(params);
-            this_plugin.SetConfig(params.config);


Config shouldn't be set to global IECore object, moved this part to loadNetwork and importNetwork

TolyaTalamanov · 2021-10-27T12:14:07Z

modules/gapi/test/infer/gapi_infer_ie_test.cpp

@@ -2239,6 +2267,141 @@ TEST(TestAgeGenderIE, InferWithBatch)
    normAssert(cv::gapi::ie::util::to_ocv(ie_gender), gapi_gender, "Test gender output");
 }

+
+TEST(ImportNetwork, Infer)


ImportNetwork only tested for Infer kernel for BGR & NV12 format, testing other kernels on both of formats cause a huge code duplication I'd scope tests refactoring firstly, @dmatveev what do you think?

ImportNetwork only tested for Infer kernel for BGR & NV12 format, testing other kernels on both of formats cause a huge code duplication I'd scope tests refactoring firstly, @dmatveev what do you think?

Added 8 tests: [Infer, InferROI, InferList, InferList2] x [BGR, NV12]

Didn't test InferList2 on raw inputs

mpashchenkov · 2021-10-27T12:37:07Z

modules/gapi/src/backends/ie/giebackend.cpp


    IE::ExecutableNetwork this_network;
    cv::gimpl::ie::wrap::Plugin this_plugin;

    InferenceEngine::RemoteContext::Ptr rctx = nullptr;

+    // FIXME: This map contains preprocessing information
+    // for input layers in case ImportNetwork. Since this map is collected in
+    // outMeta which is const, need to mark map as mutable.


preproc_map isn't mutable already.

yep, thanks!

mpashchenkov · 2021-10-27T12:39:33Z

modules/gapi/src/backends/ie/giebackend.cpp

@@ -822,6 +818,24 @@ static void configureInputInfo(const IE::InputInfo::Ptr& ii, const cv::GMetaArg
    }
 }

+static IE::PreProcessInfo configurePreProcInfo(const IE::InputInfo::CPtr& ii,
+                                               const cv::GMetaArg&        mm) {
+


Extra empty line.

TolyaTalamanov · 2021-10-29T08:05:27Z

modules/gapi/src/backends/ie/giebackend.cpp

            for (auto &&it : ade::util::zip(ade::util::toRange(uu.params.input_names),
                                            ade::util::toRange(in_metas))) {
                    const auto &input_name = std::get<0>(it);
-                    auto       &&ii = uu.inputs.at(input_name);


This caused warning on Mac build

TolyaTalamanov · 2021-11-01T12:02:38Z

modules/gapi/src/backends/ie/giebackend/giewrapper.cpp

    static IE::Core core;
    return core;
 }

+IE::Core giewrap::getCore() {


#16356

opencv/modules/dnn/src/op_inf_engine.cpp

Line 606 in 68d15fc

static InferenceEngine::Core* init_core = new InferenceEngine::Core(); // 'delete' is never called

TolyaTalamanov · 2021-11-01T15:38:55Z

modules/gapi/test/infer/gapi_infer_ie_test.cpp

-    cv::Mat m_mat;
-    Sync&   m_sync;
+    cv::Mat               m_mat;
+    std::shared_ptr<Sync> m_sync;


Just handled the case where GMockMediaAdapter live more than GMockSource

mpashchenkov

LGTM

modules/gapi/src/backends/ie/giebackend.cpp

TolyaTalamanov · 2021-11-02T08:25:30Z

@alalek Could you have a look, please?

modules/gapi/src/backends/ie/giebackend/giewrapper.cpp

modules/gapi/src/backends/ie/giebackend.cpp

dmatveev

👍 , thanks!

* InferROI * InferList * InferList2

TolyaTalamanov · 2021-11-12T10:47:52Z

modules/gapi/test/infer/gapi_infer_ie_test.cpp

@@ -2239,6 +2282,582 @@ TEST(TestAgeGenderIE, InferWithBatch)
    normAssert(cv::gapi::ie::util::to_ocv(ie_gender), gapi_gender, "Test gender output");
 }

+TEST(ImportNetwork, Infer)
+{
+    const std::string device = "MYRIAD";


Filed task to avoid code duplication in tests

TolyaTalamanov · 2021-11-12T12:16:35Z

@alalek Could you merge it?

TolyaTalamanov · 2021-11-15T11:28:10Z

@alalek it seems to be ready for merge

…importNetwork [G-API] Fix bugs in GIEBackend * Remove inputs/outputs map from IEUnit * Add test * Add NV12 test * Reorganize setBlob function * Check that backend don't overwrite blob precision * Stop setting config to global IE::Core * Replace mutable to const_cast * Update modules/gapi/test/infer/gapi_infer_ie_test.cpp * Update modules/gapi/test/infer/gapi_infer_ie_test.cpp * Make blob parameter as const ref * Cosmetic fixes * Fix failed test on inferROI * Removed double ref for ii * Disable tests * Skip tests if device not available * Use Sync prim under shared_ptr to avoid issue on MAC * Apply WA for IE::Core * Apply WA for MAC build * Try to apply another WA * Not release IE::Core for apple * Put comment * Support PreprocInfo for * InferROI * InferList * InferList2 * Remove empty line * Fix alignment Co-authored-by: Maxim Pashchenkov <maxim.pashchenkov@intel.com>

Remove inputs/outputs map from IEUnit

4768cd0

TolyaTalamanov added bug category: g-api / gapi labels Oct 21, 2021

TolyaTalamanov added this to the 4.5.5 milestone Oct 21, 2021

TolyaTalamanov requested a review from mpashchenkov October 21, 2021 08:11

TolyaTalamanov commented Oct 21, 2021

View reviewed changes

TolyaTalamanov added 3 commits October 24, 2021 20:36

Add test

fec9f28

Add NV12 test

7cbaa31

Reorganize setBlob function

72541dd

TolyaTalamanov commented Oct 25, 2021

View reviewed changes

TolyaTalamanov added 3 commits October 25, 2021 14:23

Check that backend don't overwrite blob precision

621ff05

Stop setting config to global IE::Core

49138fe

Replace mutable to const_cast

c93b7b0

TolyaTalamanov changed the title ~~[G-API] Fix bug with empty map for importNetwork~~ [G-API] Fix bugs in GIEBackend Oct 26, 2021

TolyaTalamanov commented Oct 26, 2021

View reviewed changes

mpashchenkov reviewed Oct 26, 2021

View reviewed changes

TolyaTalamanov mentioned this pull request Oct 26, 2021

[G-API] Enable 2D inputs for GIEBackend #20925

Merged

6 tasks

TolyaTalamanov and others added 2 commits October 26, 2021 18:03

Update modules/gapi/test/infer/gapi_infer_ie_test.cpp

9a2a458

Co-authored-by: Maxim Pashchenkov <maxim.pashchenkov@intel.com>

Update modules/gapi/test/infer/gapi_infer_ie_test.cpp

96b99ee

Co-authored-by: Maxim Pashchenkov <maxim.pashchenkov@intel.com>

TolyaTalamanov commented Oct 27, 2021

View reviewed changes

Make blob parameter as const ref

4e7fb0c

TolyaTalamanov commented Oct 27, 2021

View reviewed changes

mpashchenkov reviewed Oct 27, 2021

View reviewed changes

TolyaTalamanov added 3 commits October 27, 2021 16:19

Cosmetic fixes

c02ce0d

Fix failed test on inferROI

2f3e328

Removed double ref for ii

ad05ab4

TolyaTalamanov commented Oct 29, 2021

View reviewed changes

TolyaTalamanov added 2 commits October 29, 2021 11:34

Disable tests

65f1d17

Skip tests if device not available

782abb2

TolyaTalamanov force-pushed the at/fix-empty-map-importNetwork branch from 5a3268f to 782abb2 Compare October 29, 2021 12:42

TolyaTalamanov added 2 commits October 29, 2021 17:44

Use Sync prim under shared_ptr to avoid issue on MAC

e854129

Apply WA for IE::Core

7c8b0c4

TolyaTalamanov commented Nov 1, 2021

View reviewed changes

TolyaTalamanov added 3 commits November 1, 2021 15:19

Apply WA for MAC build

0a46a64

Try to apply another WA

3d89765

Not release IE::Core for apple

40ab0ba

TolyaTalamanov commented Nov 1, 2021

View reviewed changes

mpashchenkov approved these changes Nov 1, 2021

View reviewed changes

modules/gapi/src/backends/ie/giebackend.cpp Show resolved Hide resolved

dmatveev self-assigned this Nov 10, 2021

dmatveev reviewed Nov 10, 2021

View reviewed changes

modules/gapi/src/backends/ie/giebackend/giewrapper.cpp Show resolved Hide resolved

modules/gapi/src/backends/ie/giebackend/giewrapper.cpp Show resolved Hide resolved

Put comment

67a3055

smirnov-alexey reviewed Nov 12, 2021

View reviewed changes

modules/gapi/src/backends/ie/giebackend.cpp Show resolved Hide resolved

dmatveev approved these changes Nov 12, 2021

View reviewed changes

TolyaTalamanov added 2 commits November 12, 2021 13:31

Support PreprocInfo for

dcfa9ec

* InferROI * InferList * InferList2

Remove empty line

d36e9d8

TolyaTalamanov commented Nov 12, 2021

View reviewed changes

Fix alignment

dcd8ae2

TolyaTalamanov requested a review from smirnov-alexey November 15, 2021 10:03

smirnov-alexey approved these changes Nov 15, 2021

View reviewed changes

alalek merged commit 2b2e515 into opencv:4.x Nov 15, 2021

alalek mentioned this pull request Dec 30, 2021

(5.x) Merge 4.x #21371

Merged

alalek mentioned this pull request Feb 22, 2022

(5.x) Merge 4.x #21651

Merged

	params.model_path= compileAgeGenderBlob();
	params.model_path = compileAgeGenderBlob();

Uh oh!

[G-API] Fix bugs in GIEBackend #20918

[G-API] Fix bugs in GIEBackend #20918

Uh oh!

Conversation

TolyaTalamanov commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Bugs overview

Build configuration

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!

mpashchenkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

TolyaTalamanov Nov 1, 2021 • 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!

mpashchenkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TolyaTalamanov commented Nov 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TolyaTalamanov commented Nov 12, 2021

Uh oh!

TolyaTalamanov commented Nov 15, 2021

Uh oh!

Uh oh!

TolyaTalamanov commented Oct 21, 2021 •

edited

Loading

TolyaTalamanov Nov 1, 2021 •

edited

Loading