G-API: Python. Desync. by mpashchenkov · Pull Request #20298 · opencv/opencv

mpashchenkov · 2021-06-22T09:05:13Z

Wrapping desync operation for Python.

Questionable place of m_desync flag.

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

Magic commands:

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.2.0
build_image:Custom Mac=openvino-2021.2.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=*

TolyaTalamanov · 2021-06-22T09:58:04Z

modules/gapi/misc/python/shadow_gapi.hpp

           class GAPI_EXPORTS_W IStreamSource { };
       } // namespace wip
+
+       namespace streaming {


put bracket on the next line

TolyaTalamanov · 2021-06-23T12:35:31Z

modules/gapi/misc/python/pyopencv_gapi.hpp

+    switch (v.index())
+    {
+        case GOptRunArg::index_of<cv::optional<cv::Mat>>():
+            return pyopencv_from(*util::get<cv::optional<cv::Mat>>(v));


Implement pyopencv_from(optional<>)

Consider Py_RETURN_NONE

TolyaTalamanov · 2021-06-23T12:39:36Z

modules/gapi/misc/python/pyopencv_gapi.hpp

+}
+
+template<>
+PyObject* pyopencv_from(const GOptRunArgs& value)


Make it more generic

Added generic variant of function.

TolyaTalamanov · 2021-06-23T12:39:58Z

modules/gapi/misc/python/pyopencv_gapi.hpp

+            return pyopencv_from(util::get<cv::GOptRunArgs>(v));
+        default:
+            PyErr_SetString(PyExc_TypeError, "Failed to recognize OptRunArgs");
+            return Py_None;


return NULL

TolyaTalamanov · 2021-06-23T12:40:07Z

modules/gapi/misc/python/pyopencv_gapi.hpp

+            PyErr_SetString(PyExc_TypeError, "Failed to recognize OptRunArgs");
+            return Py_None;
+    }
+    return Py_None;


return NULL

TolyaTalamanov · 2021-06-23T12:42:02Z

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

                if proc_num_frames == max_num_frames:
                    break

+        def test_desync(self):


Would be great to have more complicated test

TolyaTalamanov · 2021-06-23T12:55:47Z

modules/gapi/src/compiler/gcompiler.cpp

        compiled.priv().setup(std::move(pE));
    }
    else GAPI_Assert(false && "Impossible happened -- please report a bug");
+    compiled.priv().getDesync() = cgr.metadata().contains<Desynchronized>();


Let's start with a getter in GStreamingExecutor

alalek · 2021-06-30T20:51:01Z

PR rebase is required to resolve conflicts.

dmatveev · 2021-07-01T06:56:03Z

@mpashchenkov please push an updated version so we prioritize the review.

TolyaTalamanov

BTW have already implemented desync to support arbitrary G-types ?

TolyaTalamanov · 2021-07-01T07:49:03Z

modules/gapi/misc/python/pyopencv_gapi.hpp

 }

+template<>
+PyObject* pyopencv_from(const GRunArgs& value)


template<> PyObject* pyopencv_from(const GRunArgs& value) { return value.size() == 1 ? pyopencv_from(value[0]) : pyopencv_generic_vec_from(values); }

TolyaTalamanov · 2021-07-01T07:49:35Z

modules/gapi/misc/python/pyopencv_gapi.hpp


-template<>
-PyObject* pyopencv_from(const GRunArgs& value)
+template<typename T>


I believe this function can be removed, see below

TolyaTalamanov · 2021-07-01T07:49:55Z

modules/gapi/misc/python/pyopencv_gapi.hpp

+template<>
+PyObject* pyopencv_from(const GOptRunArgs& value)
+{
+    return pyopencv_from_run_args(value);


TolyaTalamanov · 2021-07-01T07:51:03Z

modules/gapi/misc/python/pyopencv_gapi.hpp

+    {
+        return pyopencv_from(*opt);
+    }
+    else


Can it be without else branch ?

TolyaTalamanov · 2021-07-01T07:52:20Z

modules/gapi/misc/python/pyopencv_gapi.hpp

+}
+
+template <>
+PyObject* pyopencv_from(const cv::util::variant<cv::GRunArgs, cv::GOptRunArgs>& v)


