Fixed handling of new stream, especially for stateful OCV kernels by AsyaPronina · Pull Request #21731 · opencv/opencv

AsyaPronina · 2022-03-16T15:40:54Z

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

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

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

There is a stateful kernel in OCV which can preserve the state to work with it. Once new stream happens we should re-initialize the state from the scratch. For the Streaming G-API mode, re-initialization is called automatically once new stream happened, for Regular G-API mode user should take care of it.

To update OCV stateful kernels for a new stream, handleNewStream() should be called on the GCPUExecutable instance.
In the Streaming mode, GStreamingExecutor::setSource() method handles that, as it is indeed responsible for configuration of the executor for a new stream and notifying other peers about it.
In the Regular mode, user should call prepareForNewStream() on GCompiled instance.

BUG description

Problem was in that, that in streaming G-API mode, on the first graph run, kernels' states initialization is called twice for the GComputation compiled with metadata.
Main root cause here is that kernel states initialization isn't ran just in response to GCPUExecutable::handleNewStream(). It is also called upon the GCPUExecutable instantiation -- in the GCPUExecutable constructor.
So, firstly, kernel states are initialized in GCPUExecutable constructor called at graph compilation stage if the metadatas are passed. And, secondly, during the first run, they will also be re-initialized by GStreamingExecutor::setSource() call, notifying about new happened stream and called GCPUExecutable::handleNewStream().

It wasn't the case for graphs compiled without meta, because the GCompiler::compileStreaming() call doesn't launch GCPUExecutable constructor in case if metadata is not provided.
But, GStreamingExectutor::setSource() calls GCPUExecutable constructor.
In GStreamingExecutor::setSource() execution goes to islands recompilation or graph reshape in case if there is no original metadata. Reshape was not the case for OCV backend until recently and execution always falls to the islands recompilation. Islands recompilation will call the GCPUExecutable constructor and setup kernel states.
But after that, there is a check in GStreamingExecutor::setSource() if re-compilation was called or not and if called, then no notification about new stream will be sent to the peers and no second call to kernel states re-initialization will happen.

There are 2 problems here:

not unified notifying mechanism of the new stream from GStreamingExecutor in different scenarios. What will happened if multiple backends will handle handleNewStream() call and not only OCV?
GStreamingExecutor knows something about independent backends(that OCV backend calls states initialization in constructor) and acts based on this knowledge leading to tight coupling between backend component and itself.

Solution

Solution was:

To unify notification mechanism and call handleNewStream() unconditionally in GStreamingExecutor::setSource().
Unify kernel states initialization calling conditions - only in response to handleNewStream() call.
For the Regular G-API mode, there is no code which notifies about new stream (because it is user responsibility to let G-API graph know that previous call to apply() was for previous stream and new call to apply() is already for new one). But, G-API needs somehow say about new stream for the first time, to prepare all the stuff before execution. Because there were no previous calls to apply() and everything is clear, G-API can notify internal components itself about new stream.
So, call to the prepareForNewStream() was added to the GExecutor::run() method under condition that it will be called only once.

modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp

AsyaPronina

Thanks a lot, Ruslan!

rgarnov

Looks good, thanks!

modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp

dmatveev

Issues:

No meaningful description to the problem and the way you solve it. I can't approve it without that; I won't reconstruct the understating from the code either. In the future, MRs with no proper description won't be accepted. Review can't start if there's no description, especially if it is a bug you're fixing. Which bug?? Why it is a bug? What is the solution?
Tests on regular mode execution are missing. Are you sure it is working OK?

modules/gapi/src/executor/gexecutor.cpp

dmatveev · 2022-03-22T18:29:38Z

modules/gapi/src/executor/gexecutor.cpp


-    // Run the script
+    // Ask every IslandExecutable to prepate its internal states for new coming stream (4)
+    std::call_once(m_prep_flag, [this](){ this->prepareForNewStream(); });


So this happens only once. Is this flag reset anywhere?

No, this flag can't be reset as I've got it
By the way, this flag is different for any different object of GExecutor.
So, I might say that prepareForNewStream() is called once per each GExecutor.

This is wrong.

It may be called at any moment for a GCompiled and NO internal recompilation may happen.

