G-API: oneVPL Add VPP IE preproc by sivanov-work · Pull Request #21363 · opencv/opencv

sivanov-work · 2021-12-29T11:45:11Z

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

sivanov-work · 2021-12-29T11:45:54Z

@sivanov-work rebase on #21232 as soon as merged

mpashchenkov

I'll look again later.

mpashchenkov · 2022-01-27T09:25:46Z

modules/gapi/src/backends/ie/giebackend.cpp

+                                        out_keep_alive_frame);
+                *out_keep_alive_frame = frame;
+            }
+        } // oherwise it is not suitable frame, then check on other preproc backend or rely on IE plugin


oherwise -> otherwise

mpashchenkov · 2022-01-27T11:07:10Z

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_engine.cpp

+                                    "MFX_ERR_REALLOC_SURFACE is not processed");
+            break;
+        case MFX_WRN_IN_EXECUTION:
+            /*try {


for debugging purposes and should be removed

mpashchenkov · 2022-01-31T07:56:11Z

modules/gapi/src/backends/ie/giebackend.cpp

+            GAPI_LOG_DEBUG(nullptr, "VPP preprocessing for decoded frame will be used");
+
+            auto inputs = ctx.uu.net.getInputsInfo();
+            const auto &input_name = ctx.uu.params.input_names.at(0);


The name can be sent from Infer::run() function. But it will only be used here.

And 0->i, but you know it.

incorrect in generic case:

I. InferROI:
1 is for blob, 0 - for input

IE::Blob::Ptr this_blob = extractBlob(*ctx, 1, cv::gapi::ie::TraitAs::IMAGE, slot_ptr); setROIBlob(req, *(ctx->uu.params.input_names.begin()), this_blob, this_roi, *ctx);

II. The same problem in InferList
III. the more complex in InferList2

in retrospective vision: if was needed to modify setBlob instead extractBlob
I'll try to make some changes in setBlob and revert extractBlob changes

mpashchenkov · 2022-01-31T08:07:10Z

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


 GSource::Priv::Priv() :
-    mfx_handle(MFXLoad()),
+//    mfx_handle(MFXLoad()),


to do not forget uncomment it after VPL releases their bug fix

mpashchenkov · 2022-01-31T08:08:24Z

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

    MFXDispReleaseImplDescription(mfx_handle, mfx_impl_description);
    GAPI_LOG_INFO(nullptr, "Unload MFX handle: " << mfx_handle);
-    MFXUnload(mfx_handle);
+    //MFXUnload(mfx_handle);


M B it will be uncommented in the future.

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

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

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

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_session.hpp

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

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

sivanov-work · 2022-02-03T09:00:21Z

modules/gapi/src/backends/ie/giebackend.cpp

    GAPI_Assert(ctx.inShape(i) == cv::GShape::GFRAME &&
                "Remote blob is supported for MediaFrame only");
+    cv::MediaFrame frame = ctx.inFrame(i);
+#ifdef HAVE_ONEVPL


try remove defines

TolyaTalamanov · 2022-02-07T07:51:52Z

modules/gapi/CMakeLists.txt

 if(HAVE_GAPI_ONEVPL)
  # NB: TARGET opencv_perf_gapi doesn't exist before `ocv_add_perf_tests`
  if(TARGET opencv_perf_gapi)
+    target_include_directories(opencv_perf_gapi PRIVATE "${CMAKE_CURRENT_LIST_DIR}/src")


Why it's needed?

default include path doen't provide search for gapi/src/ path. but it require to turn on perf tests preprocengine with vpl source together

What do you want to test from gapi/src ?
Honestly we do the same for opencv_test_gapi with HUGE comment, but it's better to avoid if it's possible

TolyaTalamanov · 2022-02-07T08:38:11Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

+                                                             std::declval<std::tuple<resize_t>>()));
+class OneVPLSourcePerf_PP_Test : public TestPerfParams<source_description_preproc_t> {};
+
+PERF_TEST_P_(OneVPLSourcePerf_PP_Test, TestPerformance)


What does it measure? resize+crop?

crop - means extract roi from frame
why you asked?

I just didn't get what this test actually do

it's performance test: collect absolute metrics for decode + preproc execution. It is used in comparison with perf test for decode only

TolyaTalamanov · 2022-02-07T08:41:41Z

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_engine.hpp

+
+    cv::util::optional<pp_params> is_applicable(const cv::MediaFrame& in_frame) override;
+
+    pp_session initialize_preproc(const pp_params& params,


Can this preproc be unbind from OV API?

OV2.0 doesn't have InputInfo

It allows to get rid from linkage with OV in perf tests.

We need to implement custom wrap over InputInfo which would fully represent it functionality
But as you said that there OV2.0 is coming - it looks redundant work for me to spend some time to realize how this wrapper would be like.
Instead i would wait for OV2.0 outcome (when it officially released) and merge InputInfo and OV2.0 eventually together.
@TolyaTalamanov do you agree?

I hope that it will be unbind from IE specific classes, let's discuss this open offline with team

TolyaTalamanov · 2022-02-07T08:44:33Z

modules/gapi/CMakeLists.txt

        ocv_target_include_directories(opencv_perf_gapi SYSTEM PRIVATE ${OPENCL_INCLUDE_DIRS})
    endif()
+
+    if(OPENCV_GAPI_INF_ENGINE)


Definitely unnecessary to link perf tests with IE

preproc uses IE network info to set up its parameters.

@TolyaTalamanov Have you any ideas how to overcome that in simplest way?

Probably don't build sample at all. @smirnov-alexey do you remember how to properly handle it?

TolyaTalamanov · 2022-02-07T08:46:31Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

 #include <opencv2/gapi/streaming/cap.hpp>

+#ifdef HAVE_INF_ENGINE
+#include <inference_engine.hpp>


What's exactly needed from inference_engine.hpp ? I guess InputsInfo, then see comments below

TolyaTalamanov · 2022-02-07T09:43:56Z

modules/gapi/src/backends/ie/giebackend.cpp

+            if (out_keep_alive_frame != nullptr) {
+                GAPI_LOG_DEBUG(nullptr, "remember preprocessed frame to keep it busy from reuse, slot: " <<
+                                        out_keep_alive_frame);
+                *out_keep_alive_frame = frame;


hmmm, but you assigned it to frame which is already located ctx, so it shouldn't be destroyed, should it?

Please correct me if i didn't understand and and confusing you with my answer:
I allocate empty MediaFrame as a slot in ctx beforehand in req.execute because i must retain frame per-request in general way
Here, i reuse slot with emptyMediaFrame and put my new preprocessed frame in this slot by copying. It is fully allowable because MediaFrame contains shared pointer on its adapter which ensure guarantee that MediaAdapter would not be destroyed until last link in slot is alive

My point was, why do you need extra out_keep_alive_frame if you already assigned new pre-processed frame to context ?

context may serve multiple requests in general way: but it require to distinguish which request must destroy particular frame. Thus new additional pair of function were introduces to satisfy "minimal code changes" purposes

TolyaTalamanov · 2022-02-07T09:47:20Z

modules/gapi/src/backends/ie/giebackend.cpp

+#ifdef HAVE_ONEVPL
+#ifdef HAVE_DIRECTX
+#ifdef HAVE_D3D11
+    if (ctx.uu.prepoc_engine_impl) {


The condition of enabling preprocessing isn't clear again.

Preprocessing should be enabled in case:

User specified GPU OV plugin

Frame came from VPL Source (has VPL adapter)

Actually it is

User specified GPU OV plugin

see https://github.com/opencv/opencv/pull/21363/files#diff-437e5530bca341dae63e89a5b62027702b1b4ee3d954e99d490b24a6127a2350R253
for creation. VA_DEVICE -- means GPU plugin

and

Frame came from VPL Source (has VPL adapter)

Applicable condition prepoc_engine_impl->is_applicable(frame)

for creation. VA_DEVICE -- means GPU plugin

It should something more explicit

Applicable condition prepoc_engine_impl->is_applicable(frame)

But firstly we have to make sure that frame comes with appropriate media adapter

It should something more explicit

My aim in implementing such functionality was "minimal user query" and "maximum automatization"
If user had passed param VA_DEVICE already then it was clear purpose to use such device in InferRemoteCtx and prepare PP context

But firstly we have to make sure that frame comes with appropriate media adapter

is_applicable makes checking out on appropriate media adapter and decides: is that MediaFrame could be preprocessed by existing PP engine or not

TolyaTalamanov · 2022-02-07T10:07:44Z

modules/gapi/src/backends/ie/giebackend.cpp

                                 std::size_t i,
-                                 cv::gapi::ie::TraitAs hint) {
+                                 cv::gapi::ie::TraitAs hint,
+                                 cv::MediaFrame* out_keep_alive_frame = nullptr) {


Why it's needed as separate function parameter? You can put frame into context

You can put frame into context

i need key which identifying request which i must hold frame. Because finally I got PostCallback which identifies request but not frame to deallocate

To simplify & improve logic we must refactore each part of existing extracBlob and it's neighborhoods. But current implementation carries out minimal changes approach and implement fast-fix solution

TolyaTalamanov · 2022-02-07T10:41:48Z

modules/gapi/samples/onevpl_infer_single_roi.cpp

-                                            {"VA_DEVICE", accel_device_ptr} });
-
+                                              {"VA_DEVICE", accel_device_ptr},
+        /*TODO turn on auto vpp preproc*/     {"GAPI_DEVICE_CTX", accel_ctx_ptr} });


