[G-API] GStreamingExecutor abstract interface by OrestChura · Pull Request #21816 · opencv/opencv

OrestChura · 2022-04-04T10:34:20Z

This pull request separates GStreamingExecutor into interface and GTBBStreamingExecutor which is a TBB implementation for the streaming mode 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

sivanov-work · 2022-04-04T12:25:50Z

modules/gapi/src/executor/gstreamingexecutor.cpp

+    // Need to eliminate throwings from stop(): make stop_impl() & wait_shutdown() no_throw
    try {
-        if (state == State::READY || state == State::RUNNING)
+        if (state == State::READY || state == State::RUNNING) {


optional:
here or in stop_impl it'd better to put GAPI_LOG_INFO(nullptr, "<executor name> is stopping, state: " << state);

sivanov-work · 2022-04-04T12:28:33Z

modules/gapi/src/executor/gstreamingexecutor.cpp

-
-    if (state == State::STOPPED)
+    if (state == State::STOPPED) {
        return false;


GAPI_LOG_INFO("executor has stopped: no data available")

sivanov-work · 2022-04-04T12:32:15Z

modules/gapi/src/executor/gstreamingexecutor.cpp


-    GAPI_Assert(m_sink_queues.size() == outs.size());
+    GAPI_Assert(GModel::Graph(*m_orig_graph).metadata().get<Protocol>().out_nhs.size() == outs.size() &&
+                "Number of data objects in cv::gout() must match the number of graph outputs in cv::GOut()");


Can it be placed under GAPI_DbgAssert ? Then it doesn't evaluated for Release builds

sivanov-work · 2022-04-04T12:35:11Z

modules/gapi/src/executor/gstreamingexecutor.cpp

-    if (cv::util::holds_alternative<Stop>(cmd))
-    {
+    cv::GRunArgs this_result;
+    if (!try_pull_impl(this_result)) {


The only difference between pulll and try_pull is here?

sivanov-work · 2022-04-04T12:40:28Z

modules/gapi/src/executor/gstreamingexecutor.cpp

    try {
-        if (state == State::READY || state == State::RUNNING)
+        if (state == State::READY || state == State::RUNNING) {
            stop();


UB if stop is a virtual or invoke a virtual method inside. I checked code below and find out that it triggers stop_impl which is virtual and UB is possible

sivanov-work · 2022-04-04T12:57:15Z

modules/gapi/src/executor/gtbbstreamingexecutor.cpp

+    void clear() {
+        for (auto &q : m_sync_queues) q.clear();
+        m_sync_queues.clear();
+        m_synchronized_emitters.clear();


Are there guarantees that join called before clear?
otherwise i suggest to put join inside clear

sivanov-work · 2022-04-04T12:57:38Z

modules/gapi/src/executor/gtbbstreamingexecutor.cpp

+    // Pipeline depth is equal to number of its (pipeline) steps.
+    auto has_queue_capacity = cv::gapi::getCompileArg<cv::gapi::streaming::queue_capacity>(m_comp_args);
+    const auto queue_capacity = has_queue_capacity ? has_queue_capacity->capacity :
+            3*std::count_if


sivanov-work · 2022-04-04T12:59:44Z

modules/gapi/src/executor/gtbbstreamingexecutor.cpp

+        // Produces the next video frame when pulled.
+        case T::index_of<cv::gapi::wip::IStreamSource::Ptr>():
+#if !defined(GAPI_STANDALONE)
+            emitter.reset(new VideoEmitter{emit_arg});


GAPI_LOG_INFO("create VideoEmitter ")

sivanov-work · 2022-04-04T13:00:05Z

modules/gapi/src/executor/gtbbstreamingexecutor.cpp

+        default:
+            // Create a constant emitter.
+            // Produces always the same ("constant") value when pulled.
+            emitter.reset(new ConstEmitter{emit_arg});


GAPI_LOG_INFO("create ConstEmitter" );

sivanov-work · 2022-04-04T13:05:36Z

modules/gapi/src/executor/gtbbstreamingexecutor.hpp

+
+class GTBBStreamingExecutor final : public GStreamingExecutor {
+protected:
+    const bool m_desync;


I suggest to implement TWO different GTBBStreamingExecutor:
GTBB_SYNC_StreamingExecutor ---> GTBBStreamingExecutor <-- GTBB_DESYNC_StreamingExecutor

Do do not stick flag-based logic through single and huge GTBBStreamingExecutor

@sivanov-work wouldn't it involve a huge chunk of code being copy-pasted?

sivanov-work · 2022-04-04T13:11:43Z

I suggest that we separated utils-like queues , reader and so on into different files. Because it is hard to investigate these. Also they could be reused in different components
Also i suggest that we should move out const desync flag and apply further division of TBB Executor on multiple descendants: for sync & desync cases.

smirnov-alexey · 2022-04-05T15:50:05Z

modules/gapi/src/compiler/gstreaming.cpp

 bool cv::GStreamingCompiled::Priv::pull(cv::GOptRunArgsP &&outs)
 {
-    return m_exec->pull(std::move(outs));
+    auto* exec = dynamic_cast<cv::gimpl::GTBBStreamingExecutor*>(m_exec.get());


Why do we need those checks in each pull?

smirnov-alexey · 2022-04-05T15:52:55Z

modules/gapi/src/executor/gstreamingexecutor.hpp

+    bool virtual pull_impl(cv::GRunArgs& this_result) = 0;
+    bool virtual try_pull_impl(cv::GRunArgs& this_result) = 0;
+    void virtual stop_impl() = 0;
+    void virtual wait_shutdown() = 0;


smirnov-alexey · 2022-04-05T15:53:51Z

modules/gapi/src/executor/gstreamingexecutor.hpp

-    void wait_shutdown();
-
-    cv::GTypesInfo out_info;
+    void virtual start_impl() = 0;


Could you, please, explain why _impls are virtual and not the default functions?

I see now that there is some little logic in those default functions. Is it OK to apply that logic for each potential nested executor class? If so, I'm OK with having virtual _impls

smirnov-alexey · 2022-04-05T15:57:02Z

modules/gapi/src/executor/gtbbstreamingexecutor.cpp

+    static const char *name() { return "DesyncSpecialCase"; }
+};
+
+std::vector<Q*> reader_queues(      ade::Graph &g,


Alignment is broken here and below in input_queues

smirnov-alexey · 2022-04-05T15:57:42Z

modules/gapi/src/executor/gtbbstreamingexecutor.cpp

+        bool available = std::get<2>(it);
+
+        using T = cv::GOptRunArgP;
+#define HANDLE_CASE(Type)                                               \


Please, align the \

smirnov-alexey · 2022-04-05T15:58:47Z

modules/gapi/src/executor/gtbbstreamingexecutor.cpp

+            auto &mat_opt = *cv::util::get<O<cv::Mat>*>(out_obj);
+            if (available) {
+                auto q_map = cv::util::get<cv::RMat>(res_obj).access(cv::RMat::Access::R);
+                // FIXME: Copy! Maybe we could do some optimization for this case!


Shouldn't we be concerned about this copy? Is there a task to fix it later if needed?

smirnov-alexey · 2022-04-05T16:12:19Z

modules/gapi/src/executor/gstreamingexecutor.cpp

+    // Need to eliminate throwings from stop(): make stop_impl() & wait_shutdown() no_throw
    try {
-        if (state == State::READY || state == State::RUNNING)
+        if (state == State::READY || state == State::RUNNING) {


Shouldn't we make state an atomic/guard it? I guess the question is not related to this PR but still

Please, ignore if it's not a critical section

Make GStreamingExecutor abstract

5b5295a

OrestChura added the category: g-api / gapi label Apr 4, 2022

OrestChura requested review from TolyaTalamanov, dmatveev, sivanov-work and smirnov-alexey April 4, 2022 10:34

sivanov-work reviewed Apr 4, 2022

View reviewed changes

smirnov-alexey reviewed Apr 5, 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 Apr 4, 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!

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!

sivanov-work commented Apr 4, 2022

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 Apr 5, 2022 • 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!

smirnov-alexey Apr 5, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smirnov-alexey Apr 5, 2022 •

edited

Loading

smirnov-alexey Apr 5, 2022 •

edited

Loading