G-API: ONNX. Const input by mpashchenkov · Pull Request #18902 · opencv/opencv

mpashchenkov · 2020-11-23T16:41:07Z

Added support of const input for onnx backend.

How it works:

ORT API infer is Run method:

auto outputs = this_session.Run(Ort::RunOptions{nullptr},
                                in_run_names.data(),
                                &in_tensors.front(),
                                input_names.size(),
                                out_names.data(),
                                out_names.size());

We should call Run() with order of input layers' names. We can put names in different order. If we want to use const input than we call constInput(name, cv::Mat):

auto net = cv::gapi::onnx::Params<YoloNet>{model_path}
.constInput("image_shape", ins[1])

After that name will be added in order of input names and input tensor for Mat's data is created:

input_names.emplace_back(c_in_pair.first);
in_tensors.emplace_back(createTensor(this_memory_info,
                                     in_tensor_info[idx],
                                     c_in_pair.second.first));

And we call Run with new input names and appropriate tensors, where names: {in_layer_name1, const_input_name_for_layer2} and tensors: {tensor_for_layer1, tensor_for_layer2_with_data_from_C_I).

Tests:

Test contains infer for YoloV3. YoloV3's input is resized image (416x416) and tensor with real size.

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

Custom build:

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_gapi_standalone:Custom=ade-0.1.1f

force_builders=Custom
buildworker:Custom=linux-1
build_image:Custom=ubuntu-onnx:20.04
test_modules:Custom=gapi
test_filter:Custom=*ONNX*


build_image:Custom Win=openvino-2020.3.0
build_image:Custom Mac=openvino-2020.3.0
test_modules:Custom Win=gapi
test_modules:Custom Mac=gapi

OrestChura

I have some comments, but overall seems solid

modules/gapi/src/backends/onnx/gonnxbackend.cpp

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

OrestChura · 2020-11-26T23:58:13Z

modules/gapi/src/backends/onnx/gonnxbackend.cpp

              || ade::util::any_of(out_tensor_info, is_dyn);
