G-API: oneVPL - Performance: Add async decode pipeline & add cached pool #20901

sivanov-work · 2021-10-19T05:39:28Z

This PR is a part of large PR #20469

Improve performance of VPL pipeline processing:

make it by fully async: queue multiple VPL decode requests to utilize full VPL decoder usage
check decoding readiness without blocking
Turn on cached surface pool: avoid of O(n) in searching free surface from pool

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

force_builders=XCustom,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
Xbuild_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
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*


build_image:Custom Win=gapi-onevpl-2021.6.0
buildworker:Custom Win=windows-3
build_contrib:Custom Win=OFF

modules/gapi/src/streaming/onevpl/accelerators/surface/surface_pool.cpp

modules/gapi/src/streaming/onevpl/engine/decode/decode_engine_legacy.cpp

smirnov-alexey · 2021-10-20T21:18:32Z

modules/gapi/src/streaming/onevpl/engine/decode/decode_engine_legacy.cpp

+                    GAPI_LOG_WARNING(nullptr, "[" << my_sess.session <<
+                                               "] has no surface, reason: " <<
+                                               ex.what());
+                    // TODO it is supposed to place `break;` here


Is it Ok to get into this catch? I mean even now it can be just exited correctly?

The same comment can be applied to all the catch blocks below

it's OK here: swap_surface can throw exception in case of no available "free" surface is present in pool. This lack-of-resource state processing (like std::bad_alloc for new allocation ) allows us to go on async-waiting result state and put off decoder queuing job until one or more async result completed and resource freeing

The issue of bad_alloc is that it can be handled explicitly - here we handle all the exceptions uniformly and this bad-alloc / general error difference is not clear to me

This is the reason why I adhered my point about using abort() in GAPI_Assert instead of throwing exception:
because abort() means the library itself problem which is not supposed to be handed by user but introducing BUG to fix. Otherwise exception has recoverable semantic

In current catch block we could face with two different kind of exception:

swap_surface ->get_free_surface() -> find KEY in pool (exception means library problem and supposed to be abort() with BUG/Issue creation)

and the second

swap_surface ->get_free_surface()-> pool.find_free() -> no free surface at the time (resource unavailable at moment)

The second is expected to be processed.
No other exception types is throwing.

So, according to idea that GAPI_Assert has no abort inside - we need to catch only second case. So it make sense to find out what types of exception is throwing by GAPI_Assert and sort out different types in pool.find_free

modules/gapi/src/streaming/onevpl/engine/decode/decode_engine_legacy.cpp

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

AsyaPronina · 2021-10-21T11:31:35Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

+    using namespace cv::gapi::wip::onevpl;
+
+    const auto params = GetParam();
+    source_t src = findDataFile(get<0>(params));


should we put false here as second argument to not complain on file not found error? Or I am wrong?

maybe yes, i will add it

looks like false is confusing here. it added but has reverted

From my perspective:

if user didn't configured EXTRA path in here initTestDataPathSilent() then SkipException is OK here

IF user did configured EXTRA path here, then IT MUST contain requested test files. So findDataFile must request it availability

Please align it with the latest news from @mpashchenkov

asked a detailed question in @mpashchenkov PR by #20995 (comment)

#20995 (comment)
So:
test must fail if no env OPENCV_TEST_DATA_PATH is set (in case of test requires external test data)
test must fail if OPENCV_TEST_DATA_PATH set and filepath is incorrect.

Looks like i need to remove my initTestDataPathSilent check

AsyaPronina · 2021-10-21T11:33:16Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

+
+    const auto params = GetParam();
+    source_t src = findDataFile(get<0>(params));
+    codec_t type = get<1>(params);


Don't you find using std::tie(src, type) = params; better?

as for me this transforms into the same 3 lines

The idea behind using std::tie instead of get<N> is type safety

i'm not sure that it is important in UTs, because an one tuple type is for a single one test and when you changed the tuple set - it had caused by the reason that you intended to change test by itself.
If a some common tuple set (hypothetically) was changed by adding new type for a some specific single new one test then it is more convenient for me to do not brake ALL tests which depended from common tuple set by adding std::ignore for all of them (in case if i have 100500 legacy unit tests).