https://docs.opencv.org/4.x/d2/d2c/classcv_1_1GCompiled.html#ab3f7a54ed698a86ca6c63b4351eddb20

Also. Should this happen in run or during the gexecutor construction? When it is constructed, valid meta should be there already/

Yes, I mean, first prepareForNewStream() is called once per each GExecutor, which is not called by user.
Yes, it should be called upon the GExecutor construction, std::call_once is used to emulate similar behaviour.
But, in the GExecutor execution model it seemed for me that it just fits better to be as 4th step:
// Naive execution model is similar to current CPU (OpenCV) plugin
// execution model:
// 1. Allocate all internal resources first (NB - CPU plugin doesn't do it)
// 2. Put input/output GComputation arguments to the storage
// 3. For every Island, prepare vectors of input/output parameter descs
// 4. Ask every GIslandExecutable to prepate its internal states for a new stream
// 5. Iterate over a list of operations (sorted in the topological order)
// 6. For every operation, form a list of input/output data objects
// 7. Run GIslandExecutable
// 8. writeBack
And I think. that 4th step should be implemented in run(). But I was wrong, it shouldn't be exactly in run().

Thanks, fixed!!

Note I am not talking here about the initialization procedure.
prepareForNewStream can be called by user manually for the existing GExecutor.

modules/gapi/src/executor/gstreamingexecutor.cpp

- Fixed explanation comments - Expanded test for stateful OCV kernels in Regular mode

AsyaPronina · 2022-03-24T15:59:15Z

Issues:

No meaningful description to the problem and the way you solve it. I can't approve it without that; I won't reconstruct the understating from the code either. In the future, MRs with no proper description won't be accepted. Review can't start if there's no description, especially if it is a bug you're fixing. Which bug?? Why it is a bug? What is the solution?

Tests on regular mode execution are missing. Are you sure it is working OK?

Hello! Thanks a lot! Added description and test for regular mode!

dmatveev · 2022-03-25T18:33:44Z

modules/gapi/src/executor/gexecutor.cpp


-    // Run the script
+    // Ask every IslandExecutable to prepate its internal states for new coming stream (4)
+    std::call_once(m_prep_flag, [this](){ this->prepareForNewStream(); });


Also. Should this happen in run or during the gexecutor construction? When it is constructed, valid meta should be there already/

dmatveev · 2022-03-25T18:36:38Z

modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp

+                cv::compile_args(cv::gapi::kernels<GOCVCountStateSetups>(), params));
+        EXPECT_TRUE(params.pSetupsCount != nullptr);
+        EXPECT_EQ(1, *params.pSetupsCount);
+    }


So there is still not test on regular mode entering a new stream.

Yes, you are right, there is no test which tests right new stream and state after that, but there is a test which does such testing in the context of another test case:

opencv/modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp

Line 265 in a5bf23f

callsCounter.prepareForNewStream();

By the way, it seems needed to add such separate test

Thanks, fixed!

but there is a test which does such testing in the context of another test case:

That test should fail with your previous implementation. Obviously it tests some other aspect.

- Moved notification about new stream to the constructor

- Moved notification about new stream to the constructor - Added test on state reset for Regular mode

sivanov-work

OPTIONAL:
added several hint to turn logger up

sivanov-work · 2022-03-28T13:02:43Z

modules/gapi/src/executor/gexecutor.cpp

    } // for(gim nodes)
+
+    // (4)
+    prepareForNewStream();


I suggest you add logger somewhere in

void ***:prepareForNewStream() { GAPI_LOG_INFO(nullptr, "inputs nodes: " << m_ops.size()) <---- for (auto &op : m_ops) { op.isl_exec->handleNewStream(); } }

sivanov-work · 2022-03-28T13:04:15Z

modules/gapi/src/executor/gexecutor.cpp

-    // 5. For every operation, form a list of input/output data objects
-    // 6. Run GIslandExecutable
-    // 7. writeBack
+    // 4. Ask every GIslandExecutable to prepate its internal states for a new stream


As for me - it's good idea to put logger with INFO in such places 1,2,3...

sivanov-work · 2022-03-28T13:06:13Z

modules/gapi/src/executor/gexecutor.cpp


-    // Run the script
+    // Run the script (5)
    for (auto &op : m_ops)


GAPI_LOG_DEBUG(nullptr, "Run the script for inputs nodes: " << m_ops.size())

