[WIP] G-API: Wrap GOpaque by TolyaTalamanov · Pull Request #18900 · opencv/opencv

TolyaTalamanov · 2020-11-23T13:01:09Z

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

Build configuration

Xforce_builders_only=linux,docs
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.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=*

modules/gapi/include/opencv2/gapi/gproto.hpp

TolyaTalamanov · 2020-11-26T14:40:32Z

modules/gapi/misc/python/pyopencv_gapi.hpp

        {
            args.emplace_back(reinterpret_cast<pyopencv_GMat_t*>(item)->v);
        }
+        else if (PyObject_TypeCheck(item, reinterpret_cast<PyTypeObject*>(pyopencv_GArrayP2f_TypePtr)))


Since each instance for GArray and GOpaque is a separate object we need to have a huge if here, we can make a separate object for Python that will contain all types for GArray and GOpaque and will be created like this:
g_in = GArray(cv.detail.OpaqueKind.RECT).
I didn't do this because OpaqueKind is located in the internal namespace detail, let's discuss it locally

Not relevant

alalek · 2020-11-27T18:31:20Z

@TolyaTalamanov Please rebase this PR

TolyaTalamanov · 2020-11-30T10:49:00Z

@dkurt @smirnov-alexey Could you take a look, please ?

modules/gapi/include/opencv2/gapi/gkernel.hpp

modules/gapi/include/opencv2/gapi/gcomputation.hpp

modules/gapi/test/gapi_gcomputation_tests.cpp

TolyaTalamanov · 2020-12-16T10:14:08Z

modules/gapi/include/opencv2/gapi/garg.hpp

    return GRunArgsP{ GRunArgP(detail::wrap_host_helper<Ts>::wrap_out(args))... };
 }

+// NB: Used by python wrapper


Where should it be placed ?

Maybe in gcommon?

Are you going to address this?

TolyaTalamanov · 2020-12-16T10:15:27Z

modules/gapi/include/opencv2/gapi/garray.hpp

 };

 using GArrayP2f = GArray<cv::Point2f>;
+using GRects    = GArray<cv::Rect>;


Does it make sense to implement wrapper for GArray & GOpaque to avoid making using for every GArray & GOpaque type ?

Do you really need using everywhere? And why does it bother you? I don't think you need an extra wrapper here

It is impossible to wrap template object into python, so I just make using and wrap it in shadow_gapi.hpp

TolyaTalamanov · 2020-12-16T10:16:04Z

modules/gapi/include/opencv2/gapi/gopaque.hpp


+// NB: Used by python wrapper
+using GSize = GOpaque<cv::Size>;
+using GRect = GOpaque<cv::Rect>;


Does it make sense to implement wrapper for GArray & GOpaque to avoid making using for every GArray & GOpaque type ?

TolyaTalamanov · 2020-12-16T10:17:11Z

modules/gapi/misc/python/pyopencv_gapi.hpp


+// NB: Python wrapper generate T_U for T<U>
+// This behavior is only observed for inputs
+using GOpaque_Size = cv::GOpaque<cv::Size>;


Should it be fixed on python code generator side ?

Oh I see what do you mean by using using. Does this behavior block us?

No, it can be fixed like this

TolyaTalamanov · 2020-12-16T10:17:49Z

modules/gapi/misc/python/pyopencv_gapi.hpp

        {
            args.emplace_back(reinterpret_cast<pyopencv_GArrayP2f_t*>(item)->v.strip());
        }
+        else if (PyObject_TypeCheck(item, reinterpret_cast<PyTypeObject*>(pyopencv_GSize_TypePtr)))


Looks really ugly

TolyaTalamanov · 2020-12-16T10:54:48Z

@smirnov-alexey @dmatveev Could you check that ?

dmatveev · 2020-12-16T11:09:00Z

modules/gapi/include/opencv2/gapi/garg.hpp

+struct GTypeInfo;
+using GTypesInfo = std::vector<GTypeInfo>;
+
+using ExtractArgsCallback = std::function<cv::GRunArgs(const cv::GTypesInfo& info)>;


Is it relevant to garg.hpp at all?

I don't think so, maybe we need a special place to move python stuff to there

Just put a FIXME here (mentioning where it is actually required)

dmatveev · 2020-12-16T11:33:04Z

modules/gapi/misc/python/test/test_gapi_imgproc.py

+
+        # G-API
+        g_in  = cv.GMat()
+        g_out = cv.gapi.boundingRect(g_in)


So does this work without defining GOpaque<Size> as GSize ?

No, because it's needed to wrap GOpaque<Size> into python somehow

Not relevant
use cv.GOpaqueT(cv.gapi.CV_SIZE)