InferenceEngine::ParamMap shouldn't contain information not related with IE plugins such as GAPI_DEVICE_CTX

Agree,
It just a prototyped solution. There were discussion to use these pointer as a part of cv::gapi::ie::Params and planned as this PR refactoring
@TolyaTalamanov Do you agree?

It must discussed offline with team

Previous discussion finished with:

cv::gapi::ie::Params add method cfg_device_contet(device_ptr, context_ptr) to hold on device & contrxt ptr, which should be used in giebackend as remote ctx & preproc engine creation

TolyaTalamanov · 2022-02-07T10:44:38Z

modules/gapi/src/backends/ie/giebackend.cpp

            rctx = ie_core.CreateContext(params.device_id, *ctx_params);
+
+            // Guess additional parameters is enough for creating preproc engine
+            auto dev_param_it = ctx_params->find("VA_DEVICE");


If it's IE::ParamMap content, why it's handled in our code???

IE::ParamMap is intended to pass specific device into OV to make a remote context
Because we need in specific device too - i REused it here too to avoid produce new redundant entities

Let's discuss it offline as well

TolyaTalamanov · 2022-02-07T11:15:35Z

Open questions

When VPL preprocessing should be enabled?
AT: User set onevpl::GSource and specified GPU plugin for cv::gapi::ie::Params.
What is needed for instance preprocessing engine?
Which backends/platforms should it cover?
How it looks like for user?

