add cuda::Stream constructor with cuda stream flags #19286
Conversation
|
updated PR to remove python wrap for the new constructor. Python and move semantics are not supported. And not workable for python to have a true cudaStream_t from OpenCV or numby. Might be possible via CuPy, but that is out of scope. |
diablodale
changed the title
|
I see two sets of warnings/errors in the CI builds. I request assist as I do not understand them warnings caused by the Py wrapper infrastructurehttps://pullrequest.opencv.org/buildbot/builders/precommit_opencl_linux/builds/27820/steps/compile%20release/logs/warnings%20%282%29. The parameter on the C++ constructor is intentionally required and should not have a default value. This is the generated code. {
PyObject* pyobj_cudaFlags = NULL;
unsigned cudaFlags;
const char* keywords[] = { "cudaFlags", NULL };
if( PyArg_ParseTupleAndKeywords(py_args, kw, "O:Stream", (char**)keywords, &pyobj_cudaFlags) &&
pyopencv_to(pyobj_cudaFlags, cudaFlags, ArgInfo("cudaFlags", 0)) )
{
new (&(self->v)) Ptr<cv::cuda::Stream>(); // init Ptr with placement new
if(self) ERRWRAP2(self->v.reset(new cv::cuda::Stream(cudaFlags)));
return 0;
}
}fail in the "custom" CI platformI see errors in some .cu file that is likely generated by something that is part of a maybe a contrib module. I am unable to discern the process and/or initial source code to do any diagnostics. |
try
Problem is from here: opencv/opencv_contrib#2807 (comment) |
|
The cuda api parameter signature is I use
This is part of the OpenCV CI. I have no control of that system. Do you want to fix that or turn off the failing platform? |
|
This is not about Python only. Bindings wrappers support only subset of types.
This doesn't matter until you have such platform in your hands (with CUDA). |
|
I will experiment today, but no guarantees. The most important user to me is C++. And I will not compromise c++ because of python itself or the python infrastructure. The C++ constructor will have I will include a disclaimer in the code/docs that the sign is unreliable and may cause problems now or in the future. |
|
I do not see anything that I would change in the constructor. The two warnings I reported above only appear in 3 platforms. The two test cases I wrote pass and my app itself works well. The python wrapping infrastructure already has some code to support opencv/modules/python/src2/cv2.cpp Lines 840 to 852 in ea41f89 opencv/modules/python/src2/cv2.cpp Lines 741 to 754 in ea41f89 I also found at least one function in opencv that uses the size_t support and I don't see warnings about it in the logs. |
| cv::cuda::Stream cvStream(cudaStreamNonBlocking); | ||
| @endcode | ||
| */ | ||
| CV_WRAP Stream(const unsigned cudaFlags); |
There was a problem hiding this comment.
Remove CV_WRAP from here to keep this C++ only.
We can add CV_WRAP static Stream create(int cudaFlags) or similar for bindings later.
There was a problem hiding this comment.
I thought of an alternative. I will need to code it and force push to see if it removes the two warnings in the CI platforms. (Functionality has not been a problem...only two warnings on a subset of platforms).
I can code the constructor to accept an int (you suggest this is preferred in OpenCV) and also reject negative numbers. By accepting only positive int values...I can safely cast it to an unsigned int. This alternative supports all the values that the NVidia API currently supports. If in the future NVidia defines a value that is larger than a signed int can represent and someone uses that value, then this constructor will fail. And OpenCV will need to make a choice how to handle NVidia's new value.
I would prefer to not have the cost of a branch/if in the constructor. What is the OpenCV philosophy to use CV_DbgAssert(param>=0). I would prefer that so there is no cost in release builds.
If the core team doesn't like that, then I can use CV_Assert(param>=0).
modules/core/test/test_mat.cpp
Outdated
| @@ -9,6 +9,10 @@ | |||
| #include "opencv2/core/eigen.hpp" | |||
| #endif | |||
|
|
|||
| #if defined(HAVE_CUDA) | |||
| #include <cuda_runtime.h> | |||
| #endif | |||
There was a problem hiding this comment.
It make sense to create new file test_cuda.cpp for this test
There was a problem hiding this comment.
ok. I will move it into a new file.
modules/core/test/test_mat.cpp
Outdated
| @@ -2355,5 +2359,15 @@ TEST(Mat, regression_18473) | |||
| EXPECT_EQ((int)5, (int)m.at<short>(19, 49, 99)); | |||
| } | |||
|
|
|||
| TEST(CUDA_Stream, construct_cudaFlags) | |||
| { | |||
| #ifdef HAVE_CUDA | |||
There was a problem hiding this comment.
Guard the whole test (or the file as mentioned above) with #ifdef HAVE_CUDA.
IMHO, "#else" branch is not really needed (it is hard enough to introduce such regression).
There was a problem hiding this comment.
Ok. I'll incorporate with the coming PR update.
There was a problem hiding this comment.
What is wrong with cv::cuda::StreamAccessor?
It was specially added to work with CUDA SDK directly (see all CUDA enums / types, respect type safety).
Stream API has limitation, which doesn't allow to use 3rdparty dependencies (CUDA SDK).
StreamAccessor is not a part of bindings.
modules/python/test/test_cuda.py
Outdated
| @@ -33,6 +33,8 @@ def test_cuda_interop(self): | |||
| self.assertTrue(cuMat.cudaPtr() != 0) | |||
| stream = cv.cuda_Stream() | |||
| self.assertTrue(stream.cudaPtr() != 0) | |||
| asyncstream = cv.cuda_Stream(1) | |||
There was a problem hiding this comment.
Consider adding a comment with description for magic numbers:
asyncstream = cv.cuda_Stream(1) # cudaStreamNonBlocking
There was a problem hiding this comment.
The CUDA api defines are documented in the doxygen for the constructor. I can also add it to this test file, no problem.
|
Did you write again "What is wrong with cv::cuda::StreamAccessor?"? |
| cv::cuda::Stream cvStream(cudaStreamNonBlocking); | ||
| @endcode | ||
| */ | ||
| CV_WRAP Stream(const int cudaFlags); |
There was a problem hiding this comment.
I took a look a bit more at bindings generators.
There is size_t type support in bindings.
There was a problem hiding this comment.
I am pushing again to check for any warnings on the CI platforms. Using size_t in this way is not in alignment with intention of the type https://en.cppreference.com/w/cpp/types/size_t
It may work on all common platforms/data models today. And it may fail in the future due to using size_t in a way the C++ standard did not intend.
adds feature described in issue #19285
includes code, test, and doc.
TLDR: adds a constructor for cuda::Stream which accepts a raw cudaStream_t and assumes ownership of it including destroying the raw CUDA stream in the Stream's destructor.
Pull Request Readiness Checklist
Patch to opencv_extra has the same branch name.