[G-API] GExecutor abstract interface by OrestChura · Pull Request #21762 · opencv/opencv

OrestChura · 2022-03-22T18:54:53Z

Inheritor of #19329

This pull request separates GExecutor into interface (more accurately the base part with customization points) and GSerialExecutor which is an implementation for the naive executor.

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

OrestChura · 2022-03-22T18:58:46Z

modules/gapi/src/executor/gexecutor.hpp

+// - cv::MediaFrame        - for media buffers
 // - cv::Scalar            - for single values (with up to four components inside)
 // - cv::detail::VectorRef - an untyped wrapper over std::vector<T>
+// - cv::detail::OpaqueRef - an untyped wrapper over an object with an arbitrary type T


Is it a valid change from me? I'm just not sure these types used for exchanging between Islands

Looks valid

sivanov-work · 2022-03-23T04:56:58Z

modules/gapi/src/executor/gserialexecutor.cpp

+    }
+}
+
+void assignMetaStubExec(Mag& mag, const RcDesc &rc, const cv::GRunArg::Meta &meta) {


as for me it looks like these bunch of functions which interact with Mag should be members of this class Mag
Or located in different file at least

Could you move out this functions into different file, please?

No need to do that, Magazine class itself is not bound to its contents (it is template)

We can consider this as part of further refactoring if ever we'll need it in more than 1 place

sivanov-work

Are there any unit tests for check backward compatibility for that changes?

sivanov-work · 2022-03-23T04:58:34Z

modules/gapi/src/executor/gserialexecutor.cpp

+    virtual StreamMsg get() override
+    {
+        cv::GRunArgs res;
+        for (const auto &rc : desc()) { res.emplace_back(magazine::getArg(mag, rc)); }


is it possible to make res.reserve(desc.size()) ?

sivanov-work · 2022-03-23T05:02:50Z

modules/gapi/src/executor/gserialexecutor.cpp

+    {
+        set(rcs);
+    }
+};


I recommend moving this (input + output) out into separate files also

Input/Output implementations are executor-specific, maybe no need to move

sivanov-work · 2022-03-23T05:09:42Z

modules/gapi/src/executor/gserialexecutor.cpp

+    }
+};
+
+void cv::gimpl::GSerialExecutor::initResource(const ade::NodeHandle & nh, const ade::NodeHandle &orig_nh)


not the clearest method name: what kind of resource ? As for me methods with prefix init called at once usually to init something global resource but here we sequencially make initResource in loop.
Can we give more suitable name for that?

sivanov-work · 2022-03-23T05:20:07Z

modules/gapi/src/executor/gserialexecutor.cpp

+        // (5), (6)
+        Input i{m_res, op.in_objects};
+        Output o{m_res, op.out_objects};
+        op.isl_exec->run(i, o);


Previously

struct OpDesc { std::vector<RcDesc> in_objects; std::vector<RcDesc> out_objects; std::shared_ptr<GIslandExecutable> isl_exec; };

I cannot understand an idea: what is the reason to group in_objject, out_objects and isl_exec together and do not use it's coupling benefit. It looks like it should be decoupled or regroupped them in any other way

It is legacy, this executor has been written based on CPU backend so CPU operations turned into "Islands" (but remained "ops" for some reason)

sivanov-work · 2022-03-23T05:21:53Z

modules/gapi/src/executor/gserialexecutor.cpp

+    GAPI_Assert(canReshape());
+    auto& g = *m_orig_graph.get();
+    ade::passes::PassContext ctx{g};
+    passes::initMeta(ctx, inMetas);


if this is some kind of checks then should it be done on canReshape part?

It's not a check

smirnov-alexey · 2022-03-25T08:52:58Z

modules/gapi/src/executor/gserialexecutor.cpp

+// It is subject to the license terms in the LICENSE file found in the top-level directory
+// of this distribution and at http://opencv.org/license.html.
+//
+// Copyright (C) 2018-2022 Intel Corporation


Just 2022 I suppose

smirnov-alexey · 2022-03-25T08:57:06Z

modules/gapi/src/executor/gserialexecutor.cpp