mpashchenkov · 2022-02-07T15:08:47Z

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_engine.cpp

+        acceleration_policy->init(mfx_vpp_session);
+        try {
+            // ask to allocate external memory pool
+            mfxFrameAllocRequest vppRequests[2];


I have lost thread. Why is 2?

it is MFX/VPL API: MFXVideoVPP_QueryIOSurf returns array of 2 elements

it is possible to add naming variable...

mpashchenkov · 2022-02-07T15:09:32Z

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_engine.cpp

+                throw std::runtime_error("Cannot execute MFXVideoVPP_QueryIOSurf");
+            }
+
+            // NB: Assing ID as upper limit descendant to distinguish specific VPP allocation


Assing -->Assign

mpashchenkov · 2022-02-07T15:13:16Z

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_engine.cpp

+            // The decoder detected a new sequence header in the bitstream.
+            // Video parameters may have changed.
+            // In external memory allocation case, might need to reallocate the output surface
+            /*GAPI_DbgAssert(false && "VPLLegacyDecodeEngine::process_error - "


Should it be uncommented?

(in decode_engine_legacy.cpp too)

i don't know how to process this WRN because it looks like doesn't affect on anything and happens spontaneously in the beginning of stream
I suppose we need to add logger here and make it falls through

mpashchenkov · 2022-02-07T15:18:27Z

modules/gapi/test/streaming/gapi_streaming_vpp_preproc_test.cpp

+    mfxSession mfx_vpp_session {nullptr};
+    sts = MFXCloneSession(mfx_decode_session, &mfx_vpp_session);
+    ASSERT_EQ(MFX_ERR_NONE, sts);
+    */


Should it work?

should be deleted

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_engine.cpp

OrestChura · 2022-02-07T20:55:40Z

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_engine.cpp

+                }
+            } while (MFX_ERR_NONE == sess.last_status && !my_sess.vpp_out_queue.empty());
+            return ExecutionStatus::Continue;
+        },


This "Wait for ASYNC decode result" lambda is big and also common with VPLLegacyDecodeEngine - should it be shared?