Put a FIXME that variant can be wrapped once for all types

TolyaTalamanov · 2021-07-01T07:57:30Z

modules/gapi/src/compiler/gstreaming.cpp

+            {
+                OpaqueRef ref;
+                util::get<ConstructOpaque>(info.ctor)(ref);
+                args.emplace_back(cv::util::make_optional(ref));


Can the ref be moved to make_optional ?

Moved to make_optional now.

TolyaTalamanov · 2021-07-01T07:58:08Z

modules/gapi/src/compiler/gstreaming.cpp

+    bool is_over = false;
+
+    using runArgs = cv::util::variant<cv::GRunArgs, cv::GOptRunArgs>;
+    if (m_priv->isDesync()) {


I would name hasDesync

And removed.

TolyaTalamanov · 2021-07-01T08:00:18Z

modules/gapi/src/compiler/gstreaming_priv.hpp


    void setInInfo(const GTypesInfo& info) { m_in_info = std::move(info); }
    const GTypesInfo& inInfo() const { return m_in_info; }
+    bool isDesync() { return m_exec->isDesync(); };


@dmatveev Is there any graceful way to check whether graph contains desync ?
We didn't find :(

Removed. All logic in gstreamingexecutor now.

TolyaTalamanov · 2021-07-01T08:02:08Z

modules/gapi/test/streaming/gapi_streaming_tests.cpp

+            EXPECT_EQ(1u, outputs.size());
+            auto opt_mat = cv::util::get<cv::optional<cv::Mat>>(outputs[0]);
+            auto with_value = opt_mat.has_value();
+            EXPECT_TRUE(with_value);


EXPECT_TRUE(opt_mat.has_value);

Have a look how it's done in other test files

TolyaTalamanov · 2021-07-01T08:03:36Z

modules/gapi/test/streaming/gapi_streaming_tests.cpp

    EXPECT_EQ(0., cv::norm(out, out_ref, cv::NORM_INF));
 }

+inline void checkPullOverload(const cv::Mat& ref,


I believe it must be inside anonymous namespace instead of inline.

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

modules/gapi/misc/python/pyopencv_gapi.hpp

modules/gapi/src/compiler/gstreaming_priv.hpp

modules/gapi/src/compiler/gstreaming.cpp

modules/gapi/src/executor/gstreamingexecutor.cpp

modules/gapi/src/executor/gstreamingexecutor.hpp

dmatveev

Good to go! Approved

dmatveev · 2021-07-01T16:36:13Z

modules/gapi/src/compiler/gstreaming_priv.hpp

    void start();
    bool pull(cv::GRunArgsP &&outs);
    bool pull(cv::GOptRunArgsP &&outs);
+    std::tuple<bool, cv::util::variant<cv::GRunArgs, cv::GOptRunArgs>> pull();


I believe there was already pull(void) introduced by @TolyaTalamanov back in the early Python binding development time - which allocated its outputs which are not optional. Is it gone now? Is it replaced by this version now?

No, there was GStreamingCompiled::pull not Priv::pull.

modules/gapi/src/executor/gstreamingexecutor.cpp

dmatveev

Go ahead and merge!

dmatveev · 2021-07-01T18:03:02Z

modules/gapi/src/executor/gstreamingexecutor.hpp


    void wait_shutdown();

+    cv::GTypesInfo out_info;


I believe it should start with m_ like other fields but please fix that LATER.

TolyaTalamanov

Great, we did it !!! 🔥 👍

TolyaTalamanov · 2021-07-01T19:10:32Z

modules/gapi/src/compiler/gstreaming_priv.hpp

    void start();
    bool pull(cv::GRunArgsP &&outs);
    bool pull(cv::GOptRunArgsP &&outs);
+    std::tuple<bool, cv::util::variant<cv::GRunArgs, cv::GOptRunArgs>> pull();


No, there was GStreamingCompiled::pull not Priv::pull.

TolyaTalamanov · 2021-07-01T19:11:11Z

modules/gapi/src/executor/gstreamingexecutor.cpp

    GAPI_Assert(m_collector_map.size() > 0u);
    m_out_queue.set_capacity(queue_capacity * m_collector_map.size());
+
+    // FIXME: The code duplicates logic of collectGraphInfo()


collectGraphInfo() can be moved to more general file.

TolyaTalamanov · 2021-07-01T19:12:58Z

modules/gapi/src/executor/gstreamingexecutor.hpp

    void start();
    bool pull(cv::GRunArgsP &&outs);
    bool pull(cv::GOptRunArgsP &&outs);
+    std::tuple<bool, cv::util::variant<cv::GRunArgs, cv::GOptRunArgs>> pull();


I would put that it used only for python

TolyaTalamanov · 2021-07-01T19:13:06Z

modules/gapi/test/streaming/gapi_streaming_tests.cpp

+                       const bool has_output,
+                       cv::util::variant<cv::GRunArgs, cv::GOptRunArgs>& args) {
+    EXPECT_TRUE(has_output);
+    using runArgs = cv::util::variant<cv::GRunArgs, cv::GOptRunArgs>;


TolyaTalamanov · 2021-07-01T19:13:51Z

modules/gapi/test/streaming/gapi_streaming_tests.cpp

+    EXPECT_FALSE(ccomp.running());
+}
+
+TEST(GAPI_Streaming_Desync, Python_Pull_Overload)


Let's discuss tests offline

G-API: Python. Desync. * Desync. GMat. * Alignment

mpashchenkov added test category: python bindings category: g-api / gapi labels Jun 22, 2021

mpashchenkov requested a review from TolyaTalamanov June 22, 2021 09:05

mpashchenkov force-pushed the mp/python-desync branch from d195d67 to d4613e7 Compare June 22, 2021 09:22

TolyaTalamanov reviewed Jun 23, 2021

View reviewed changes

mpashchenkov force-pushed the mp/python-desync branch 2 times, most recently from 5fcbd34 to 6ceed68 Compare July 1, 2021 07:38

dmatveev self-assigned this Jul 1, 2021

dmatveev added this to the 4.5.3 milestone Jul 1, 2021

TolyaTalamanov reviewed Jul 1, 2021

View reviewed changes

dmatveev requested changes Jul 1, 2021

View reviewed changes

mpashchenkov force-pushed the mp/python-desync branch from 6302d8e to c0ef34d Compare July 1, 2021 13:07

dmatveev approved these changes Jul 1, 2021

View reviewed changes

Maxim Pashchenkov added 2 commits July 1, 2021 20:18

Desync. GMat.

9844dc6

Alignment

f05e7eb

mpashchenkov force-pushed the mp/python-desync branch from 39f9564 to f05e7eb Compare July 1, 2021 17:33

dmatveev approved these changes Jul 1, 2021

View reviewed changes

alalek merged commit 05f1939 into opencv:master Jul 1, 2021

TolyaTalamanov reviewed Jul 1, 2021

View reviewed changes

alalek mentioned this pull request Oct 15, 2021

(5.x) Merge 4.x #20886

Merged

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

Merge pull request opencv#20298 from mpashchenkov:mp/python-desync

0c55d16

G-API: Python. Desync. * Desync. GMat. * Alignment

Uh oh!

Conversation

mpashchenkov commented Jun 22, 2021

Pull Request Readiness Checklist

Magic commands:

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!

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!

alalek commented Jun 30, 2021

Uh oh!

dmatveev commented Jul 1, 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!

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!

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