G-API: Connect OneVPL source and OpenCL backend #22559

smirnov-alexey · 2022-09-23T11:13:24Z

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

modules/gapi/src/backends/ocl/goclcore.cpp

smirnov-alexey · 2022-09-23T11:57:47Z

@mpashchenkov @TolyaTalamanov @sivanov-work @dmatveev please, take a look. In terms of design I suggest to merge this initial version for the functionality and redesign in the future if it will be needed for other adapters. Please, address the comments above. All this PR's credits go to @mpashchenkov

smirnov-alexey · 2022-09-23T11:58:27Z

I also need to double check local build after the changes

SergeyIvanov87

I think we need Unit Tests for goclbackend functionality

modules/gapi/samples/one_vpl_source_to_bgr_conv.cpp

modules/gapi/src/backends/ocl/goclcore.cpp

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

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

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

TolyaTalamanov

Great work 🔥

I'd also add some tests since sample doesn't run on CI. I'd also think if sample is even needed :)

modules/gapi/src/backends/ocl/goclcore.cpp

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

modules/gapi/src/backends/ocl/goclcore.cpp

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

modules/gapi/samples/one_vpl_source_to_bgr_conv.cpp

TolyaTalamanov · 2022-09-28T14:26:04Z

Do you have any performance numbers?

dmatveev

So.. does this actually work now? :)

modules/gapi/samples/one_vpl_source_to_bgr_conv.cpp

modules/gapi/src/backends/ocl/goclcore.cpp

modules/gapi/include/opencv2/gapi/streaming/onevpl/utils.hpp

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

SergeyIvanov87

Please move out extra platform dependencies from utils.hpp/cpp. It must not be here, utils is supposed to be zero-dependency component

modules/gapi/src/backends/ocl/goclcore.cpp

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

modules/gapi/include/opencv2/gapi/streaming/onevpl/utils.hpp

modules/gapi/samples/one_vpl_source_to_bgr_conv.cpp

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

smirnov-alexey · 2022-10-10T10:38:22Z

Do you have any performance numbers?

Once we are ready to merge it I'll measure the performance locally (since currently we might still be optimizing something)

SergeyIvanov87

I don't like 'default' functions and 'parse_cfg_params' inside VPL core utils: we had better separate high-level components and low-level components utilities in different files at least.
Also i see no reason to wrap up 3 rows in a single function with confusing meaning 'default_src'. Because there is no 'default' configuration at all

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

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

modules/gapi/src/backends/ocl/goclcore.cpp

SergeyIvanov87

DefaultVPLSource must not be here

modules/gapi/include/opencv2/gapi/streaming/onevpl/default.hpp

modules/gapi/samples/one_vpl_source_to_bgr_conv.cpp

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

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

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

SergeyIvanov87

LGTM

modules/gapi/src/streaming/onevpl/demux/async_mfp_demux_data_provider.cpp

modules/gapi/samples/onevpl_source_to_bgr_conv.cpp

modules/gapi/src/backends/ocl/goclcore.cpp

alalek · 2022-11-03T04:39:35Z

wants to merge 10 commits

Conflicting files
modules/gapi/samples/onevpl_infer_single_roi.cpp

Please,

squash commits into 1
rebase patch to resolve conflicts

smirnov-alexey · 2022-11-07T14:21:48Z

Do you have any performance numbers?

146 fps on 768x576 input. If needed it could be optimized, I guess

alalek · 2022-11-07T17:16:51Z

modules/gapi/src/backends/ocl/goclcore.cpp

+        // FIXME: assuming here that the context is always the same
+        // TODO: Textures are reusable, so to improve the peroformance here
+        //       consider creating a hash map texture <-> device/ctx
+        static thread_local cv::ocl::Context& ctx = cv::directx::ocl::initializeContextFromD3D11Device(pD3D11Device);


static thread_local cv::ocl::Context&

Storing reference is unsafe in this way.

ocl::Context is based on shared_ptr approach (PImpl pattern). So there is no huge problem to copy it.

Fixed, thank you

smirnov-alexey · 2022-11-09T16:16:37Z

modules/gapi/test/streaming/gapi_streaming_vpl_core_test.cpp

+
+    // Compare results
+    // FIXME: investigate why 2 sources produce different number of frames sometimes
+    for (size_t i = 0; i < std::min(ocl_mats.size(), cpu_mats.size()); ++i)


@TolyaTalamanov @SergeyIvanov87 @dmatveev any objections on that? If you have any ideas how to check ocl + vpl interaction differently please introduce them

Does this check work?
If yes - i suggest we should keep it as is

