G-API: Extend MediaFrame to be able to extract additional info besides access by smirnov-alexey · Pull Request #20151 · opencv/opencv

smirnov-alexey · 2021-05-24T22:57:32Z

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

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.3.0:20.04
build_image:Custom Win=openvino-2021.3.0
build_image:Custom Mac=openvino-2021.3.0

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

smirnov-alexey · 2021-05-24T22:59:07Z

@dmatveev @TolyaTalamanov @rgarnov please, review

TolyaTalamanov

Great! 🔥

Only cosmetic fixes are required

modules/gapi/include/opencv2/gapi/media.hpp

modules/gapi/src/api/media.cpp

TolyaTalamanov · 2021-05-25T04:57:25Z

modules/gapi/test/common/gapi_core_tests_inl.hpp

            cv::MediaFrame::View::Strides ss = { m_mat.step, 0u, 0u, 0u };
            return cv::MediaFrame::View(std::move(pp), std::move(ss));
        }
+        cv::util::any blobParams() const override {


Every MediaFrame and Adapter must implement this ? No default implementation by default ?

Agreed. Provided a default implementation and removed all the overloads

TolyaTalamanov · 2021-05-25T04:59:41Z

modules/gapi/test/infer/gapi_infer_onnx_test.cpp

        cv::MediaFrame::View::Strides ss = { m_mat.step, 0u, 0u, 0u };
        return cv::MediaFrame::View(std::move(pp), std::move(ss), Cb{m_cb});
    }
+    cv::util::any blobParams() const override {


Is it really needed to define this default behavior manually ?

Answered above

TolyaTalamanov · 2021-05-25T05:00:48Z

modules/gapi/test/infer/gapi_infer_ie_test.cpp

+    cv::Mat bgr = cv::Mat::eye(240, 320, CV_8UC3);
+    cv::MediaFrame frame = cv::MediaFrame::Create<TestMediaBGR>(bgr);
+
+    cv::util::any any_params = frame.blobParams();


Why not just params

Ok, but you don't need to store it in separate variable

TolyaTalamanov · 2021-05-25T05:02:44Z

modules/gapi/test/infer/gapi_infer_ie_test.cpp

+    InferenceEngine::ParamMap pmap({{"HELLO", 42},
+                                    {"COLOR_FORMAT",
+                                     InferenceEngine::ColorFormat::NV12}});
+    InferenceEngine::TensorDesc tdesc;


So, maybe it makes sense to test with non-empty TensorDesc

TolyaTalamanov · 2021-05-25T05:12:47Z

modules/gapi/test/infer/gapi_infer_ie_test.cpp

+    auto params = cv::util::any_cast<std::pair<InferenceEngine::TensorDesc,
+                                               InferenceEngine::ParamMap>>(any_params);
+
+    InferenceEngine::ParamMap pmap({{"HELLO", 42},


I would make it like this:

auto expected = std::make_pair(IE::TensorDesc{IE::Precision::U8, {1, 3, 300, 300}, IE::Layout::NCHW}, IE::ParamMap{{"HELLO", 42}, {"COLOR_FORMAT", IE::ColorFormat::NV12}}); auto actual = cv::util::any_cast<decltype(expected)>>(frame.params()); EXPECT_EQ(expected, actual);

Done, thanks for the example

dmatveev

This PR can be simplified dramatically :)

modules/gapi/include/opencv2/gapi/media.hpp

dmatveev · 2021-05-31T09:27:17Z

modules/gapi/include/opencv2/gapi/media.hpp

    virtual ~IAdapter() = 0;
    virtual cv::GFrameDesc meta() const = 0;
    virtual MediaFrame::View access(MediaFrame::Access) = 0;
+    virtual cv::util::any blobParams() const = 0;


No as it breaks all the existing adapters. Please supply a default implementation instead (and make it safe enough to call by default - so no asserts/exceptions here if this default implementation is used).

Got you, fixed

dmatveev · 2021-05-31T09:28:09Z

modules/gapi/test/gapi_frame_tests.cpp

    }
+    cv::util::any blobParams() const override {
+        return std::make_pair<std::string, int>("hello", 42);
+    }


Please drop these changes after you introduce a default implementation

dmatveev · 2021-05-31T09:28:13Z

modules/gapi/test/gapi_frame_tests.cpp

+    cv::util::any blobParams() const override {
+        GAPI_Assert(false && "Not implemented");
+        return {};
+    }


Please drop these changes after you introduce a default implementation

dmatveev · 2021-05-31T09:28:27Z

modules/gapi/test/infer/gapi_infer_ie_test.cpp

+    cv::util::any blobParams() const override {
+        GAPI_Assert(false && "Not implemented");
+        return {};
+    }


smirnov-alexey · 2021-05-31T12:33:42Z

@dmatveev @TolyaTalamanov please, review the changes

smirnov-alexey · 2021-05-31T13:17:58Z

@alalek How can I fix CN checks (do I need to)?

[Pipeline] sh
+ curl -s https://api.github.com/repos/smirnov-alexey/opencv_extra/branches
[Pipeline] }
ERROR: script returned exit code 7