sivanov-work · 2022-03-28T13:06:44Z

modules/gapi/src/executor/gexecutor.cpp


-    // (7)
+    // (8)
    for (auto it : ade::util::zip(ade::util::toRange(proto.outputs),


GAPI_LOG_DEBUG(nullptr, "WriteBack ".... )

sivanov-work · 2022-03-28T13:07:19Z

modules/gapi/src/executor/gstreamingexecutor.cpp

-        {
-            op.isl_exec->handleNewStream();
-        }
+        // Notify island executable about a new stream to let it update its internal variables.


LOG_INFO/DEBUG(nullptr, "Notify island executable about a new stream to let it update its internal variables.")

sivanov-work · 2022-03-28T13:09:38Z

modules/gapi/src/executor/gstreamingexecutor.cpp

+        op.isl_exec->handleNewStream();

        m_threads.emplace_back(islandActorThread,
                               op.in_objects,


If islandActorThread is infinitive actor thread (while(true)) then it is good idea to improve it

islandActorThread () ... GAPI_LOG_INFO(..., <name of island executor> " Started") while(true) { } GAPI_LOG_INFO(..., Finished")

sivanov-work · 2022-03-28T13:10:03Z

modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp

+
+    struct CountStateSetupsParams
+    {
+        int* pSetupsCount = nullptr;


Thanks a lot for the catch!!
I 've chosen to not use exactly weak_ptr, because think that CountStateSetupsParams is good enough to be owner of passed memory
Fixed with std::shared_ptr, not std::unique_ptr, just to allow copying

but if you indented to use copy then how's about mltithreading? do wee need to make operation ++ atomic?

sivanov-work · 2022-03-28T13:11:43Z

modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp

+        static void setup(const cv::GMatDesc &, std::shared_ptr<int> &,
+                          const cv::GCompileArgs &compileArgs)
+        {
+            auto params = cv::gapi::getCompileArg<CountStateSetupsParams>(compileArgs)


if it once-time triggered then i suggest:

auto params =... if () { } GAPI_LOG_INFO(nullptr, "done with setup count: " << params.pSetupsCount ? *params.pSetupsCount : nullptr)

dmatveev · 2022-03-29T22:47:36Z

modules/gapi/src/executor/gexecutor.cpp

-    // 5. For every operation, form a list of input/output data objects
-    // 6. Run GIslandExecutable
-    // 7. writeBack
+    // 4. Ask every GIslandExecutable to prepate its internal states for a new stream


prepate -> prepare

Also, (4) is misleading. It doesn't happen on any run.

dmatveev · 2022-03-29T22:52:14Z

modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp

+                cv::compile_args(cv::gapi::kernels<GOCVCountStateSetups>(), params));
+        EXPECT_TRUE(params.pSetupsCount != nullptr);
+        EXPECT_EQ(1, *params.pSetupsCount);
+    }


but there is a test which does such testing in the context of another test case:

That test should fail with your previous implementation. Obviously it tests some other aspect.

dmatveev · 2022-03-29T22:54:48Z

modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp

+    // States re-initialization will be called upon their kernels executions only
+    EXPECT_EQ(1, *params.pSetupsCount);


But why?
Now this handleNewStream() method is called for every island executable immediately on prepareForNewStream(), isn't it? In this case, I'd expect to see 2 here.

modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp

AsyaPronina · 2022-03-31T09:29:15Z

OPTIONAL: added several hint to turn logger up

Hello, Sergey! Could I create for now a ticket for me to update code according to the comments?

sivanov-work · 2022-03-31T11:58:55Z

Hi Asya,
ticket has been created

dmatveev

Better but still have some questions.
Can be addressed later, though.

dmatveev · 2022-04-04T14:09:38Z

modules/gapi/src/backends/cpu/gcpubackend.cpp

+                GAPI_LOG_WARNING(NULL,
+                    "\nGCPUExecutable::reshape was called. Resetting states of stateful kernels.");


Shouldn't this be printed every time or there are reasons to say it only once?

Only not to spam this message every reshape() if it happens often

dmatveev · 2022-04-04T14:14:33Z

modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp

    run("cv/video/1920x1080.avi", "mog2");
 }
+
+TEST(StatefulKernel, StateIsResetOnceOnReshapeInStreaming)


Why do we care about reshape in streaming?
If source changed, reset should happen - isn't it?

This test is here to highlight an issue that we can have 2 setup() calls in such cases

rgarnov · 2022-04-04T14:35:40Z

@alalek can we proceed with the merge please?

…am_event Fixed handling of new stream, especially for stateful OCV kernels * Fixed handling of new stream, especially for stateful OCV kernels * Removed duplication from StateInitOnce tests * Addressed review comments for PR opencv#21731 - Fixed explanation comments - Expanded test for stateful OCV kernels in Regular mode * Addressed review comments for PR opencv#21731 - Moved notification about new stream to the constructor - Added test on state reset for Regular mode * Addresed review comments * Addressed review comments Co-authored-by: Ruslan Garnov <ruslan.garnov@intel.com>

AsyaPronina requested a review from rgarnov March 16, 2022 15:40

AsyaPronina force-pushed the asyadev/fix_new_stream_event branch 2 times, most recently from 4e40049 to a31dfe8 Compare March 16, 2022 17:15

rgarnov requested changes Mar 16, 2022

View reviewed changes

modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp Outdated Show resolved Hide resolved

Fixed handling of new stream, especially for stateful OCV kernels

3c2c3ba

AsyaPronina force-pushed the asyadev/fix_new_stream_event branch from a31dfe8 to 3c2c3ba Compare March 17, 2022 14:33

Removed duplication from StateInitOnce tests

05829ef

AsyaPronina commented Mar 17, 2022

View reviewed changes

rgarnov approved these changes Mar 17, 2022

View reviewed changes

modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp Outdated Show resolved Hide resolved

rgarnov requested a review from dmatveev March 21, 2022 10:41

dmatveev self-assigned this Mar 22, 2022

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

dmatveev added this to the 4.6.0 milestone Mar 22, 2022

dmatveev requested changes Mar 22, 2022

View reviewed changes

Addressed review comments for PR opencv#21731

a5bf23f

- Fixed explanation comments - Expanded test for stateful OCV kernels in Regular mode

dmatveev reviewed Mar 25, 2022

View reviewed changes

AsyaPronina requested a review from sivanov-work March 28, 2022 09:53

AsyaPronina added a commit to AsyaPronina/opencv that referenced this pull request Mar 28, 2022

Addressed review comments for PR opencv#21731

c5a9c4b

- Moved notification about new stream to the constructor

Addressed review comments for PR opencv#21731

d940969

- Moved notification about new stream to the constructor - Added test on state reset for Regular mode

AsyaPronina force-pushed the asyadev/fix_new_stream_event branch from c5a9c4b to d940969 Compare March 28, 2022 12:47

sivanov-work reviewed Mar 28, 2022

View reviewed changes

dmatveev reviewed Mar 29, 2022

View reviewed changes

smirnov-alexey reviewed Mar 30, 2022

View reviewed changes

modules/gapi/test/cpu/gapi_ocv_stateful_kernel_tests.cpp Outdated Show resolved Hide resolved

Addresed review comments

fb6d6ff

Addressed review comments

45307a3

dmatveev approved these changes Apr 4, 2022

View reviewed changes

alalek merged commit 91a5e75 into opencv:4.x Apr 4, 2022

opencv-pushbot mentioned this pull request Apr 23, 2022

(5.x) Merge 4.x #21903

Merged

		// States re-initialization will be called upon their kernels executions only
		EXPECT_EQ(1, *params.pSetupsCount);

		GAPI_LOG_WARNING(NULL,
		"\nGCPUExecutable::reshape was called. Resetting states of stateful kernels.");

Uh oh!

Conversation

AsyaPronina commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

BUG description

Solution

Uh oh!

Uh oh!

AsyaPronina left a comment

Choose a reason for hiding this comment

Uh oh!

rgarnov left a comment

Choose a reason for hiding this comment

Uh oh!

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!

AsyaPronina Mar 28, 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!

Uh oh!

Uh oh!

AsyaPronina commented Mar 24, 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!

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!

Choose a reason for hiding this comment

Uh oh!

AsyaPronina commented Mar 16, 2022 •

edited

Loading

AsyaPronina Mar 28, 2022 •

edited

Loading