And may be investigate the frames distinguish issue in next time if it should prioritized.

For troubleshooting it we need:

at first - check frames count when decoding RAW file (without container). In this case we are cutting off WFP (multiple working threads & queues) and checking VPL decondig engine only

If all is OK with 1 step then we need for decoding containerized video (as in your example). In this case additional thread-workers come in play.

But it seems like additional task might be required not related to current PR

So, i'm OK with it :)

Yes, it works. I opened a separate bug on that with a reproducer. I also suggest to merge as is if nobody objects

SergeyIvanov87

LGTM

SergeyIvanov87 · 2022-11-09T16:37:18Z

modules/gapi/test/streaming/gapi_streaming_vpl_core_test.cpp

+    };
+
+    std::vector<CfgParam> source_cfgs;
+    source_cfgs.push_back(create_from_string("mfxImplDescription.AccelerationMode:MFX_ACCEL_MODE_VIA_D3D11"));


i suppose it should be simple way, like: CfgParam::create()

please feel free to ignore it: comment not to apply

SergeyIvanov87 · 2022-11-09T16:44:50Z

modules/gapi/test/streaming/gapi_streaming_vpl_core_test.cpp

+
+    // Compare results
+    // FIXME: investigate why 2 sources produce different number of frames sometimes
+    for (size_t i = 0; i < std::min(ocl_mats.size(), cpu_mats.size()); ++i)


Does this check work?
If yes - i suggest we should keep it as is

And may be investigate the frames distinguish issue in next time if it should prioritized.

For troubleshooting it we need:

at first - check frames count when decoding RAW file (without container). In this case we are cutting off WFP (multiple working threads & queues) and checking VPL decondig engine only

If all is OK with 1 step then we need for decoding containerized video (as in your example). In this case additional thread-workers come in play.

But it seems like additional task might be required not related to current PR

So, i'm OK with it :)

smirnov-alexey · 2022-11-10T14:24:10Z

@alalek can we merge this?

smirnov-alexey · 2022-11-11T12:07:09Z

@asmorkalov can we merge this?

alalek · 2022-11-11T16:11:56Z

dmatveev self-assigned this on Oct 3

Dmitry as G-API lead defines the final approver for G-API functionality. Currently he holds this role (no delegation) and there is no approval.

dmatveev

All good but please be careful about headers!

dmatveev · 2022-11-16T09:51:41Z

modules/gapi/include/opencv2/gapi/streaming/onevpl/default.hpp

+#ifndef OPENCV_GAPI_STREAMING_ONEVPL_UTILS_HPP
+#define OPENCV_GAPI_STREAMING_ONEVPL_UTILS_HPP
+
+#include "opencv2/gapi/own/exports.hpp" // GAPI_EXPORTS


Shouldn't this be in <>? Note it is a public header (shipped with the compiled OpenCV)

cc: @alalek

not necessary

Ref: https://stackoverflow.com/questions/21593/what-is-the-difference-between-include-filename-and-include-filename

dmatveev · 2022-11-16T10:51:40Z

modules/gapi/src/backends/ocl/goclcore.cpp

 #include "backends/ocl/goclcore.hpp"
+#include <opencv2/gapi/util/throw.hpp>


Please order the includes properly (public headers mixed with the internal ones)

dmatveev · 2022-11-16T10:52:21Z

modules/gapi/src/backends/ocl/goclcore.cpp

+#pragma comment(lib, "dxgi")
+#undef NOMINMAX
+#undef D3D11_NO_HELPERS
+#include "opencv2/core/directx.hpp"


dmatveev · 2022-11-16T10:52:26Z

modules/gapi/src/backends/ocl/goclcore.cpp

+#endif // HAVE_D3D11
+#endif // HAVE_DIRECTX
+
+#include "opencv2/core/ocl.hpp"


Minor refactoring Partially address review comments Move DX-related stuff from the sample to a default source Simplify the default OneVPL config Address minor review comments Add class for the default VPL source WIP: Add initial stub for tests with description Removing default vpl source and minor refactoring Refactor default files Fix build and application crash Address review comments Add test on VPL + OCL interaction compared to CPU behavior Fix test

smirnov-alexey · 2022-11-16T15:12:36Z

@alalek can we merge this?

smirnov-alexey mentioned this pull request Sep 23, 2022

G-API: VPL source to BGR kernel #21737

Closed

6 tasks

smirnov-alexey commented Sep 23, 2022

View reviewed changes

modules/gapi/src/backends/ocl/goclcore.cpp Outdated Show resolved Hide resolved

smirnov-alexey commented Sep 23, 2022