dmatveev · 2020-12-16T11:46:03Z

modules/gapi/misc/python/test/test_gapi_imgproc.py

+        comp = cv.GComputation(cv.GIn(g_in), cv.GOut(g_out))
+
+        actual = comp.apply(cv.gin(points))
+        print('actual = ', actual)


Also.

I want to create a graph which has GOpaque<cv::Rect> among of its inputs. How does it looks like in Python?

g_in = cv.GRect()

BTW, need to remove this print

Not relevant

TolyaTalamanov · 2021-01-12T09:43:37Z

modules/python/src2/cv2.cpp

 }

+template<>
+bool pyopencv_to_generic_vec<bool>(PyObject* obj, std::vector<bool>& value, const ArgInfo& info)


@alalek Do you mind if I add this specialization ?

TolyaTalamanov · 2021-01-12T10:35:59Z

@dmatveev @smirnov-alexey Could you have a look ?

smirnov-alexey · 2021-01-12T12:55:58Z

@TolyaTalamanov I would recommend to switch the build to openvino 2021.01 due to red tests

smirnov-alexey · 2021-01-12T13:00:58Z

modules/gapi/include/opencv2/gapi/python_bridge.hpp

+        }
+    }
+
+    cv::detail::GOpaqueU strip() {


Actually, I would rename that

Why ? What's your suggestion ?

I think it's a bit harsh word but maybe it's just me:) You could use unpack or extract

Ok, don't think that it's really big effort to change that. Let's wait for the @dmatveev comments.

modules/gapi/misc/python/test/test_gapi_infer.py

smirnov-alexey

There are some minor comments left but overall looks good

dmatveev

Too much changes here. Type lists are constantly duplicated. The graph compilation flow is not clear now. No description provided to the MR.

If possible, I'd break it into smaller parts for an easier review. With such massive changes it is quite easy to break old contracts.

dmatveev · 2021-01-12T19:44:12Z

modules/gapi/include/opencv2/gapi/garg.hpp

+struct GTypeInfo;
+using GTypesInfo = std::vector<GTypeInfo>;
+
+using ExtractArgsCallback = std::function<cv::GRunArgs(const cv::GTypesInfo& info)>;


Just put a FIXME here (mentioning where it is actually required)

dmatveev · 2021-01-12T19:44:53Z

modules/gapi/include/opencv2/gapi/gcomputation.hpp


    /// @private -- Exclude this function from OpenCV documentation
-    GAPI_WRAP GRunArgs apply(GRunArgs &&ins, GCompileArgs &&args = {});
+    GAPI_WRAP GRunArgs apply(const cv::ExtractArgsCallback& callback, GCompileArgs &&args = {});


This cv::ExtractArgsCallback shouldn't be in cv::. cv::detail:: is better.

dmatveev · 2021-01-12T19:47:44Z

modules/gapi/include/opencv2/gapi/gstreaming.hpp

     */
-    GAPI_WRAP void setSource(GRunArgs &&ins);
+    void setSource(GRunArgs &&ins);
+    GAPI_WRAP void setSource(const cv::ExtractArgsCallback& callback);


Please separate these two, and make this 2nd setSource "private" for doxygen.

dmatveev · 2021-01-12T19:48:40Z

modules/gapi/include/opencv2/gapi/python_bridge.hpp

+    CV_SIZE,
+    CV_RECT,
+    CV_SCALAR,
+    CV_MAT,


Please declare this list of CV_XXX / type pairs once, and avoid all the code duplication below.

dmatveev · 2021-01-12T19:49:15Z

modules/gapi/include/opencv2/gapi/python_bridge.hpp

