G-API: Get input model layout from the IR if possible in OV 2.0 backend by smirnov-alexey · Pull Request #24658 · opencv/opencv

smirnov-alexey · 2023-12-06T14:53:10Z

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 another license that is incompatible with OpenCV
The PR is proposed to the proper branch
There is a reference to the 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

smirnov-alexey · 2023-12-06T14:57:05Z

This PR fixes smart classroom G-API demo in OMZ: openvinotoolkit/open_model_zoo#3881

dmatveev · 2023-12-06T15:09:23Z

modules/gapi/test/infer/gapi_infer_ov_tests.cpp

+
+    auto f = std::make_shared<ov::Model>(ov::ResultVector{result}, ov::ParameterVector{data1}, "function_name");
+
+    ov::serialize(f, model_path, weights_path, ov::pass::Serialize::Version::IR_V11);


Probably there's some general approach to producing temporary files in tests in OpenCV.. Like some specific folder or naming convention?

btw maybe we in the future we need to extend our API to accept objects of ov::Model directly instead of xml/bin

btw maybe we in the future we need to extend our API to accept objects of ov::Model directly instead of xml/bin

In that case it won't be ov::Model because headers will depend on OpenVINO

Dude we did this with PlaidML already

plaidml.hpp looks clear from framework details

dmatveev · 2023-12-07T09:16:50Z

@TolyaTalamanov do you have any comments here?

TolyaTalamanov · 2023-12-07T12:17:22Z

modules/gapi/src/backends/ov/govbackend.cpp

