Skip to content

G-API: oneVPL - Implement IDeviceSelector & default cfg_param-based selector#20738

Merged
alalek merged 15 commits intoopencv:masterfrom
sivanov-work:merge_master_vpl_dev_select
Oct 20, 2021
Merged

G-API: oneVPL - Implement IDeviceSelector & default cfg_param-based selector#20738
alalek merged 15 commits intoopencv:masterfrom
sivanov-work:merge_master_vpl_dev_select

Conversation

@sivanov-work
Copy link
Copy Markdown
Contributor

@sivanov-work sivanov-work commented Sep 23, 2021 •
edited by alalek
Loading

Designed IDeviceSelector allows user to provide some customization in selection of GPU/Accel device when set up onevpl::Source object. In future is allow to support some hybrid-pipeline when several devices selected as decode/vpp acceleration
At the moment CfgParamDeviceSelector is implementing IDeviceSelector and supporting either default device selection or externally provided DX11 device as VPL source accelerator

Example of Integration with inference high-level scenario

DeviceSelector sel (...);
onevpl::Source vpl_source(<file>, sel);
...
auto ctx = sel.get_context();
...
infer::createNetwork(..., ctx.get_ptr());
...

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

@sivanov-work sivanov-work force-pushed the merge_master_vpl_dev_select branch from fef0fed to fb18751 Compare September 23, 2021 12:40
@dmatveev dmatveev self-assigned this Sep 30, 2021
@dmatveev dmatveev added this to the 4.5.4 milestone Sep 30, 2021
dmatveev
dmatveev reviewed Sep 30, 2021
View reviewed changes
modules/gapi/src/streaming/onevpl/cfg_param_device_selector.cpp
GAPI_LOG_DEBUG(nullptr, "Add HW acceleration support");
mfxVariant accel_mode = cfg_param_to_mfx_variant(*accel_mode_it);

switch(accel_mode.Data.U32) {
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this mean?

Copy link
Copy Markdown
Contributor Author

@sivanov-work sivanov-work Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mfxVariant Data is an c-like union. each configuration parameter specifically fills its defined type in union and mfxImplDescription.AccelerationMode has U32 type value

Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so why do we handle only that case?

Copy link
Copy Markdown
Contributor Author

@sivanov-work sivanov-work Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because accel_mode.Data is c-union, and only one member U32 is correct for the value determined by "mfxImplDescription.AccelerationMode" description

let's look at this as std::variant<...> and as VPL mentioned in their Docs, value of type of mfxImplDescription.AccelerationMode should be stored as U32.

modules/gapi/src/streaming/onevpl/cfg_param_device_selector.cpp
}
mfxVariant accel_mode = cfg_param_to_mfx_variant(*accel_mode_it);

switch(accel_mode.Data.U32) {
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this case is handled in previous constuctor, too - should that logic be unified?

Copy link
Copy Markdown
Contributor Author

@sivanov-work sivanov-work Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the first ctor uses device creation from default adapter - no external pointers from user
but this ctor just wrap user-created device passed by void* pointer AND uses actual device type through CfgParams to restore type and work out some checks

modules/gapi/test/streaming/gapi_streaming_vpl_device_selector.cpp
}
}
} // namespace opencv_test
#endif // HAVE_ONEVPL
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the full test bodies be enclosed under HAVE_ macros?

Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, once and for all

Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..or the idea is to extend this with other flags as they arrive?

I'd prefer different tests cover different cases otherwise you can't say for sure what actually passed/failed in the test looking at its name when it has that lots of compile conditionals inside

Copy link
Copy Markdown
Contributor Author

@sivanov-work sivanov-work Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean
TEST(OneVPL_Source_Device_Selector_CfgParam, PtrDeviceFromCfgParam)
then
TEST(OneVPL_Source_Device_Selector_CfgParam, DX11_PtrDeviceFromCfgParam)
then
TEST(OneVPL_Source_Device_Selector_CfgParam, YYY_PtrDeviceFromCfgParam)

agree, it makes sense

Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

smirnov-alexey
smirnov-alexey reviewed Oct 7, 2021
View reviewed changes
smirnov-alexey
smirnov-alexey reviewed Oct 7, 2021
View reviewed changes
smirnov-alexey
smirnov-alexey reviewed Oct 7, 2021
View reviewed changes
smirnov-alexey
smirnov-alexey reviewed Oct 7, 2021
View reviewed changes
smirnov-alexey
smirnov-alexey reviewed Oct 7, 2021
View reviewed changes
dmatveev
dmatveev approved these changes Oct 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok to merge, thanks!

@sivanov-work sivanov-work force-pushed the merge_master_vpl_dev_select branch from de615ce to 323d81d Compare October 18, 2021 12:49
@sivanov-work sivanov-work force-pushed the merge_master_vpl_dev_select branch from 92d3e84 to 2dddeaf Compare October 19, 2021 06:46
@sivanov-work
Copy link
Copy Markdown
Contributor Author

sivanov-work commented Oct 20, 2021

@alalek could you merge it please?

@alalek alalek merged commit 1f9a7b8 into opencv:master Oct 20, 2021
@alalek
Copy link
Copy Markdown
Member

alalek commented Oct 21, 2021 •
edited
Loading

There is build regression caused by this patch: http://pullrequest.opencv.org/buildbot/builders/master_openvino-opencl-win64/builds/608

build_image:Custom Win=openvino-2021.4.1
buildworker:Custom Win=windows-3

(used machine has minimal setup of "MSVS Build Tools 2019")

@sivanov-work
Copy link
Copy Markdown
Contributor Author

sivanov-work commented Oct 21, 2021

ok, I will try to make a patch

@sivanov-work
Copy link
Copy Markdown
Contributor Author

sivanov-work commented Oct 21, 2021

added PR #20921

@sivanov-work sivanov-work mentioned this pull request Oct 21, 2021
Merged
6 tasks
@alalek alalek mentioned this pull request Dec 30, 2021
Merged
@alalek alalek mentioned this pull request Feb 22, 2022
Merged
@sivanov-work sivanov-work deleted the merge_master_vpl_dev_select branch March 5, 2022 05:48
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
@sivanov-work
Merge pull request opencv#20738 from sivanov-work:merge_master_vpl_de…
b38ac96 
…v_select

 G-API: oneVPL - Implement IDeviceSelector & default cfg_param-based selector

* Initial commit

* Add MACRO undef

* Change IDeviceSelector, Change Inf sample for choose external device

* Fix compilation

* Address some comments

* Fix compilation

* Add missing header

* Add EXPORT to dev selector

* Add guard

* Remove enum type attr

* Fix compilation without VPL

* Add HAVE_INFER guard in sample

* Remove unusable include from tests

* Remove unusable include from sample

* Remove cl_d3d11 header from unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmatveev dmatveev dmatveev approved these changes

@mshabunin mshabunin mshabunin left review comments

@smirnov-alexey smirnov-alexey smirnov-alexey approved these changes

@rgarnov rgarnov Awaiting requested review from rgarnov

@AsyaPronina AsyaPronina Awaiting requested review from AsyaPronina

@TolyaTalamanov TolyaTalamanov Awaiting requested review from TolyaTalamanov

Assignees

@dmatveev dmatveev

Labels

category: g-api / gapi

Projects

None yet

Milestone

4.5.5

Development

Successfully merging this pull request may close these issues.

6 participants

@sivanov-work @alalek @dmatveev @mshabunin @smirnov-alexey @asmorkalov