added new function, cv::ocl::attachContext #4072
Conversation
…* platformID, void* context, void* deviceID) which allow to attach externally created OpenCL context to OpenCV.
|
@alalek @vpisarev @SergeySivolgin please review new function, cv::ocl::attachContext, which allows to attach externally created OpenCL context to OpenCV |
modules/core/src/ocl.cpp
Outdated
There was a problem hiding this comment.
If there is something wrong with OpenCL runtimes (you assume it and try to check this by "platformName"), then you may receive exception/crash during execution of this function, because pointer d is alien for OpenCV OpenCL runtime.
CV_OclDbgAssert is misused here. Errors should be raised anyway.
You didn't initialize pl variable, so if you bypass this error then value of this variable is undefined for code below.
I still believe that initializeContextFromHandle is enough. If you want to guard from mismatched OpenCL dlls you may try to use GetModuleHangle("OpenCL.dll) near LoadLibrary to bypass it (and you can use library same as user - in case of lazy OpenCL initialization).
…type, void* cl_mem_obj, UMat& dst, UMatUsageFlags usageFlags = cv::USAGE_DEFAULT) which attaches user allocated OpenCL clBuffer to UMat
There was a problem hiding this comment.
rows & cols can be retrieved using clGetImageInfo() and should better be retrieved this way
There was a problem hiding this comment.
this function convert clBuffer not a clImage
There was a problem hiding this comment.
@alalek Alexander, please take a look on new func convertFromBuffer. This function converts clBuffer allocated at user OpenCL code to UMat.
…nvertFromBuffer func)
…DK not found in system
…tainMemObject for convertFromBuffer func
modules/core/src/ocl.cpp
Outdated
There was a problem hiding this comment.
Code duplication is bad idea. Try to move all pieces (similar code for Mat) into header file
…ncv). need to try standalone sample build
commented out cmake_minimum_required(VERSION 3.1)
|
@alalek @vpisarev @SergeySivolgin added convertFromImage func and updated sample |
|
@vladimir-dudnik Could you add specification to API with requirements for input objects lifetime and input objects content modification? (Is there copy / move / map logic? sync / async "copy" processing?) P.S. Please use doxygen supported comments. Example: |
There was a problem hiding this comment.
cl_mem_obj --> cl_mem_buffer in convertFromBuffer
cl_mem_obj --> cl_mem_image in convertFromImage
?
There was a problem hiding this comment.
added doxygen style comments
modules/core/src/umatrix.hpp
Outdated
There was a problem hiding this comment.
we don't need a separate header file. Use forward declaration:
// a.cpp
void func()
{ /* impl */}
and it can be used in:
// b.cpp
// forward declaration
void func();
static void func2()
{
func(); // usage
}
There was a problem hiding this comment.
ok, done with fwd decls
There was a problem hiding this comment.
It is dangerous, because you will not receive any compiler warning if you change implementation
It is better to add this include file before implementation into umatrix.cpp to track contract and implementation.
There was a problem hiding this comment.
not clear what do you mean? The linker will not resolve this ref if API changed.
There was a problem hiding this comment.
Right, C++ name mangling save us in this case (but it will not help with extern "C")
Anyway, It is unclear and unnecessary code duplication.
There was a problem hiding this comment.
TMBK declaration of functions in corresponding header file is better in case they are reffered from more than one compile unit. But different design policy might be used, do not know if "we do not need separate header file" stands for OpenCV design policy.
|
Great job! |
|
I think we can merge this PR. |
|
Are there any way to squash these 30+ commits? If no, I can do it manually before merge. |
|
Merged as commit: |
…trix calculation error
Commits: added new function, cv::ocl::attachContext(String& platformName, void* platformID, void* context, void* deviceID) which allow to attach externally created OpenCL context to OpenCV. add definitions of clRetainDevice, clRetainContext funcs removed definitions for clRetainContext, clRetainDevice fixed build issue under Linux fixed uninitialized vars, replace dbgassert in error handling remove function which is not ready yet add new function, cv::ocl::convertFromBuffer(int rows, int cols, int type, void* cl_mem_obj, UMat& dst, UMatUsageFlags usageFlags = cv::USAGE_DEFAULT) which attaches user allocated OpenCL clBuffer to UMat uncommented clGetMemObjectInfo definition (otherwise prevent opencv build) fixed build issue on linux and android add step parameter to cv::ocl::convertFromBuffer func suppress compile-time warning added sample opencl-opencv interoperability (showcase for cv::ocl::convertFromBuffer func) CMakeLists.txt modified to not create sample build script if OpenCL SDK not found in system fixed build issue (apple opencl include dir and spaces in CMake file) added call to clRetainContext for attachContext func and call to clRetainMemObject for convertFromBuffer func uncommented clRetainMemObject definition added comments and cleanup add local path to cmake modules search dirs (instead of replacing) remove REQUIRED for find_package call (sample build together with opencv). need to try standalone sample build opencl-interop sample moved to standalone build set minimum version requirement for sample's cmake to 3.1 put cmake_minimum_required under condition, so do not check if samples not builded remove code dups for setSize, updateContinuityFlag, and finalizeHdr commented out cmake_minimum_required(VERSION 3.1) add safety check for cmake version add convertFromImage func and update opencl-interop sample uncommented clGetImageInfo defs uncommented clEnqueueCopyImageToBuffer defs fixed clEnqueueCopyImageToBuffer defs add doxygen comments remove doxygen @fn tag try to restart buildbot add doxygen comments to directx interop funcs remove internal header, use fwd declarations in affected compile units instead
Commits: added new function, cv::ocl::attachContext(String& platformName, void* platformID, void* context, void* deviceID) which allow to attach externally created OpenCL context to OpenCV. add definitions of clRetainDevice, clRetainContext funcs removed definitions for clRetainContext, clRetainDevice fixed build issue under Linux fixed uninitialized vars, replace dbgassert in error handling remove function which is not ready yet add new function, cv::ocl::convertFromBuffer(int rows, int cols, int type, void* cl_mem_obj, UMat& dst, UMatUsageFlags usageFlags = cv::USAGE_DEFAULT) which attaches user allocated OpenCL clBuffer to UMat uncommented clGetMemObjectInfo definition (otherwise prevent opencv build) fixed build issue on linux and android add step parameter to cv::ocl::convertFromBuffer func suppress compile-time warning added sample opencl-opencv interoperability (showcase for cv::ocl::convertFromBuffer func) CMakeLists.txt modified to not create sample build script if OpenCL SDK not found in system fixed build issue (apple opencl include dir and spaces in CMake file) added call to clRetainContext for attachContext func and call to clRetainMemObject for convertFromBuffer func uncommented clRetainMemObject definition added comments and cleanup add local path to cmake modules search dirs (instead of replacing) remove REQUIRED for find_package call (sample build together with opencv). need to try standalone sample build opencl-interop sample moved to standalone build set minimum version requirement for sample's cmake to 3.1 put cmake_minimum_required under condition, so do not check if samples not builded remove code dups for setSize, updateContinuityFlag, and finalizeHdr commented out cmake_minimum_required(VERSION 3.1) add safety check for cmake version add convertFromImage func and update opencl-interop sample uncommented clGetImageInfo defs uncommented clEnqueueCopyImageToBuffer defs fixed clEnqueueCopyImageToBuffer defs add doxygen comments remove doxygen @fn tag try to restart buildbot add doxygen comments to directx interop funcs remove internal header, use fwd declarations in affected compile units instead
new function,
cv::ocl::attachContext(String& platformName, void* platformID, void* context, void* deviceID)
which allow to attach externally created OpenCL context to OpenCV.