-    if (is_dynamic && !params.custom_post_proc) {
+    if (is_dynamic
+        && !params.custom_post_proc) {


so, why this change? Is it necessary?

The idea of the author.

Mb for the debug, to produce errors on different lines (?)

you could leave formatting for this if as-is.

modules/gapi/src/backends/onnx/gonnxbackend.cpp

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

modules/gapi/src/backends/onnx/gonnxbackend.cpp

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

modules/gapi/src/backends/onnx/gonnxbackend.cpp

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

dmatveev

The main question is -- why altering the IE version?

dmatveev · 2020-12-14T12:24:15Z

cmake/FindONNX.cmake

 # For now, check the old name ORT_INSTALL_DIR
 if(ORT_INSTALL_DIR AND NOT ONNXRT_ROOT_DIR)
-  set(ONNXRT_ROOT_DIR ORT_INSTALL_DIR)
+  set(ONNXRT_ROOT_DIR ${ORT_INSTALL_DIR})


Isn't this merged already?

I caught it in this branch. It is merged.

But then there should be no such change

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

dmatveev · 2020-12-14T12:34:15Z

modules/gapi/src/backends/onnx/gonnxbackend.cpp

              || ade::util::any_of(out_tensor_info, is_dyn);
-    if (is_dynamic && !params.custom_post_proc) {
+    if (is_dynamic
+        && !params.custom_post_proc) {


you could leave formatting for this if as-is.

modules/gapi/src/backends/onnx/gonnxbackend.cpp

dmatveev

👍 let's merge this!

dmatveev · 2020-12-14T13:56:38Z

@mpashchenkov note you don't have IE builds in your MR script -- I'd add those to check if IE is still valid.

You can combine it with a rebase to turn 16 commits into one.

OrestChura

LGTM

alalek · 2020-12-14T15:54:15Z

Please squash commits and rebase as 1 commit.
GitHub diff should be clean from garbage and "non-existed" changes.

alalek · 2020-12-14T16:36:13Z

modules/gapi/src/backends/onnx/gonnxbackend.cpp

    if (params.num_out == 1u && params.output_names.empty()) {
        params.output_names = { out_tensor_info.front().name };
    }
-


unnecessary change

alalek · 2020-12-14T16:40:06Z

modules/gapi/src/backends/onnx/gonnxbackend.cpp

 };

+namespace {
+std::string pdims(const std::vector<int64_t> &dims) {


namespace {

Again, use "static" instead.

Subscribe and check all ongoing notifications/discussions about G-API PRs / Issues.
It is really annoying to repeat the same things.

P.S. "Why" answer you may find in corresponding discussion.

Wait, that's the backend's code, and we use anonymous namespaces as a more "modern" replacement to static there a lot.

I remember there were problems in tests but what's wrong with the module code?

It is not related to anonymous types in templates.
It is mostly to have better names in debugger.

Anonymous namespace is still necessary for internal types (but avoid using them in tests due to templates issues)

alalek · 2020-12-14T16:55:53Z

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

+    if (!initialized)
+    {
+        // Since G-API has no own test data (yet), it is taken from the common space
+        const char* testDataPath = getenv("OPENCV_TEST_DATA_PATH");


OPENCV_TEST_DATA_PATH

Again, it points to opencv_extra/testdata only.

It can't be used for extra locations with:

DNN's large models (OPENCV_DNN_TEST_DATA_PATH)

ONNX models from LFS ( ??? , on CI OPENCV_DNN_TEST_DATA_PATH is reused as fallback)

They are dedicated locations, so they should have own variables with fallbacks.

This function has zero effect (OPENCV_TEST_DATA_PATH is added by ts module).

I believe it is time to grab all these initTestDataPath() calls and put them into test_main.cpp. See example here.

/cc @dmatveev

Good point, thanks! I believe this code is a copy-paste from some other module where we were stealing DL models from DNN or so.

If the existing tests' lookups for test data (not models) continue working without this, it makes sense to remove this block.

Also, a good point on unifying overall initTestDataPath once and for everybody. @mpashchenkov can you please open a ticket on that? With the mentioned example in. Thanks!

Ticket created.

@alalek
Why does the function have zero effect?
What is the correct way to call findDataFile function?
It doesn't work without initTestDataPath.
It is same in ts module, but how to use it correctly?
My way:

export OPENCV_TEST_DATA_PATH = /path_to/opencv_extra/testdata/

call initTestDataPath (useless step, as I understand it)

call findDataFile

@alalek, can you give advice about this?

There is no "gapi" test data in opencv_extra, so no need to pass its prefix:

-CV_TEST_MAIN("gapi") +CV_TEST_MAIN("")

alalek · 2020-12-14T16:57:08Z

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

+
+TEST_F(ONNXYoloV3MultInput, InferConstInput)
+{
+    useModel("/object_detection_segmentation/yolov3/model/yolov3-10");


again, leading '/'

alalek · 2020-12-14T17:01:56Z

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

+    }
+};
+
+class ONNXYoloV3MultInput : public ONNXWithRemap {


ONNXYoloV3MultInput

ONNXYoloV3MultiInput ?

alalek · 2020-12-14T17:02:10Z

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

+    useModel("/object_detection_segmentation/yolov3/model/yolov3-10");
+    // Tensor with incorrect image size
+    // is used for check case when InputLayers and constInput have same names
+    cv::Mat bad_shape;


Is it a bad arg test? Which check should fail?

This test checks use of data from const input when input names are incorrectly set. If input names contain names from const input, then calculation will be performed according to data given by .ConstInput() function.

alalek · 2020-12-14T17:07:21Z

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

+            const auto out_size = std::min(out_onnx.at(i).total(), out_gapi.at(i).total());
+            for (size_t d_idx = 0; d_idx < out_size; ++d_idx) {


These checks are not reliable.
If out_gapi.at(i).total() is zero then no checks are performed at all.

Added assert for zero size check.

alalek · 2020-12-14T17:10:47Z

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

+}
+
 // FIXME: taken from the DNN module
 void normAssert(const cv::InputArray& ref, const cv::InputArray& test,


const cv::InputArray&

InputArray is already a const reference.

alalek · 2020-12-14T17:14:35Z

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

+        if (optr[size] <= 0) {
+            std::cout << optr[size] << std::endl;
+        }


Message is unclear, so it can't help with investigation of test failures.

alalek · 2020-12-14T17:18:09Z

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

+    virtual void SetUp() {
+        const int yolo_in_h = 416;
+        const int yolo_in_w = 416;
+        cv::Mat yolov3_input, shape, prep_mat;;


alalek · 2020-12-14T22:40:50Z

added this to the 4.5.1. milestone 9 hours ago

Please do not create NEW milestones!

dmatveev · 2020-12-15T08:58:46Z

Please do not create NEW milestones!

Sorry, that was a typo. It was supposed to be 4.5.1

TolyaTalamanov · 2020-12-15T14:13:09Z

modules/gapi/src/backends/onnx/gonnxbackend.cpp

+        in_names_without_const.clear();
+        std::copy_if(params.input_names.begin(), params.input_names.end(),
+                     std::back_inserter(in_names_without_const),
+                     [&](std::string& name) {


I believe it can be const std::string&

TolyaTalamanov · 2020-12-15T14:15:00Z

modules/gapi/src/backends/onnx/gonnxbackend.cpp

+                                             in_tensor_info[idx],
+                                             c_in_pair.second.first));
+        // Puts const input names in sequence for Run
+        // Ort can match input tensors to CNN inputs by names


Renamed to ONNXRuntime.

TolyaTalamanov · 2020-12-15T14:16:41Z

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

+    const T* const inptr = in.ptr<T>();
+    T* const optr = out.ptr<T>();
+    const size_t size = std::min(out.total(), in.total());
+    for (size_t i = 0; i < size; ++i) {


std::copy ?

Yes, thanks.

TolyaTalamanov · 2020-12-15T14:18:07Z

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

+    }
+}
+
+void remapSsdPorts(const std::unordered_map<std::string, cv::Mat> &onnx,


remapSSDPorts

dmatveev · 2021-01-12T16:41:24Z

@alalek anything else remaining here w.r.t tests?

G-API: ONNX. Const input * Added const input for ONNX backend * Returned initMatrixRandu, added some comments, rebase

mpashchenkov added test category: g-api / gapi labels Nov 23, 2020

mpashchenkov requested review from OrestChura and TolyaTalamanov November 23, 2020 16:41

OrestChura reviewed Nov 27, 2020

View reviewed changes

OrestChura reviewed Nov 30, 2020

View reviewed changes

modules/gapi/src/backends/onnx/gonnxbackend.cpp Outdated Show resolved Hide resolved

OrestChura added the feature label Nov 30, 2020

OrestChura reviewed Nov 30, 2020

View reviewed changes

modules/gapi/src/backends/onnx/gonnxbackend.cpp Outdated Show resolved Hide resolved

modules/gapi/src/backends/onnx/gonnxbackend.cpp Outdated Show resolved Hide resolved

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

OrestChura reviewed Dec 11, 2020

View reviewed changes

modules/gapi/src/backends/onnx/gonnxbackend.cpp Outdated Show resolved Hide resolved

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

dmatveev reviewed Dec 14, 2020

View reviewed changes

dmatveev approved these changes Dec 14, 2020

View reviewed changes

dmatveev self-assigned this Dec 14, 2020

dmatveev added this to the 4.5.1. milestone Dec 14, 2020

OrestChura approved these changes Dec 14, 2020

View reviewed changes

alalek reviewed Dec 14, 2020

View reviewed changes

TolyaTalamanov reviewed Dec 15, 2020

View reviewed changes

mpashchenkov force-pushed the mp/onnx-const-input branch from 662297c to 6a6e301 Compare December 15, 2020 14:52

Maxim Pashchenkov added 2 commits December 25, 2020 15:53

Added const input for ONNX backend

40d17f6

Returned initMatrixRandu, added some comments, rebase

183eef0

mpashchenkov force-pushed the mp/onnx-const-input branch from 6a6e301 to 183eef0 Compare December 25, 2020 13:08

alalek merged commit 3eaeca5 into opencv:master Jan 12, 2021

alalek mentioned this pull request Apr 9, 2021

(5.x) Merge 4.x #19885

Merged

a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023

Merge pull request opencv#18902 from mpashchenkov:mp/onnx-const-input

6c316a8

G-API: ONNX. Const input * Added const input for ONNX backend * Returned initMatrixRandu, added some comments, rebase

		const auto out_size = std::min(out_onnx.at(i).total(), out_gapi.at(i).total());
		for (size_t d_idx = 0; d_idx < out_size; ++d_idx) {

Uh oh!

Conversation

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

Uh oh!

Added support of const input for onnx backend.

How it works:

Tests:

Pull Request Readiness Checklist

Custom build:

Uh oh!

OrestChura left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

dmatveev commented Dec 14, 2020

Uh oh!

OrestChura left a comment

Choose a reason for hiding this comment

Uh oh!

alalek commented Dec 14, 2020

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!

mpashchenkov commented Nov 23, 2020 •

edited

Loading

mpashchenkov Dec 14, 2020 •

edited

Loading