CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
[G-API] Enable 2D inputs for GIEBackend #20925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[G-API] Enable 2D inputs for GIEBackend #20925
Conversation
|
||
// NB: For some reason RGB image is 2D image | ||
// (since channel component is not counted here). | ||
// In order to disnguish 2D tensor from image, have a look on model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disnguish -- > distinguish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
This branch based: #20918 |
@alalek These warnings come from
Should I just suppress them for |
@@ -207,6 +207,10 @@ if(OPENCV_GAPI_INF_ENGINE) | |||
ocv_target_link_libraries(${the_module} PRIVATE ${INF_ENGINE_TARGET}) | |||
endif() | |||
|
|||
if (HAVE_NGRAPH) | |||
ocv_target_link_libraries(${the_module} PRIVATE ngraph::ngraph) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alalek I noticed that InferenceEngine
and ngraph
are handled different way, do we need to make alias to ngraph::ngraph
like NGRAPH_TARGET
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense not to use InferenceEngine
separately from ngraph
by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separately
find_package(InferenceEngine)
and find_package(ngraph)
.
Check DNN module integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's so similar to my implementation, isn't it? Except maybe this: https://github.com/opencv/opencv/blob/master/modules/dnn/CMakeLists.txt#L162 in DNN definitions set globally, in G-API we need this only for opencv_test_gapi
, so far
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this question closed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alalek Is it OK?
PR based on this: #20918 |
@@ -22,6 +22,10 @@ | |||
#include "backends/ie/util.hpp" | |||
#include "backends/ie/giebackend/giewrapper.hpp" | |||
|
|||
#ifdef HAVE_NGRAPH | |||
#include <ngraph/ngraph.hpp> | |||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alalek These warnings come from
ngraph
572>C:\Program Files (x86)\IntelSWTools\openvino_2021.4.689\deployment_tools\ngraph\include\ngraph/op/abs.hpp(26,57): warning C4100: 'visitor': unreferenced formal parameter (compiling source file C:\build\precommit_custom_windows\opencv\modules\gapi\test\infer\gapi_infer_ie_test.cpp) [C:\build\precommit_custom_windows\build\modules\gapi\opencv_test_gapi.vcxproj]
Use this prolog code before ngraph include :
opencv/modules/core/src/hal_replacement.hpp
Lines 50 to 59 in 17234f8
#if defined(__clang__) // clang or MSVC clang | |
#pragma clang diagnostic push | |
#pragma clang diagnostic ignored "-Wunused-parameter" | |
#elif defined(_MSC_VER) | |
#pragma warning(push) | |
#pragma warning(disable : 4100) | |
#elif defined(__GNUC__) | |
#pragma GCC diagnostic push | |
#pragma GCC diagnostic ignored "-Wunused-parameter" | |
#endif |
and similar prolog code (check referenced file).
/cc @ilya-lavrenov FYI (public headers should not emit warnings)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, works fine now, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alalek it's fixed in latest master (checked by openvinotoolkit/openvino#8403 where I don't have to fix such issues)
Secondly, you can include IE headers as SYSTEM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as SYSTEM
Even on Windows and MSVC?
in latest master
Upcoming release is 2021.4 LTS update. So we still forced to use/validate that.
@@ -207,6 +207,10 @@ if(OPENCV_GAPI_INF_ENGINE) | |||
ocv_target_link_libraries(${the_module} PRIVATE ${INF_ENGINE_TARGET}) | |||
endif() | |||
|
|||
if (HAVE_NGRAPH) | |||
ocv_target_link_libraries(${the_module} PRIVATE ngraph::ngraph) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this question closed now?
@@ -22,6 +22,20 @@ | |||
#include "backends/ie/util.hpp" | |||
#include "backends/ie/giebackend/giewrapper.hpp" | |||
|
|||
#ifdef HAVE_NGRAPH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should tests link with ngraph::ngraph
too? I don't remember if it is the case for CMake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, but ngraph
is enabled by using -DWITH_NGRAPH=ON
and this flag isn't mandatory to use IE
itself.
So if user just do cmake
as same as usual: -DWITH_INF_ENGINE=ON
ngraph won't be found.
@ilya-lavrenov Could you help, please? Do you know whether ngraph
will be mandatory part of IE
in upcoming OpenVINO 2.0
release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but please close the linker question
494aa33
to
0d2531f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@alalek I believe it's ready to be merged |
…ts-giebackend [G-API] Enable 2D inputs for GIEBackend * Enable 2D inputs * Fix typo
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.
Build configuration