View reviewed changes

modules/gapi/src/backends/ocl/goclcore.cpp Outdated Show resolved Hide resolved

asmorkalov requested review from dmatveev and sivanov-work September 23, 2022 11:45

asmorkalov added the category: g-api / gapi label Sep 23, 2022

smirnov-alexey commented Sep 23, 2022

View reviewed changes

modules/gapi/src/backends/ocl/goclcore.cpp Show resolved Hide resolved

smirnov-alexey requested review from TolyaTalamanov and mpashchenkov September 23, 2022 12:02

SergeyIvanov87 reviewed Sep 23, 2022

View reviewed changes

TolyaTalamanov reviewed Sep 28, 2022

View reviewed changes

dmatveev self-assigned this Oct 3, 2022

dmatveev added this to the 4.7.0 milestone Oct 3, 2022

dmatveev reviewed Oct 3, 2022

View reviewed changes

SergeyIvanov87 reviewed Oct 5, 2022

View reviewed changes

modules/gapi/include/opencv2/gapi/streaming/onevpl/utils.hpp Outdated Show resolved Hide resolved

SergeyIvanov87 reviewed Oct 5, 2022

View reviewed changes

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

SergeyIvanov87 reviewed Oct 5, 2022

View reviewed changes

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

SergeyIvanov87 requested changes Oct 5, 2022

View reviewed changes

SergeyIvanov87 reviewed Oct 5, 2022

View reviewed changes

modules/gapi/src/backends/ocl/goclcore.cpp Show resolved Hide resolved

SergeyIvanov87 reviewed Oct 5, 2022

View reviewed changes

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

dmatveev reviewed Oct 6, 2022

View reviewed changes

SergeyIvanov87 requested changes Oct 10, 2022

View reviewed changes

SergeyIvanov87 requested changes Oct 12, 2022

View reviewed changes

SergeyIvanov87 self-requested a review November 2, 2022 16:28

SergeyIvanov87 approved these changes Nov 2, 2022

View reviewed changes

modules/gapi/src/streaming/onevpl/demux/async_mfp_demux_data_provider.cpp Outdated Show resolved Hide resolved

modules/gapi/samples/onevpl_source_to_bgr_conv.cpp Show resolved Hide resolved

SergeyIvanov87 reviewed Nov 2, 2022

View reviewed changes

modules/gapi/src/backends/ocl/goclcore.cpp Show resolved Hide resolved

alalek reviewed Nov 7, 2022

View reviewed changes

smirnov-alexey force-pushed the as/vpl_ocl branch 3 times, most recently from 94991ef to f3b5ca4 Compare November 8, 2022 17:38

smirnov-alexey commented Nov 9, 2022

View reviewed changes

SergeyIvanov87 approved these changes Nov 9, 2022

View reviewed changes

dmatveev approved these changes Nov 16, 2022

View reviewed changes

smirnov-alexey force-pushed the as/vpl_ocl branch from f3b5ca4 to 4c74e6d Compare November 16, 2022 13:30

alalek merged commit ce80e0d into opencv:4.x Nov 16, 2022

alalek mentioned this pull request Jan 8, 2023

(5.x) Merge 4.x #23113

Merged

		#include "backends/ocl/goclcore.hpp"
		#include <opencv2/gapi/util/throw.hpp>

G-API: Connect OneVPL source and OpenCL backend #22559

G-API: Connect OneVPL source and OpenCL backend #22559

Conversation

smirnov-alexey commented Sep 23, 2022 • edited

Pull Request Readiness Checklist

smirnov-alexey commented Sep 23, 2022

smirnov-alexey commented Sep 23, 2022

SergeyIvanov87 left a comment

Choose a reason for hiding this comment

TolyaTalamanov left a comment

Choose a reason for hiding this comment

TolyaTalamanov commented Sep 28, 2022

dmatveev left a comment

Choose a reason for hiding this comment

SergeyIvanov87 left a comment

Choose a reason for hiding this comment

smirnov-alexey commented Oct 10, 2022

SergeyIvanov87 left a comment

Choose a reason for hiding this comment

SergeyIvanov87 left a comment

Choose a reason for hiding this comment

SergeyIvanov87 left a comment

Choose a reason for hiding this comment

alalek commented Nov 3, 2022

smirnov-alexey commented Nov 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyIvanov87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smirnov-alexey commented Nov 10, 2022

smirnov-alexey commented Nov 11, 2022

alalek commented Nov 11, 2022

dmatveev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smirnov-alexey commented Nov 16, 2022

smirnov-alexey commented Sep 23, 2022 •

edited