+
+struct GAPI_EXPORTS_W_SIMPLE GArrayT
+{
+    using Storage = MakeVariantType<  cv::GArray


what is cv::GArray here?

dmatveev · 2021-01-12T19:54:53Z

modules/gapi/misc/python/pyopencv_gapi.hpp

+    {
+        if (PyObject_TypeCheck(obj, reinterpret_cast<PyTypeObject*>(pyopencv_GOpaqueT_TypePtr)))
+        {
+            auto& opaque = reinterpret_cast<pyopencv_GOpaqueT_t*>(obj)->v;


dmatveev · 2021-01-12T20:02:53Z

modules/gapi/src/api/gcomputation.cpp

+
+    auto ins = callback(m_priv->m_lastCompiled.priv().inInfo());
+
+    // NB: After getting the arguments, recompile the graph with the correct meta


Double compilation may be costly

..especially when dealing with NNs.

dmatveev · 2021-01-12T20:04:04Z

modules/gapi/src/api/gcomputation.cpp

-                        util::throw_error(std::logic_error("Unsupported kind for GArray"));
-                }
+                cv::detail::VectorRef ref;
+                util::get<cv::detail::ConstructVec>(info.ctor)(ref);


Not sure if any other function in this file/class operates on the same low level of abstraction - it is all built on some higher-level entities. Please keep the abstraction level.

dmatveev · 2021-01-12T20:04:46Z

modules/gapi/src/compiler/gcompiler.cpp

+        // FIXME: select which executor will be actually used,
+        // make GExecutor abstract.
+        pE.reset(new GExecutor(std::move(pg)));
+    }


Is the above comment still valid after your changes?

dmatveev · 2021-01-12T20:05:59Z

modules/gapi/src/compiler/gcompiler.cpp

    std::unique_ptr<ade::Graph> pG = generateGraph();
    runPasses(*pG);
-    compileIslands(*pG);
+    if (!m_metas.empty())


Can compilation go a wrong way now? What if I call a regular compile() (like in e.g. v4.2) with an empty metadata vector - what will happen?

TolyaTalamanov · 2021-03-02T09:42:17Z

Was split and merged

TolyaTalamanov force-pushed the at/wrap-gopaque branch from 2e2ce45 to 77f55fc Compare November 24, 2020 11:15

TolyaTalamanov mentioned this pull request Nov 26, 2020

[G-API] Wrap GArray #18762

Merged

6 tasks

TolyaTalamanov commented Nov 26, 2020

View reviewed changes

modules/gapi/include/opencv2/gapi/gproto.hpp Show resolved Hide resolved

TolyaTalamanov commented Nov 26, 2020

View reviewed changes

TolyaTalamanov force-pushed the at/wrap-gopaque branch from b716e9c to 353267e Compare November 26, 2020 14:44

smirnov-alexey self-requested a review November 26, 2020 14:46

TolyaTalamanov force-pushed the at/wrap-gopaque branch from f563c41 to 0c015e1 Compare November 30, 2020 10:06

smirnov-alexey reviewed Dec 4, 2020

View reviewed changes

modules/gapi/include/opencv2/gapi/gkernel.hpp Show resolved Hide resolved

smirnov-alexey reviewed Dec 4, 2020

View reviewed changes

modules/gapi/include/opencv2/gapi/gcomputation.hpp Show resolved Hide resolved

smirnov-alexey reviewed Dec 4, 2020

View reviewed changes

modules/gapi/test/gapi_gcomputation_tests.cpp Show resolved Hide resolved

TolyaTalamanov force-pushed the at/wrap-gopaque branch 2 times, most recently from ea95239 to 6c6180c Compare December 16, 2020 10:23

TolyaTalamanov commented Dec 16, 2020

View reviewed changes

dmatveev reviewed Dec 16, 2020

View reviewed changes

TolyaTalamanov force-pushed the at/wrap-gopaque branch 2 times, most recently from 8f1e0e6 to 4b85848 Compare December 28, 2020 12:50

Wrap GOpaque

e5b7976

TolyaTalamanov force-pushed the at/wrap-gopaque branch from 4b85848 to e5b7976 Compare December 28, 2020 12:52

pyopencv_from_generic_vec for vector<bool>

4cc17a7

TolyaTalamanov commented Jan 12, 2021

View reviewed changes

TolyaTalamanov requested review from dmatveev and smirnov-alexey January 12, 2021 10:36

smirnov-alexey reviewed Jan 12, 2021

View reviewed changes

modules/gapi/misc/python/test/test_gapi_infer.py Show resolved Hide resolved

smirnov-alexey approved these changes Jan 12, 2021

View reviewed changes

dmatveev requested changes Jan 12, 2021

View reviewed changes

This was referenced Jan 13, 2021

[G-API] Introduce GOpaque and GArray for python #19319

Merged

[G-API] Introduce cv.gin/cv.descr_of for python #19322

Merged

TolyaTalamanov closed this Mar 2, 2021


		auto ins = callback(m_priv->m_lastCompiled.priv().inInfo());

		// NB: After getting the arguments, recompile the graph with the correct meta

Uh oh!

Conversation

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

Uh oh!

Pull Request Readiness Checklist

Build configuration

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalek commented Nov 27, 2020

Uh oh!

TolyaTalamanov commented Nov 30, 2020

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!

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 Dec 16, 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!

TolyaTalamanov commented Dec 16, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TolyaTalamanov commented Jan 12, 2021

Uh oh!

smirnov-alexey commented Jan 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

TolyaTalamanov commented Nov 23, 2020 •

edited

Loading

smirnov-alexey Dec 16, 2020 •

edited

Loading