+
+void cv::gimpl::GSerialExecutor::runImpl(cv::gimpl::GRuntimeArgs &&args)
+{
+    // (2)


Please, remove those steps or add documentation on them at the begging of the runImpl

I'd rather add that documentation piece back :)

smirnov-alexey · 2022-03-25T08:58:19Z

modules/gapi/src/executor/gserialexecutor.hpp

+// It is subject to the license terms in the LICENSE file found in the top-level directory
+// of this distribution and at http://opencv.org/license.html.
+//
+// Copyright (C) 2018-2022 Intel Corporation


dmatveev

👍 probably the what's needs to be addressed is a comment about serial executor's execution flow

dmatveev · 2022-03-30T09:17:47Z

modules/gapi/src/compiler/gcompiler.cpp

    GModel::ConstGraph cgr(*pg);
-
+    // FIXME: select which executor will be actually used,
+    // make GExecutor abstract.


GExecutor is irrelevant here, since this method is about Streaming

dmatveev · 2022-03-30T09:20:52Z

modules/gapi/src/executor/gserialexecutor.cpp

+    }
+}
+
+void assignMetaStubExec(Mag& mag, const RcDesc &rc, const cv::GRunArg::Meta &meta) {


No need to do that, Magazine class itself is not bound to its contents (it is template)

dmatveev · 2022-03-30T09:21:25Z

modules/gapi/src/executor/gserialexecutor.cpp

+    }
+}
+
+void assignMetaStubExec(Mag& mag, const RcDesc &rc, const cv::GRunArg::Meta &meta) {


We can consider this as part of further refactoring if ever we'll need it in more than 1 place

dmatveev · 2022-03-30T09:22:00Z

modules/gapi/src/executor/gserialexecutor.cpp

+    {
+        set(rcs);
+    }
+};


Input/Output implementations are executor-specific, maybe no need to move

dmatveev · 2022-03-30T09:22:38Z

modules/gapi/src/executor/gserialexecutor.cpp

+
+void cv::gimpl::GSerialExecutor::runImpl(cv::gimpl::GRuntimeArgs &&args)
+{
+    // (2)


I'd rather add that documentation piece back :)

dmatveev · 2022-03-30T09:24:12Z

modules/gapi/src/executor/gserialexecutor.cpp

+        // (5), (6)
+        Input i{m_res, op.in_objects};
+        Output o{m_res, op.out_objects};
+        op.isl_exec->run(i, o);


It is legacy, this executor has been written based on CPU backend so CPU operations turned into "Islands" (but remained "ops" for some reason)

alalek · 2022-03-30T22:52:57Z

modules/gapi/src/executor/gserialexecutor.hpp

+} // namespace gimpl
+} // namespace cv
+
+#endif // OPENCV_GAPI_GEXECUTOR_HPP


should be aligned with OPENCV_GAPI_SERIAL_EXECUTOR_HPP

or even add _SRC_: OPENCV_GAPI_SRC_SERIAL_EXECUTOR_HPP

alalek · 2022-04-01T22:41:37Z

Conflicting files
modules/gapi/src/executor/gexecutor.cpp

Please rebase to resolve conflicts.

anton-potapov and others added 3 commits March 22, 2022 12:55

G-API GExecutor internal interface

6d1fcdb

Applying recent changes from gexecutor.cpp

5abb079

Some refinements, changed class descriptions

8f24f2d

OrestChura added the category: g-api / gapi label Mar 22, 2022

OrestChura mentioned this pull request Mar 22, 2022

G-API GExecutor internal interface #19329

Closed

6 tasks

OrestChura requested review from dmatveev, rgarnov and smirnov-alexey March 22, 2022 18:56

OrestChura commented Mar 22, 2022

View reviewed changes

sivanov-work reviewed Mar 23, 2022

View reviewed changes

smirnov-alexey reviewed Mar 25, 2022

View reviewed changes

dmatveev approved these changes Mar 30, 2022

View reviewed changes

dmatveev self-assigned this Mar 30, 2022

dmatveev added this to the 4.6.0 milestone Mar 30, 2022

alalek reviewed Mar 30, 2022

View reviewed changes

dmatveev mentioned this pull request Aug 30, 2022

G-API: Introduce abstract base classes for GExecutor and GStreamingExecutor #22451

Merged

6 tasks

asmorkalov closed this Sep 5, 2022

Uh oh!

Conversation

OrestChura commented Mar 22, 2022

Pull Request Readiness Checklist

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!

sivanov-work 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!

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

alalek commented Apr 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants