Skip to content
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

Stream default to Stream::Null() when no default in function prototype #20019

Merged
merged 1 commit into from May 1, 2021

Conversation

r2d3
Copy link
Contributor

@r2d3 r2d3 commented May 1, 2021

this corrects bug #16592 where a Stream is created at
each GpuMat::load(arr,stream) call

a correct solution would have been to add a default to GpuMat::load
but due to circular dependence between Stream and GpuMat, this is not possible

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
force_builders=Custom
buildworker:Custom=linux-4,linux-6
build_image:Custom=ubuntu-cuda:18.04

@r2d3 r2d3 changed the title Stream default to Stream::Null() when no default in function prototype WIP: Stream default to Stream::Null() when no default in function prototype May 1, 2021
@r2d3 r2d3 force-pushed the cudaStreamCreate_bug branch 3 times, most recently from 9ea3b9d to 919396e Compare May 1, 2021 02:17
@alalek
Copy link
Member

alalek commented May 1, 2021

Thank you for contribution!

It would be nice if you could add simple test here: https://github.com/opencv/opencv/blob/4.5.2/modules/python/test/test_cuda.py

@r2d3
Copy link
Contributor Author

r2d3 commented May 1, 2021

Hello @alalek

what kind of test do you need ?

  1. that the patch does not break mat_device.upload(mat_host, stream) ?
  2. that the patch correct the issue ? to test that the issue is corrected, we need to use nvprof from the CUDA Toolkit for launching the test.

I have a second question. My fix looks like a hack. Here is the problem creating the bug.
The generated Python code for GpuMat::upload is declaring Stream stream variables to parse the stream parameter. But the default ctor creates a new stream by calling cudaStreamCreate. The fix is to have Stream stream=Stream::Null() in the Python bindings code.

Other functions are using a default value in the C++ function prototype:
CV_EXPORTS_W void resize(InputArray src, OutputArray dst, Size dsize, double fx=0, double fy=0, int interpolation = INTER_LINEAR, Stream& stream = Stream::Null());

But this is not possible for GpuMat::upload because there is a circular dependency between GpuMat and Stream classes.

So patching gen2.py is the only solution I found to generate the correct stream parameter parsing.

@r2d3
Copy link
Contributor Author

r2d3 commented May 1, 2021

jenkins cn please retry a build

Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

There is already .upload check. More accurate test is hard to write.

The patch looks good to me 👍

@alalek
Copy link
Member

alalek commented May 1, 2021

Please remove "WIP: " when patch is ready (BTW, you can use GitHub's "drafts" instead)

this corrects bug opencv#16592 where a Stream is created at
each GpuMat::load(arr,stream) call

a correct solution would have been to add a default to GpuMat::load
but due to circular dependence between Stream and GpuMat, this is not possible
add test_cuda_upload_download_stream to test_cuda.py
@r2d3 r2d3 changed the title WIP: Stream default to Stream::Null() when no default in function prototype Stream default to Stream::Null() when no default in function prototype May 1, 2021
@r2d3
Copy link
Contributor Author

r2d3 commented May 1, 2021

Hello @alalek

I added a test_cuda_upload_download_stream to test_cuda.py as the current upload/download test were not using stream.

Running nvprof python3.6 test_cuda.py allows to see if cudaStreamCreate is called only 2 times.

@opencv-pushbot opencv-pushbot merged commit 7de627c into opencv:master May 1, 2021
@r2d3 r2d3 deleted the cudaStreamCreate_bug branch May 1, 2021 18:51
@alalek alalek mentioned this pull request Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra cudaStreamCreate/cudaStreamDestroy
3 participants