I believe I got no branches in opencv_extra

alalek · 2021-05-31T14:20:20Z

CN checks

Ignore. These failures are infrastructure-related.

TolyaTalamanov

LGTM 👍

TolyaTalamanov · 2021-06-01T09:44:47Z

modules/gapi/test/infer/gapi_infer_ie_test.cpp

        return cv::MediaFrame::View(std::move(pp), std::move(ss), Cb{m_cb});
    }
+    cv::util::any blobParams() const override {
+        return std::make_pair<InferenceEngine::TensorDesc,


I believe you don't have to specify template manually, make_pair(...) would be enough

dmatveev

It seems I commented on these parts in another review, #20156 -- please incorporate the relevant review comments here and I think it can be merged.

Thanks!

smirnov-alexey · 2021-06-07T16:06:56Z

@alalek can we merge this?

dmatveev

@alalek can you please merge this one before the freeze? Thanks

…rame G-API: Extend MediaFrame to be able to extract additional info besides access * Extend MediaFrame to be able to extract additional info besides access * Add default implementation for blobParams() * Add comment on the default blobParams()

Extend MediaFrame to be able to extract additional info besides access

6700fe0

TolyaTalamanov reviewed May 25, 2021

View reviewed changes

asmorkalov added the category: g-api / gapi label May 25, 2021

smirnov-alexey mentioned this pull request May 25, 2021

G-API: Support remote inference #20156

Merged

6 tasks

dmatveev self-assigned this May 31, 2021

dmatveev added this to the 4.5.3 milestone May 31, 2021

dmatveev reviewed May 31, 2021

View reviewed changes

Add default implementation for blobParams()

48fc58e

TolyaTalamanov approved these changes Jun 1, 2021

View reviewed changes

dmatveev approved these changes Jun 3, 2021

View reviewed changes

Add comment on the default blobParams()

713a257

dmatveev approved these changes Jun 7, 2021

View reviewed changes

alalek merged commit d9ed9a9 into opencv:master Jun 8, 2021

alalek mentioned this pull request Jun 13, 2021

(5.x) Merge 4.x #20266

Merged

alalek mentioned this pull request Oct 15, 2021

(5.x) Merge 4.x #20886

Merged

Uh oh!

Conversation

smirnov-alexey commented May 24, 2021 • 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 May 24, 2021

Uh oh!

TolyaTalamanov left a comment

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!

dmatveev left a comment

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!

smirnov-alexey commented May 31, 2021

Uh oh!

smirnov-alexey commented May 31, 2021

Uh oh!

alalek commented May 31, 2021

Uh oh!

TolyaTalamanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

smirnov-alexey commented Jun 7, 2021

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

smirnov-alexey commented May 24, 2021 •

edited

Loading