-            m_input_model_layout.emplace(input_name, default_layout);
+            const auto& input_layout = ::ov::layout::get_layout(m_model->input(input_name));
+            if (!input_layout.empty()) {
+                input_info.model().set_layout(input_layout);


input_info.model() and m_model should refer to the same object, I'm wondering why this line is even needed.

Looks even though model has model layout it must be specified to PrePostProcessor as well.

Worth a comment at least, since it's not clear why we need to specify model layout for the preprocessor created from exactly the same model.

PrePostProcessor ppp(model); audo layout = ov::layout::get::layout(model->input(input_name)); if (!layout.empty()) { ppp.input(input_name).model().set_layout(layout); }

Ok, looks we figured out it locally. Need to force model layout only if model doesn't have one otherwise keep it as-is.

It works without this line. OV works fine

TolyaTalamanov · 2023-12-07T12:18:34Z

modules/gapi/test/infer/gapi_infer_ov_tests.cpp

+    ov::layout::set_layout(data1, ov::Layout("NHWC"));
+
+    auto sin = std::make_shared<ov::opset8::Sin>(data1);
+    sin->output(0).set_names({"sin_t"});  // tensor name


not necessarily needed to specify name there

TolyaTalamanov · 2023-12-07T12:19:38Z

modules/gapi/test/infer/gapi_infer_ov_tests.cpp

+
+    auto f = std::make_shared<ov::Model>(ov::ResultVector{result}, ov::ParameterVector{data1}, "function_name");
+
+    ov::serialize(f, model_path, weights_path, ov::pass::Serialize::Version::IR_V11);


btw maybe we in the future we need to extend our API to accept objects of ov::Model directly instead of xml/bin

In that case it won't be ov::Model because headers will depend on OpenVINO

TolyaTalamanov · 2023-12-07T12:26:47Z

modules/gapi/test/infer/gapi_infer_ov_tests.cpp

+                                                      model_path,
+                                                      weights_path,
+                                                      device_id);
+    EXPECT_NO_THROW(comp.apply(cv::gin(in_mat1), cv::gout(gapi_mat),


Tbh I didn't get the purpose of this test, the model has NHWC layout but you pass NCHW mat, so it won't throw any exceptions because ov backend doesn't validate either layouts match or not.

The test should be something like that:

Model has NHWC layout

Provide 4D mat and specify only tensor layout (e.g cfgInputTensorLayout('NCHW'))

Check if there is NHWC->NCHW conversion is added to the model. (it won't be added without your changes, since model layout not specified therefore won't be propagated into PrePostProcessor)

TolyaTalamanov · 2023-12-07T12:34:00Z

modules/gapi/src/backends/ov/govbackend.cpp

-            m_input_model_layout.emplace(input_name, default_layout);
+            const auto& input_layout = ::ov::layout::get_layout(m_model->input(input_name));
+            if (!input_layout.empty()) {
+                input_info.model().set_layout(input_layout);


Worth a comment at least, since it's not clear why we need to specify model layout for the preprocessor created from exactly the same model.

PrePostProcessor ppp(model); audo layout = ov::layout::get::layout(model->input(input_name)); if (!layout.empty()) { ppp.input(input_name).model().set_layout(layout); }

dmatveev · 2023-12-08T09:38:18Z

modules/gapi/src/backends/ov/govbackend.cpp

-            m_input_model_layout.emplace(input_name, default_layout);
+            const auto& input_layout = ::ov::layout::get_layout(m_model->input(input_name));
+            if (!input_layout.empty()) {
+                input_info.model().set_layout(input_layout);


Looking at it on the second day, is this a valid change at all? Why we're setting whole model's layout based on some input layout?

It's not for the whole model, it's set to input_info which refers to the particular input layer.

input_info = m_ppp.input(input_name); ... input_info.model().set_layout(input_layout);

This line will be removed, since OV does it in their PrePostProc

dmatveev · 2023-12-08T09:38:53Z

Please explain what is the issue and what is the change here.

smirnov-alexey · 2023-12-08T16:16:31Z

Please explain what is the issue and what is the change here.

The issue is that user might not want to configure input model layout which is available in the network. So, they don't set cfgLayouts(). In this case OV backend assumes by default it's always NCHW which is not the case for some models (e.g. in OMZ smart classroom gapi demo). I'll check if OV already knows proper input layout and if it's there, we do nothing (instead of setting NCHW)

TolyaTalamanov

LGTM 👍

smirnov-alexey · 2023-12-11T15:52:45Z

@asmorkalov @mshabunin can we merge this please?

smirnov-alexey · 2023-12-12T11:53:29Z

@asmorkalov @mshabunin kind reminder

smirnov-alexey · 2023-12-13T12:02:01Z

@asmorkalov @mshabunin can we merge it this week pretty please?

mshabunin

Looks good to me.

…del_layout G-API: Get input model layout from the IR if possible in OV 2.0 backend opencv#24658 ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [x] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake

Get input model layout from the IR if possible

43aa528

smirnov-alexey requested review from TolyaTalamanov and dmatveev December 6, 2023 14:53

Remove unnecessary headers

7889c82

dmatveev approved these changes Dec 6, 2023

View reviewed changes

dmatveev self-assigned this Dec 7, 2023

dmatveev added the category: g-api / gapi label Dec 7, 2023

dmatveev added this to the 4.9.0 milestone Dec 7, 2023

TolyaTalamanov reviewed Dec 7, 2023

View reviewed changes

dmatveev reviewed Dec 8, 2023

View reviewed changes

Rework input layout presumption logic

f6240db

smirnov-alexey force-pushed the as/gapi_ov_get_model_layout branch from 18ed79e to f6240db Compare December 11, 2023 12:49

TolyaTalamanov approved these changes Dec 11, 2023

View reviewed changes

mshabunin approved these changes Dec 13, 2023

View reviewed changes

asmorkalov merged commit 14688e9 into opencv:4.x Dec 13, 2023

asmorkalov mentioned this pull request Jan 19, 2024

5.x merge 4.x #24862

Merged


		auto f = std::make_shared<ov::Model>(ov::ResultVector{result}, ov::ParameterVector{data1}, "function_name");

		ov::serialize(f, model_path, weights_path, ov::pass::Serialize::Version::IR_V11);

Uh oh!

Conversation

smirnov-alexey commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

smirnov-alexey commented Dec 6, 2023

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!

dmatveev commented Dec 7, 2023

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 Dec 8, 2023 • 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!

dmatveev commented Dec 8, 2023

Uh oh!

smirnov-alexey commented Dec 8, 2023

Uh oh!

TolyaTalamanov left a comment • 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!

smirnov-alexey commented Dec 11, 2023

Uh oh!

smirnov-alexey commented Dec 12, 2023

Uh oh!

smirnov-alexey commented Dec 13, 2023

Uh oh!

mshabunin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

smirnov-alexey commented Dec 6, 2023 •

edited

Loading

TolyaTalamanov Dec 8, 2023 •

edited

Loading

TolyaTalamanov left a comment •

edited

Loading