Skip to content

cuda::flip - Use in-place npp function for inplace arguments#17863

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
nglee:dev_cudaDetachOutput
Jul 22, 2020
Merged

cuda::flip - Use in-place npp function for inplace arguments#17863
opencv-pushbot merged 1 commit intoopencv:3.4from
nglee:dev_cudaDetachOutput

Conversation

@nglee
Copy link
Copy Markdown
Contributor

@nglee nglee commented Jul 16, 2020 •
edited
Loading

Resolves #17840

#17863 (comment)

cv::cuda::GpuMat::create does not allocate a new GpuMat if required size and type equals existing size and type.

opencv/modules/core/src/cuda/gpu_mat.cu

Lines 160 to 161 in bf8136e

if (rows == _rows && cols == _cols && type() == _type && data)
return;

This PR implements cv::cuda::detachOutput to allocate a new GpuMat in such cases. Unfortunately, _dst is const so assignment is not possible. Instead, this implementation clones src.

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 OpenCV (BSD) 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

@nglee nglee force-pushed the dev_cudaDetachOutput branch from 3900414 to bfefe79 Compare July 16, 2020 17:42
@nglee nglee marked this pull request as ready for review July 16, 2020 17:57
alalek
alalek reviewed Jul 17, 2020
View reviewed changes
Copy link
Copy Markdown
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.

It is not a CUDA-only issue for this particular function.

It is related to many CPU and OpenCL implementations (almost zero tests for this mode).
So some robust generic approach should be applied.

Possible fixes degrade performance, so we want to forbid such usages (to keep OpenCV library and applications optimized).
We don't want to trying fixing that and adding corresponding tests.

see also #13570 (it is closed without raising a general issue)

@nglee nglee force-pushed the dev_cudaDetachOutput branch from bfefe79 to bb34398 Compare July 18, 2020 14:12
@nglee
Copy link
Copy Markdown
Contributor Author

nglee commented Jul 18, 2020

@alalek
The updated code referred to the following lines.
https://github.com/opencv/opencv/blob/3.4/modules/imgproc/src/filter.dispatch.cpp#L918-L924

Consulting this issue comment, should we raise an exception instead?

@nglee nglee force-pushed the dev_cudaDetachOutput branch from bb34398 to 58dcd52 Compare July 18, 2020 21:10
@asmorkalov asmorkalov added category: gpu/cuda (contrib) OpenCV 4.0+: moved to opencv_contrib and removed RFC labels Jul 20, 2020
@nglee nglee force-pushed the dev_cudaDetachOutput branch from 58dcd52 to 9411cd6 Compare July 21, 2020 01:28
@nglee
Copy link
Copy Markdown
Contributor Author

nglee commented Jul 21, 2020

#17840 (comment)

Instead of copying the source matrix, this code calls in-place npp function if given in-place arguments.

@nglee nglee changed the title Implement cv::cuda::detachOutput cuda::flip - Use in-place npp function for inplace arguments Jul 21, 2020
@nglee nglee marked this pull request as draft July 21, 2020 02:37
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Jul 21, 2020

Looks good to me. Passed test on Ubuntu 18.04 with CUDA 10.2 and GPU 1080Ti.

@nglee nglee marked this pull request as ready for review July 21, 2020 12:30
@mshabunin mshabunin requested a review from asmorkalov July 21, 2020 16:41
asmorkalov
asmorkalov approved these changes Jul 22, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

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

👍

@asmorkalov asmorkalov added the backport is needed Label for maintainers. Authors of PR can ignore this label Jul 22, 2020
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Jul 22, 2020

@nglee The same issue is reproducible with master branch too, but the code for master is moved to opencv_contrib repo. Could you propose yet another PR to contrib repository too?

@nglee nglee mentioned this pull request Jul 22, 2020
Merged
@nglee
Copy link
Copy Markdown
Contributor Author

nglee commented Jul 22, 2020

@asmorkalov
I've opened one: opencv/opencv_contrib#2612

@opencv-pushbot opencv-pushbot merged commit 0fa06b1 into opencv:3.4 Jul 22, 2020
@nglee nglee deleted the dev_cudaDetachOutput branch July 22, 2020 10:38
@mshabunin mshabunin added port/backport done Label for maintainers. Authors of PR can ignore this and removed backport is needed Label for maintainers. Authors of PR can ignore this labels Jul 22, 2020
@asmorkalov asmorkalov mentioned this pull request Jul 23, 2020
Closed
4 tasks
@tomoaki0705
Copy link
Copy Markdown
Contributor

tomoaki0705 commented Jul 28, 2020

nglee The same issue is reproducible with master branch too, but the code for master is moved to opencv_contrib repo. Could you propose yet another PR to contrib repository too?

@asmorkalov Once in the past, I was pointed from @alalek that this is not necessary, porting one PR from opencv to opencv_contrib.

#12666 (review)

once this modification is approved, I'll open a PR for opencv_contrib, too.

It is not necessary. I will merge these changes during next regular "Merge 3.4" PRs.

And alalek also did the same recently
original
ported

Was there any special reason for this PR to be ported to opencv_contrib ?
I'm just curious.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Jul 28, 2020

It's good idea to have CUDA implementations aligned between branches, especially if the patch could be ported as is without serious modifications.

@alalek alalek mentioned this pull request Jul 28, 2020
Merged
@tomoaki0705
Copy link
Copy Markdown
Contributor

tomoaki0705 commented Jul 29, 2020

@asmorkalov , my point is not "why", but "who".
If I make modification for CUDA stuff in 3.4 branch, should "I" port it to contrib, or can I keep it untouched so "alalek" can port it ?

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Jul 29, 2020

General approach is to merge 3.4 to master weekly or so. Alalek does it and you usually hould not worry about it. Cuda changes cannot be just merged to contrib with git and some manual work is required. I usually ask the PR author to do porting as soon as author is in context can can do it faster.

@tomoaki0705 tomoaki0705 mentioned this pull request Sep 16, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek left review comments

@asmorkalov asmorkalov asmorkalov approved these changes

Assignees

@asmorkalov asmorkalov

Labels

category: gpu/cuda (contrib) OpenCV 4.0+: moved to opencv_contrib port/backport done Label for maintainers. Authors of PR can ignore this

Projects

None yet

Milestone

3.4.12

Development

Successfully merging this pull request may close these issues.

7 participants

@nglee @asmorkalov @tomoaki0705 @alalek @opencv-pushbot @mshabunin @cognex-nalee