Skip to content

added in-place support for cartToPolar and polarToCart#24893

Merged
asmorkalov merged 5 commits intoopencv:4.xfrom
chacha21:cart_polar_inplace
Mar 26, 2024
Merged

added in-place support for cartToPolar and polarToCart#24893
asmorkalov merged 5 commits intoopencv:4.xfrom
chacha21:cart_polar_inplace

Conversation

@chacha21
Copy link
Copy Markdown
Contributor

@chacha21 chacha21 commented Jan 20, 2024 •
edited
Loading

  • a fused hal::cartToPolar[32|64]f() is used instead of sequential hal::magnitude[32|64]f/hal::fastAtan[32|64]f
  • ipp_polarToCart is skipped for in-place processing (it seems not to support it correctly)

relates to #24891

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 another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the 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

@chacha21
added in-place support for cartToPolar and polarToCart
64e8c4b 
a fused hal::cartToPolar[32|64]f() is used instead of sequential hal::magnitude[32|64]f/hal::fastAtan[32|64]f
ipp_polarToCart is skipped  when in-place (it seems not to support it correctly)
@opencv-alalek
Copy link
Copy Markdown
Contributor

opencv-alalek commented Jan 21, 2024

This is not so easy, because in-place is expected by be faster than regular src->dst call. At least not slower.

Current gaps are:

  • HAL API added, but there is no implementations.
  • HAL in-place API should not have dedicated buffers for src/dst as they are the same.
  • lack of IPP optimization (performance regression)
  • OpenCL kernel is still wrong due to separate buffers (it may work with some runtimes but there is no guarantee).

BTW, see also #7819 - this is my attempt but it was incomplete too.

@chacha21
Copy link
Copy Markdown
Contributor Author

chacha21 commented Jan 21, 2024 •
edited
Loading 

* HAL in-place API should not have dedicated buffers for src/dst as they are the same.

* lack of IPP optimization (performance regression)

I don't think there is a risk of perf regression because

  • the out-of-place implementation still use the same IPP functions
  • apart from the fused magnitude+fastAtan of cartToPolar, which should be an improvement, the out-of-place implementation is unchanged
  • the in-place HAL implementation for polarToCart has its own code path, and is just resorting to the same kind of small local buffer as the one needed for the 64f case; it should not be a performance killer compared to the need to allocate a full mat to resort to out-of-place
* OpenCL kernel is still wrong due to separate buffers (it may work with some runtimes but there is no guarantee).

Interesting (currently on my host machine the test_arithm does not raise any error), do you think that it is related to the ReadOnly/WriteOnly/ReadWrite specification of kernel arguments that would let the compiler assume some __restrict__ leading to incorrect results for in-place ?
A dedicated in-place polarToCartI and cartToPolarI kernels with inOut buffers would be the solution

@chacha21
proposal to make OpenCL in-place robust
2c76fec 
I lack a test case where in-place would fail due to some run time implementations (as reported by a previosu comment)
This proposal aims at expliciting that some buffers can be in/out, so that no optimization would trick the memory accesses
@chacha21
fix previous commit
f9320d2
@chacha21
better CartPolar tests, fixed original bug
01793f1 
refactored CartPolar to test more cases
this revealed a bug in the original implementation of polarToCart with implicit magnitude in 64F case
@chacha21
typo
2014c4c
@asmorkalov asmorkalov requested review from mshabunin and opencv-alalek and removed request for mshabunin February 12, 2024 13:29
@asmorkalov asmorkalov added this to the 4.10.0 milestone Feb 12, 2024
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Feb 27, 2024

@opencv-alalek @mshabunin @vpisarev could you take a look?

@vpisarev vpisarev self-requested a review March 4, 2024 21:44
@vpisarev
Copy link
Copy Markdown
Contributor

vpisarev commented Mar 4, 2024

@chacha21, we are now working on adding new universal intrinsics that implement math functions: #24941. v_sin, v_cos, v_sincos, v_atan2 are planned to be added. After that this patch could be reworked using simple in-place processing loops. Let's postpone it till after those functions are added.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Mar 7, 2024

@vpisarev @opencv-alalek is it ready for merge?

@opencv-alalek opencv-alalek removed their request for review March 7, 2024 19:53
@opencv-alalek opencv-alalek removed their assignment Mar 7, 2024
@asmorkalov asmorkalov merged commit 1a537ab into opencv:4.x Mar 26, 2024
@asmorkalov asmorkalov mentioned this pull request Apr 1, 2024
Merged
klatism pushed a commit to klatism/opencv that referenced this pull request May 17, 2024
@chacha21 @klatism
Merge pull request opencv#24893 from chacha21:cart_polar_inplace
d0458ab 
Added in-place support for cartToPolar and polarToCart opencv#24893

- a fused hal::cartToPolar[32|64]f() is used instead of sequential hal::magnitude[32|64]f/hal::fastAtan[32|64]f
- ipp_polarToCart is skipped for in-place processing (it seems not to support it correctly)

relates to opencv#24891
### Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [X] I agree to contribute to the project under Apache 2 License.
- [X] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [X] The PR is proposed to the proper branch
- [X] There is a reference to the original bug report and related work
- [X] 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
savuor pushed a commit to savuor/opencv that referenced this pull request Nov 8, 2024
@chacha21
Merge pull request opencv#24893 from chacha21:cart_polar_inplace
bd2a335 
Added in-place support for cartToPolar and polarToCart opencv#24893

- a fused hal::cartToPolar[32|64]f() is used instead of sequential hal::magnitude[32|64]f/hal::fastAtan[32|64]f
- ipp_polarToCart is skipped for in-place processing (it seems not to support it correctly)

relates to opencv#24891
### Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [X] I agree to contribute to the project under Apache 2 License.
- [X] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [X] The PR is proposed to the proper branch
- [X] There is a reference to the original bug report and related work
- [X] 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
savuor pushed a commit to savuor/opencv that referenced this pull request Nov 21, 2024
@chacha21
Merge pull request opencv#24893 from chacha21:cart_polar_inplace
474e48e 
Added in-place support for cartToPolar and polarToCart opencv#24893

- a fused hal::cartToPolar[32|64]f() is used instead of sequential hal::magnitude[32|64]f/hal::fastAtan[32|64]f
- ipp_polarToCart is skipped for in-place processing (it seems not to support it correctly)

relates to opencv#24891
### Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [X] I agree to contribute to the project under Apache 2 License.
- [X] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [X] The PR is proposed to the proper branch
- [X] There is a reference to the original bug report and related work
- [X] 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vpisarev vpisarev vpisarev approved these changes

Assignees

@vpisarev vpisarev

Labels

category: core optimization

Projects

None yet

Milestone

4.10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@chacha21 @opencv-alalek @asmorkalov @vpisarev