CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
G-API: Introduce ONNX backend for Inference #18716
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
Conversation
- Basic operations are implemented (Infer, -ROI, -List, -List2); - Implemented automatic preprocessing for ONNX models; - Test suite is extended with `OPENCV_GAPI_ONNX_MODEL_PATH` env for test data (test data is an ONNX Model Zoo repo snapshot); - Fixed kernel lookup logic in core G-API: - Lookup NN kernels not in the default package, but in the associated backend's aux package. Now two NN backends can work in the same graph. - Added Infer SSD demo and a combined ONNX/IE demo;
d1f665f
to
c2d983d
Compare
cmake/FindONNX.cmake
Outdated
@@ -0,0 +1,31 @@ | |||
ocv_clear_vars(HAVE_ONNX) | |||
|
|||
set(ORT_INSTALL_DIR "" CACHE PATH "ONNX Runtime install directory") |
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.
INSTALL_DIR
is usually used to specify location where to install something.
Consider renaming to ONNX_ROOT_DIR
(do not use ONNX_DIR
/ ONNX_ROOT
as they are reserved by 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.
Should I change it right now or after having a custom builder working?
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 extra variable into custom builder:
-DONNXRUNTIME_ROOT_DIR=/opt/onnxrt
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.
There is still confusion with ONNX / ONNXRUNTIME naming:
- WITH_ONNX
- HAVE_ONNX
- cmake/FindONNX.cmake (not the same as
find_package(ONNX)
)
set(HAVE_ONNX TRUE) | ||
# For CMake output only | ||
set(ONNX_LIBRARIES "${ORT_LIB}" CACHE STRING "ONNX Runtime libraries") | ||
set(ONNX_INCLUDE_DIR "${ORT_INCLUDE}" CACHE STRING "ONNX Runtime include path") |
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.
-ONNX_INCLUDE_DIR
+ONNX_INCLUDE_DIRS
Also remove CACHE from both vars (not interned to be specified by user due to precondition above).
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.
if I remove those from CACHE, would the "3rd party" thing display them?
if(ORT_INSTALL_DIR) | ||
find_library(ORT_LIB onnxruntime | ||
${ORT_INSTALL_DIR}/lib | ||
CMAKE_FIND_ROOT_PATH_BOTH) |
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.
CMAKE_FIND_ROOT_PATH_BOTH
Do we really need that?
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.
Have no idea, tbh. Probably it works even without it.
cmake/FindONNX.cmake
Outdated
set(ONNX_INCLUDE_DIR "${ORT_INCLUDE}" CACHE STRING "ONNX Runtime include path") | ||
|
||
# Link target with associated interface headers | ||
set(ONNX_LIBRARY "ort" CACHE STRING "ONNX Link 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.
ort
Please don't obfuscate names.
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.
ok
set_target_properties(${ONNX_LIBRARY} PROPERTIES | ||
INTERFACE_INCLUDE_DIRECTORIES ${ORT_INCLUDE} | ||
IMPORTED_LOCATION ${ORT_LIB} | ||
IMPORTED_IMPLIB ${ORT_LIB}) |
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.
after that
set(ONNX_LIBRARIES "${ONNX_LIBRARY}")
should be applied.
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.
Then CMake output will text about our internal name of this library, but won't show its full actual path
ocv_target_compile_definitions(${the_module} PRIVATE HAVE_ONNX=1) | ||
if(TARGET opencv_test_gapi) | ||
ocv_target_compile_definitions(opencv_test_gapi PRIVATE HAVE_ONNX=1) | ||
ocv_target_link_libraries(opencv_test_gapi PRIVATE ${ONNX_LIBRARY}) |
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.
-ONNX_LIBRARY
+ONNX_LIBRARIES
should be used.
cmake/FindONNX.cmake
Outdated
@@ -0,0 +1,31 @@ | |||
ocv_clear_vars(HAVE_ONNX) | |||
|
|||
set(ORT_INSTALL_DIR "" CACHE PATH "ONNX Runtime install directory") |
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 extra variable into custom builder:
-DONNXRUNTIME_ROOT_DIR=/opt/onnxrt
@alalek updated |
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.
CMake part looks good to me 👍
Thanks! |
* G-API: Introduce ONNX backend for Inference - Basic operations are implemented (Infer, -ROI, -List, -List2); - Implemented automatic preprocessing for ONNX models; - Test suite is extended with `OPENCV_GAPI_ONNX_MODEL_PATH` env for test data (test data is an ONNX Model Zoo repo snapshot); - Fixed kernel lookup logic in core G-API: - Lookup NN kernels not in the default package, but in the associated backend's aux package. Now two NN backends can work in the same graph. - Added Infer SSD demo and a combined ONNX/IE demo; * G-API/ONNX: Fix some of CMake issues Co-authored-by: Pashchenkov, Maxim <maxim.pashchenkov@intel.com>
OPENCV_GAPI_ONNX_MODEL_PATH
env for test data(test data is an ONNX Model Zoo repo snapshot);
backend's aux package. Now two NN backends can work in the same graph.
How to build:
Build & install the ONNX RT (currently tested with v1.5.1):
...then specify extra options to OpenCV CMake:
How to test:
Clone ONNX Model Zoo & selected models
When running G-API tests, specify
OPENCV_GAPI_ONNX_MODEL_PATH
environment variable to the model repo root. Expected result: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.