Additionally, AND which is more important for me: please compare

std::string path; ... std::tie(path,...) = GetParams(); path = findDataFile(path); ...

vs

auto path = findDataFile(std::get<0>(param));

because an one tuple type is for a single one test and when you changed the tuple set - it had caused by the reason that you intended to change test by itself.

Not always, unfortunately - sometimes these parameters are introduced only for particular tests in the suite and the rest is not updated.

AsyaPronina · 2021-10-21T11:35:16Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

+    std::vector<CfgParam> cfg_params {
+        CfgParam::create<std::string>("mfxImplDescription.Impl", "MFX_IMPL_TYPE_HARDWARE"),
+        CfgParam::create("mfxImplDescription.mfxDecoderDescription.decoder.CodecID", type),
+    };


Why there are difference between instantiations? std::string is not deducible?

first function deduces template argument as const char* which uses as type for construction cv::variant later.
cv::variant is pretty different from std::variant and cannot support construct_as semantic for std::string (At the moment when current PR introduced). Thus explicit template instantiation requested here due to limited functionality of cv::variant

AsyaPronina · 2021-10-21T18:06:55Z

modules/gapi/src/streaming/onevpl/accelerators/surface/surface_pool.cpp

+        std::count_if(surfaces.begin(), surfaces.end(),
+                     [](const surface_ptr_t& val) {
+            GAPI_DbgAssert(val && "Pool contains empty surface");
+            return !val->get_locks_count();


it is still more tricky to read !int()
Is this some common way to represent logic statement in some famous libraries (likes STD)?
Just a minor question, not to address!

I wonder when you asked - as for me it's well known expression, especially in C in the same way as if (num %2) and so on

+1 to Asya's point, get_lock_count() == 0 is more straight to understand

ok will change by majority request

AsyaPronina · 2021-10-21T18:10:40Z

modules/gapi/src/streaming/onevpl/engine/decode/decode_engine_legacy.cpp

+            // prepare sync object for new surface
+            LegacyDecodeSession::op_handle_t sync_pair{};
+
+            // enqueue qecode operation with current session surface


sorry for such comment!
qecode -> decode

AsyaPronina · 2021-10-21T18:44:25Z

modules/gapi/src/streaming/onevpl/engine/decode/decode_engine_legacy.cpp

    ready_frames.emplace(cv::MediaFrame(std::move(frame_adapter)), sess.generate_frame_meta());
+
+    // pop away ready sync onject


AsyaPronina · 2021-10-21T18:45:03Z

modules/gapi/src/streaming/onevpl/engine/decode/decode_engine_legacy.cpp

+
+            if (my_sess.last_status == MFX_ERR_NONE) {
+                my_sess.sync_queue.emplace(sync_pair);
+            } else if (MFX_ERR_MORE_DATA != my_sess.last_status) /* suppress MFX_ERR_MORE_DATA warning */ {


different order of operands in comparison against if above, however, such order is mostly required for == comparison...

is it intended?

it was written in different times of day :)

TolyaTalamanov · 2021-10-22T07:38:28Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

+    using namespace cv::gapi::wip::onevpl;
+
+    const auto params = GetParam();
+    source_t src = findDataFile(get<0>(params));


we usually use std::tie instead of std::get<>

TolyaTalamanov · 2021-10-22T07:42:06Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

+    };
+
+    cv::gapi::wip::IStreamSource::Ptr source_ptr;
+    try {


hmmm, wondering why it isn't under ifdef ?

it makes sense.
What is the current order for UT: to skip or to disable ?

added #ifdef HAVE_ONEVPL in the beginning

disable (put under ifdef)

modules/gapi/samples/infer_single_roi.cpp

TolyaTalamanov · 2021-10-22T07:46:15Z

modules/gapi/src/streaming/onevpl/accelerators/accel_policy_cpu.cpp

-    // create empty pool
-    pool_t pool;
-    pool.reserve(pool_size);
+    // create empty pool with reservation


I'd start with capital and usually we use these tags before comment:
NB, FIXME, TODO

sorry, what is NB ?

dmatveev · 2021-10-26T23:07:47Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

+
+    const auto params = GetParam();
+    source_t src = findDataFile(get<0>(params));
+    codec_t type = get<1>(params);


The idea behind using std::tie instead of get<N> is type safety

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

dmatveev · 2021-10-26T23:10:51Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

+    TEST_CYCLE()
+    {
+        source_ptr->pull(out);
+    }


to be honest I'm not sure how well it maps to the idea of a performance test this way.

TEST_CYCLE() measures the same thing again and again to calculate its statistic values. Here you actually decode different frames, but measure it as the same random value - not sure if it is correct.

Maybe running the whole file under the same TEST_CYCLE would make sense w.r.t stats, but then this test can take foerever...

I's true, because different frames take up different decode duration but...

It's supposed to be compared in performance metrics against VideoCap on the same input streams (and frames) AND find out hotspots by using Vtune metrics in overall pipeline. I means auxiliary code around on direct MFX decode api

Wrapping TEST_CYCLE for all case and to incorporate a source creation also will measure MFX library/DX11 CoCreate load/unload and so on and doesn't give me relative result performance in comparison with VideoCapdecode api

Wrapping TEST_CYCLE for all case and to incorporate a source creation also will measure MFX library/DX11 CoCreate load/unload and so on and doesn't give me relative result performance in comparison with VideoCapdecode api

This makes sense, but can it be done only once before the TEST_CYCLE() body kicks in? Is there a way to decompose it to prologue/epilogue so that initialization/deinitialization for every individual run is left out of profile?

dmatveev · 2021-10-26T23:11:51Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

+
+    using namespace cv::gapi::wip;
+
+    source_t src = findDataFile(GetParam(), false);


I believe this is the method which throws SkipTestException so we don't need to test it on our own

It receives initial PATH and return modified PATH according to the OPENCV_TEST_DAT_PATH prefix but for only single case if file is found by path in result.
So, if path doesn't find then no modified PATH returned and no exception was throws in case of second false params.
So i was confused by changing second params to false
Let's remove false here - false means optional, so when requested file doesn't found it returns non-modified path and throw Skip otherwise.

My suggestions:
if initTestDataPathSilent is success, that means OPENCV EXTRA is present that MUST provide test video file by requested path otherwise configuration is incorrect. so false must be removed

Please align with @mpashchenkov

dmatveev · 2021-10-26T23:12:51Z

modules/gapi/src/streaming/onevpl/accelerators/accel_policy_cpu.cpp

-    // create empty pool
-    pool_t pool;
-    pool.reserve(pool_size);
+    // create empty pool with reservation


modules/gapi/src/streaming/onevpl/utils.cpp

modules/gapi/src/streaming/onevpl/engine/decode/decode_session.cpp

dmatveev · 2021-10-28T11:59:43Z

modules/gapi/src/streaming/onevpl/engine/decode/decode_engine_legacy.cpp

+                    GAPI_LOG_WARNING(nullptr, "[" << my_sess.session <<
+                                               "] has no surface, reason: " <<
+                                               ex.what());
+                    // TODO it is supposed to place `break;` here


The issue of bad_alloc is that it can be handled explicitly - here we handle all the exceptions uniformly and this bad-alloc / general error difference is not clear to me

dmatveev · 2021-10-28T12:00:21Z

modules/gapi/src/streaming/onevpl/engine/decode/decode_engine_legacy.cpp

+                sess.swap_surface(*this);
+                return ExecutionStatus::Continue;
+            } catch (const std::exception& ex) {


dmatveev · 2021-10-28T12:00:31Z

modules/gapi/src/streaming/onevpl/engine/decode/decode_engine_legacy.cpp

+            try {
+                sess.swap_surface(*this);
+                return ExecutionStatus::Continue;
+            } catch (const std::exception& ex) {


and here too

TolyaTalamanov

LGTM :+1

TolyaTalamanov · 2021-11-02T11:06:30Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

+    };
+
+    cv::gapi::wip::IStreamSource::Ptr source_ptr;
+    try {


disable (put under ifdef)

dmatveev

Ok to merge

sivanov-work · 2021-11-10T07:46:11Z

@alalek Could you merge it please? alignment with VPL builder would be done later when all parts of vpl source merged

…perf_mod G-API: oneVPL - Performance: Add async decode pipeline & add cached pool * Add async decode pipeline & intro cached pool * Fix performacne test with checking OPENCV_EXTRA * Add sip perf test with no VPL * Fix misprint * Remove empty line.. * Apply some comments * Apply some comments * Make perf test fail if no OPENCV_TEST_DATA_PATH declared

sivanov-work added category: g-api / gapi feature labels Oct 19, 2021

sivanov-work requested review from dmatveev, AsyaPronina, smirnov-alexey and TolyaTalamanov October 19, 2021 05:42

smirnov-alexey reviewed Oct 20, 2021

View reviewed changes

modules/gapi/src/streaming/onevpl/accelerators/surface/surface_pool.cpp Show resolved Hide resolved

smirnov-alexey reviewed Oct 20, 2021

View reviewed changes

modules/gapi/src/streaming/onevpl/accelerators/surface/surface_pool.cpp Outdated Show resolved Hide resolved

smirnov-alexey reviewed Oct 20, 2021

View reviewed changes

modules/gapi/src/streaming/onevpl/engine/decode/decode_engine_legacy.cpp Show resolved Hide resolved

smirnov-alexey reviewed Oct 20, 2021

View reviewed changes

modules/gapi/src/streaming/onevpl/engine/decode/decode_engine_legacy.cpp Show resolved Hide resolved

smirnov-alexey reviewed Oct 20, 2021

View reviewed changes

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp Show resolved Hide resolved

AsyaPronina reviewed Oct 21, 2021

View reviewed changes

TolyaTalamanov reviewed Oct 22, 2021

View reviewed changes

dmatveev self-assigned this Oct 26, 2021

dmatveev added this to the 4.5.5 milestone Oct 26, 2021

dmatveev reviewed Oct 28, 2021

View reviewed changes

TolyaTalamanov approved these changes Nov 2, 2021

View reviewed changes

sivanov-work added 7 commits November 2, 2021 15:39

Add async decode pipeline & intro cached pool

2c27f87

Fix performacne test with checking OPENCV_EXTRA

579de02

Add sip perf test with no VPL

636aebd

Fix misprint

2dbb9c6

Remove empty line..

4b8b270

Apply some comments

b36a48b

Apply some comments

1d8df9f

sivanov-work force-pushed the merge_source_unite_perf_mod branch from 1220067 to 1d8df9f Compare November 2, 2021 12:48

dmatveev approved these changes Nov 8, 2021

View reviewed changes

Make perf test fail if no OPENCV_TEST_DATA_PATH declared

a6fd318

alalek merged commit da63442 into opencv:4.x Nov 10, 2021

alalek mentioned this pull request Dec 30, 2021

(5.x) Merge 4.x #21371

Merged

alalek mentioned this pull request Feb 22, 2022

(5.x) Merge 4.x #21651

Merged

sivanov-work deleted the merge_source_unite_perf_mod branch March 5, 2022 05:48

		ready_frames.emplace(cv::MediaFrame(std::move(frame_adapter)), sess.generate_frame_meta());

		// pop away ready sync onject


		using namespace cv::gapi::wip;

		source_t src = findDataFile(GetParam(), false);

Uh oh!

G-API: oneVPL - Performance: Add async decode pipeline & add cached pool #20901

G-API: oneVPL - Performance: Add async decode pipeline & add cached pool #20901

Uh oh!

Conversation

sivanov-work commented Oct 19, 2021 • edited by alalek 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!

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!

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!

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 Oct 21, 2021 • 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!

Choose a reason for hiding this comment

sivanov-work commented Oct 19, 2021 •

edited by alalek

Loading

AsyaPronina Oct 21, 2021 •

edited

Loading