Idea is the same but it's different is particular places:
different queue, different surfaces and different on_frame_ready

Yes, OK..
I believe the differences can be handled by introducing some using redefines (like session_type you did in PreprocEngine) - and referring to the different types and methods via this->session_type and this->on_frame_ready.
So, as this can be LegacyDecodeEngine or PreprocEngine, the lambda will automatically switch between usings and methods.

I still think it is useful to share the code between classes, for better understanding and sometimes easier maintenance.
But maybe it's just not for now :)
Can this refactoring be planned for the future?

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_engine.cpp

OrestChura · 2022-02-08T01:40:01Z

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_engine.cpp

+            // The decoder detected a new sequence header in the bitstream.
+            // Video parameters may have changed.
+            // In external memory allocation case, might need to reallocate the output surface
+            /*GAPI_DbgAssert(false && "VPLLegacyDecodeEngine::process_error - "


(in decode_engine_legacy.cpp too)

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_session.hpp

modules/gapi/src/streaming/onevpl/engine/processing_engine_interface.hpp

modules/gapi/src/backends/ie/giebackend.cpp

sivanov-work · 2022-02-18T12:04:31Z

It was decided to separate the PR on two parts:
-PP core: #21618
-and further integration part

current PR would be closed

sivanov-work requested review from TolyaTalamanov and mpashchenkov January 20, 2022 09:20

dmatveev self-assigned this Jan 20, 2022

dmatveev added the category: g-api / gapi label Jan 20, 2022

dmatveev added this to the 4.6.0 milestone Jan 20, 2022

sivanov-work mentioned this pull request Jan 24, 2022

G-API: oneVPL DX11 inference #21232

Merged

6 tasks

sivanov-work added 10 commits January 25, 2022 09:16

Add BaseMediAdapter for VPL

9b6386f

Add PreprocSession & PreprocEngine interface part

f55e1db

Implement preproc UT, Fix base path

4b515b7

Add common PP interface, add common pp_params

d866cf7

Rough decoupling VPL & Preproc

3772197

Add syntax sugar for PP interface

d06e809

Integrate VPP preproc in GIEbackend

c5f8323

Add PP bypass

351d3cc

Add perf tests for PP

5c0b54a

Fix warning in vpl core UT

10a6cf6

sivanov-work force-pushed the vpp_preproc branch from 4bc950e to 5c0b54a Compare January 25, 2022 06:53

sivanov-work added 7 commits January 25, 2022 13:05

Add inner preproc resolution Unit Test

2d98ffb

Remove VPP preproc description from single ROI sample

a04e2f4

Apply SetROIBlob for diferent Infer operations

47d4143

Eliminate extra branch-lines for cfg_param_parser & transcode_engine

dd282f7

Fix UT warning &PreprocSession compile

90e7d9c

Fix compilation & warnings

2e6a230

Reduce Session&Engine code amount

0207678

sivanov-work marked this pull request as ready for review January 26, 2022 13:43

sivanov-work requested a review from OrestChura January 26, 2022 13:43

mpashchenkov reviewed Jan 31, 2022

View reviewed changes

OrestChura reviewed Feb 2, 2022

View reviewed changes

sivanov-work mentioned this pull request Feb 2, 2022

G-API: oneVPL Add ROI support in VPP IE preproc #21554

Closed

6 tasks

sivanov-work commented Feb 3, 2022

View reviewed changes

TolyaTalamanov reviewed Feb 7, 2022

View reviewed changes

mpashchenkov reviewed Feb 7, 2022

View reviewed changes

OrestChura reviewed Feb 8, 2022

View reviewed changes

Apply some comments

144affc

sivanov-work mentioned this pull request Feb 15, 2022

G-API: Add VPL/VPP preproc core module #21618

Merged

6 tasks

sivanov-work closed this Feb 18, 2022


		cv::util::optional<pp_params> is_applicable(const cv::MediaFrame& in_frame) override;

		pp_session initialize_preproc(const pp_params& params,

Uh oh!

Conversation

sivanov-work commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Build Configuration

Uh oh!

sivanov-work commented Dec 29, 2021

Uh oh!

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

Uh oh!

Uh oh!

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!

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!

sivanov-work commented Dec 29, 2021 •

edited

Loading

TolyaTalamanov Feb 8, 2022 •

edited

Loading