G-API: Add VPL/VPP preproc core module by sivanov-work · Pull Request #21618 · opencv/opencv

sivanov-work · 2022-02-15T10:32:19Z

Core part of #21363
VPP preproc by itself with applied comments

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

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

TolyaTalamanov · 2022-02-15T12:49:11Z

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

                               files[1],
                               files[2]));
+
+using resize_t = std::pair<uint16_t, uint16_t>;


Actually we always use cv::Size for that purposes since pair isn't representative

TolyaTalamanov · 2022-02-15T12:53:15Z

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)


This test isn't related to preprocessing, is it? Why it's in this PR then?
Honestly I didn't get the point what it actually measures here, is it resize + crop?

These tests are performance suite: it measures absolute value of execution time for different kinds of flow:
-source decode by itself
-source decode+processing (transcode)
-source decode + PPEngine (resize)
-source decode + PPEngine in bypass mode (measure pass-through PP const)

These absolute values manual comparison represents relative performance cost of components invocation

modules/gapi/perf/streaming/gapi_streaming_source_perf_tests.cpp

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

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

TolyaTalamanov · 2022-02-15T13:35:42Z

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

    }

+    /* NB:
+     * From one hand current OpenVINO API doesn't support texture array and


Sorry for that:
On the one hand/On the other hand

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

TolyaTalamanov · 2022-02-15T13:42:04Z

modules/gapi/test/streaming/gapi_streaming_vpp_preproc_test.cpp

+#include "streaming/onevpl/accelerators/accel_policy_dx11.hpp"
+#include "streaming/onevpl/accelerators/dx11_alloc_resource.hpp"
+#include "streaming/onevpl/accelerators/utils/shared_lock.hpp"
+#define private public


allows to invoke private method of testing component in UTs

TolyaTalamanov · 2022-02-16T06:00:16Z

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

@@ -572,9 +572,10 @@ static void setBlob(InferenceEngine::InferRequest& req,
 static void setROIBlob(InferenceEngine::InferRequest& req,


BTW, why it's handled only for ROI? I believe our main use case when:

IE device - GPU

User passed remote context

MediaFrame comes with VPL adapter

In that case we have to do preprocessing on our end since IE isn't able to do it for us...

In not-long time ago there was simple function SetBlob in a way SetBlob(...make_shared_blob(Blob, roi)...) which actually prepare ROI Blob from it's argument - which doesn't work in case of GPU blob
Now SetROIBlob take care about decision: prepare ROI blob (legacy way) or not (GPU approach)

OrestChura

Some small comments from the previous review

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

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

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

OrestChura

Thank you!

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

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

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

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

rgarnov · 2022-02-18T13:58:43Z

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

+    session_type::op_handle_t in_preproc_request {nullptr,
+                                                  vpl_adapter->get_surface()->get_handle()};
+    s->sync_in_queue.emplace(in_preproc_request);
+    remember_decode_frame(vpl_adapter->get_surface()->get_handle(), in_frame);


Can we remember this frame somewhere in the process() (pipeline's lambdas)?

i don't think about that..
But as far i remember pipeline-lambdas was designed as pure- and is cutting from the other world and cannot receive different kind of data across session

actually it can be passed as another aggregated member of session_type::op_handle_t as well.
Moreover a subsequent PR about ROI extention follows this approach: https://github.com/opencv/opencv/pull/21554/files#diff-d281efabce2ff069582bd99d9c40e910c4386b65d60010ed9e4b106f4f2b237bR384

@rgarnov Let's move Frame into op_handle_t, what do you think ?

modules/gapi/test/streaming/gapi_streaming_vpp_preproc_test.cpp

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

dmatveev

No major objections here, but 43 files and around 2K LOC to trigger a simple "resize" via VPL still looks way too much. As now it only can do NV12 Resize isn't it?

dmatveev · 2022-02-21T11:55:34Z

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

+        } else if ((name == CfgParam::vpp_in_width_name()) || (name == CfgParam::vpp_in_height_name()) ||
+                   (name == CfgParam::vpp_in_crop_w_name()) || (name == CfgParam::vpp_in_crop_h_name()) ||
+                   (name == CfgParam::vpp_in_crop_x_name()) || (name == CfgParam::vpp_in_crop_y_name()) ||
+                   (name == CfgParam::vpp_out_chroma_format_name()) ||
+                   (name == CfgParam::vpp_out_width_name()) || (name == CfgParam::vpp_out_height_name()) ||
+                   (name == CfgParam::vpp_out_crop_w_name()) || (name == CfgParam::vpp_out_crop_h_name()) ||
+                   (name == CfgParam::vpp_out_crop_x_name()) || (name == CfgParam::vpp_out_crop_y_name()) ||
+                   (name == CfgParam::vpp_out_pic_struct_name())) {


uh can this be done via some map lookup?

anyway it is better now than it used to be

i'm afraid it would require to introduce std::map with lambdas and to fill that lambdas in the same way as this huge if-else implemented with the same amount of conditions

dmatveev · 2022-02-21T12:10:18Z

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

+bool FrameInfoComparator::operator()(const mfxFrameInfo& lhs, const mfxFrameInfo& rhs) const {
+    return lhs < rhs;
+}
+
+bool FrameInfoComparator::equal_to(const mfxFrameInfo& lhs, const mfxFrameInfo& rhs) {
+    return lhs == rhs;
+}


Why do we need this, given that < and == are available already?

in different namespaces:
std::less which is required for std::map try to find operator< in global or std namespace and ignored cv::gapi::wip::onevpl. By this reason i use FrameInfoComparator in std::map
On the other side my UnitTests on preproc requires checking FrameInfo before and after preproc, which means i should make operator<, and operator== for FrameInfo visible as public symbols in utils.hpp which looks confusing (for me) because utils is not part of interface

On the other side, because VPPPreprocEngine is already visible (for UT) purposes and it still require operator< (because ADL doesn't work) then i put aditional operation equal_to for this comparator class and made the whole file with VPPPreprocEngine visible for UTs

dmatveev · 2022-02-21T12:12:42Z

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

+            mfxVPPParams.vpp.Out.FourCC = MFX_FOURCC_NV12;
+            break;
+        default:
+            GAPI_LOG_WARNING(nullptr, "Unsupported MediaFormat in preprocessing: " <<


it should be CRITICAL not WARNING, isn't it?

I believe @TolyaTalamanov has added it recently

Then It requires to rearrange all logger severities in namespace onevpl ( warning ->error/critical). I would implement it in the next PR including other vpl components - not preproc only

https://github.com/opencv/opencv/blob/4.x/modules/gapi/src/backends/ie/giebackend.cpp#L728

dmatveev · 2022-02-21T12:14:16Z

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

+    cv::MediaFrame run_sync(const pp_session &session_handle,
+                            const cv::MediaFrame& in_frame) override;


It produces a MediaFrame as output? What's the reason behind this?

it emphasizes that it produces new MediaFrame with surface from different pool (not from decoding one).
In bypass-mode (when input & output params are the same) then it is the same in_frame

Since method is called as run_sync then I suppose it must return result

P.S. It is possible to "upgrade" method for copy-elision and accept R-Value here. Something in a way:

std::string to_lower(std::string &&str) { ... return std::move(str); }

dmatveev · 2022-02-21T12:16:16Z

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

+#include <opencv2/gapi/media.hpp>
+#include <opencv2/gapi/util/optional.hpp>
+
+#include "streaming\onevpl\engine\preproc_defines.hpp"


nice catch! it's strange but no compiles error
Aha... it's been built for Windows only in CI by now

dmatveev · 2022-02-21T12:16:51Z

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


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


mfx_handle is a global variable by now. It was implemented to overcome oneVPL issue with cloning session:

all VPL operations (decode, VPP) must belong to mfxSession context. this mfxSession holds DevicePtr, bitstream settings and some other infrastructural data. So to queue VPP preprocessing operation we must create different mfxSession wich will share some settings with decoding mfxSession and has different another's. At least both session must share the same VPL Library Implementation and version which selected in VPLSource configuration step.
The ideas was:
pass decode session object as MediaFrameAdapter data, then PreprocEngine extract it from MediaFrameAdapter in is_applicable method and perform mfxSessionClone in order to share the MAJOR session params: Implementation, Version, Decoder ID and so; then operates with VPP cloned session and forget about decode session (to avoid huge operation for mfxLoad and choosing implementation based on selected criteria, which may be diffrent from producing VPL source and so on).

But i faced with some issue in oneVPL implementation which rejects cloning session due to some inner bug.
To overcome this issue i made mfx_handle as global variable (temporary) with limitation "only single VPL Source can exist" then i just use this global mfx_handle state to create a NEW session (not as clone) from it's state.
After new VPL version releases we will reactivate idea with Cloning Session and revert such commented lines back

dmatveev · 2022-02-21T12:16:59Z

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);


OrestChura

A small comment, please check but only if there would be some other commits required

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

sivanov-work · 2022-02-24T10:15:59Z

@alalek could you merge it please?

G-API: Add VPL/VPP preproc core module * Add BaseMediAdapter for VPL * Add PreprocSession & PreprocEngine interface part * Implement preproc UT, Fix base path * Add common PP interface, add common pp_params * Rough decoupling VPL & Preproc * Add syntax sugar for PP interface * Integrate VPP preproc in GIEbackend * Add PP bypass * Add perf tests for PP * Fix warning in vpl core UT * Add inner preproc resolution Unit Test * Remove VPP preproc description from single ROI sample * Apply SetROIBlob for diferent Infer operations * Eliminate extra branch-lines for cfg_param_parser & transcode_engine * Fix UT warning &PreprocSession compile * Fix compilation & warnings * Reduce Session&Engine code amount * Apply some comments * Revert IE changes, rename preproc * Fix for DX11 infer for OV: turn off texture array * Remove dependency PP on IE * Change fixture tests params * Apply other comments & turn off ROI for GPU * Fix compilation: remove forgotten INFER define * Apply debt comments * Fix PP UTs: add FrameInfo value comparator * Fix style * Remove standalone map for preproc frames storage * Add other comments

sivanov-work added 21 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

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

Apply some comments

144affc

Revert IE changes, rename preproc

b4e0a8a

Fix for DX11 infer for OV: turn off texture array

0b35605

Remove dependency PP on IE

bfdabff

sivanov-work requested review from TolyaTalamanov, dmatveev, mpashchenkov and rgarnov February 15, 2022 10:32

TolyaTalamanov reviewed Feb 15, 2022

View reviewed changes

TolyaTalamanov reviewed Feb 16, 2022

View reviewed changes

asmorkalov added the category: g-api / gapi label Feb 16, 2022

sivanov-work added 2 commits February 16, 2022 11:09

Change fixture tests params

195b835

Apply other comments & turn off ROI for GPU

ffaab65

Fix compilation: remove forgotten INFER define

c247be3

OrestChura reviewed Feb 17, 2022

View reviewed changes

Apply debt comments

ddb4351

OrestChura approved these changes Feb 17, 2022

View reviewed changes

Fix PP UTs: add FrameInfo value comparator

8cad85a

OrestChura reviewed Feb 18, 2022

View reviewed changes

modules/gapi/src/streaming/onevpl/utils.cpp Show resolved Hide resolved

OrestChura self-requested a review February 18, 2022 11:02

Fix style

2b28a39

OrestChura approved these changes Feb 18, 2022

View reviewed changes

sivanov-work mentioned this pull request Feb 18, 2022

G-API: oneVPL Add VPP IE preproc #21363

Closed

6 tasks

rgarnov requested changes Feb 18, 2022

View reviewed changes

dmatveev self-assigned this Feb 21, 2022

dmatveev added this to the 4.6.0 milestone Feb 21, 2022

dmatveev approved these changes Feb 21, 2022

View reviewed changes

sivanov-work added 2 commits February 22, 2022 15:24

Remove standalone map for preproc frames storage

a8dd375

Add other comments

db1f10e

sivanov-work requested a review from rgarnov February 24, 2022 07:27

OrestChura approved these changes Feb 24, 2022

View reviewed changes

modules/gapi/src/streaming/onevpl/engine/preproc/preproc_engine.cpp Show resolved Hide resolved

rgarnov approved these changes Feb 24, 2022

View reviewed changes

alalek merged commit 8f1c502 into opencv:4.x Feb 24, 2022

opencv-pushbot mentioned this pull request Apr 23, 2022

(5.x) Merge 4.x #21903

Merged

		@@ -572,9 +572,10 @@ static void setBlob(InferenceEngine::InferRequest& req,
		static void setROIBlob(InferenceEngine::InferRequest& req,

		cv::MediaFrame run_sync(const pp_session &session_handle,
		const cv::MediaFrame& in_frame) override;

Uh oh!

Conversation

sivanov-work commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Build Configuration

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!

Choose a reason for hiding this comment

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!

OrestChura left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OrestChura left a comment

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!

Uh oh!

Uh oh!

Uh oh!

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!

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 Feb 15, 2022 •

edited

Loading