Skip to content

cuda::GpuMat::convertTo - fix for in-place arguments#17982

Merged
opencv-pushbot merged 2 commits intoopencv:3.4from
nglee:dev_cudaGpuMatConvertToInplaceFix
Aug 9, 2020
Merged

cuda::GpuMat::convertTo - fix for in-place arguments#17982
opencv-pushbot merged 2 commits intoopencv:3.4from
nglee:dev_cudaGpuMatConvertToInplaceFix

Conversation

@nglee
Copy link
Copy Markdown
Contributor

@nglee nglee commented Jul 29, 2020 •
edited by alalek
Loading

Resolves #13092

If given in-place arguments to GpuMat::convertTo, *this and dst refers to the same newly allocated GpuMat after these two lines:

_dst.create(size(), rtype);
GpuMat dst = _dst.getGpuMat();

In consequence, *this refers to a different GpuMat from which src refers to.

Before this PR, in the following line:

funcs[sdepth][ddepth](reshape(1), dst.reshape(1), stream);

the first argument derives from *this which refers to the same data as dst.

So it should be fixed as the following line to make the first argument derive from src:

funcs[sdepth][ddepth](src.reshape(1), dst.reshape(1), stream);
                      ^^^^

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

allow_multiple_commits=1

@mshabunin
Copy link
Copy Markdown
Contributor

mshabunin commented Jul 30, 2020

@nglee , could you please add a small test for this scenario or extend one of existing tests?

@asmorkalov asmorkalov added category: gpu/cuda (contrib) OpenCV 4.0+: moved to opencv_contrib pr: needs test New functionality requires minimal tests set labels Jul 30, 2020
@nglee
Copy link
Copy Markdown
Contributor Author

nglee commented Jul 30, 2020 •
edited
Loading

@mshabunin There are some tests included in this PR(11ac26b). Should I add more test scenarios?

@mshabunin mshabunin removed the pr: needs test New functionality requires minimal tests set label Jul 30, 2020
@mshabunin
Copy link
Copy Markdown
Contributor

mshabunin commented Jul 30, 2020

There are some tests included in this PR

Oh, I see now, perhaps I was reviewing only first commit.

asmorkalov
asmorkalov reviewed Jul 31, 2020
View reviewed changes
modules/cudaarithm/test/test_gpumat.cpp
cv::Mat dst_gold;
src.convertTo(dst_gold, depth2, a, b);

EXPECT_MAT_NEAR(dst_gold, d_srcDst, depth2 < CV_32F ? 1.0 : 1e-4);
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov Jul 31, 2020

Choose a reason for hiding this comment

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

Why 1.0 for integer types? Where do you have troubles with it?

Copy link
Copy Markdown
Contributor Author

@nglee nglee Jul 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The values(1.0, 1e-4) are equal to existing non-in-place tests, which a large part of these in-place tests are copied from:

opencv/modules/cudaarithm/test/test_gpumat.cpp

Line 287 in 11ac26b

EXPECT_MAT_NEAR(dst_gold, dst, depth2 < CV_32F ? 1.0 : 1e-4);

Copy link
Copy Markdown
Contributor Author

@nglee nglee Jul 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For these tests that do not include scaling:
CUDA_TEST_P(GpuMat_ConvertTo, WithOutScaling)
CUDA_TEST_P(GpuMat_ConvertTo, InplaceWithOutScaling)
we can set the tolerance to 0.0.

But for tests that include scaling:
CUDA_TEST_P(GpuMat_ConvertTo, WithScaling)
CUDA_TEST_P(GpuMat_ConvertTo, InplaceWithScaling)
some tests fail when tolerance is 0.0, but pass for 1.0.
I think this is acceptable since scaling includes multiplication.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Jul 31, 2020

@alalek Could you look at it? I'm not sure that convertTo is suitable for in-place processing in general case.

@nglee
Copy link
Copy Markdown
Contributor Author

nglee commented Jul 31, 2020 •
edited
Loading

@alalek @asmorkalov Could you review this comment: #17982 (comment)

@nglee
Copy link
Copy Markdown
Contributor Author

nglee commented Aug 4, 2020

To justify why this fix is needed, other codes get the source matrix from src, not *this such as here:

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

Line 626 in 793e7c0

funcs[ddepth](src.reshape(1), dst.reshape(1), stream);

@asmorkalov

I'm not sure that convertTo is suitable for in-place processing in general case.

Even if arguments to converTo are given in an in-place manner if the destination GpuMat type differs from the source GpuMat type, it allocates new GPU memory. So in essence, it is not an in-place conversion internally.

@opencv-pushbot opencv-pushbot merged commit 3f65c12 into opencv:3.4 Aug 9, 2020
This was referenced Aug 14, 2020
Merged
Merged
@nglee nglee deleted the dev_cudaGpuMatConvertToInplaceFix branch August 15, 2020 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov left review comments

@alalek alalek alalek approved these changes

Assignees

@alalek alalek

Labels

category: gpu/cuda (contrib) OpenCV 4.0+: moved to opencv_contrib test

Projects

None yet

Milestone

3.4.12

Development

Successfully merging this pull request may close these issues.

6 participants

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