added in-place support for cartToPolar and polarToCart #24893

chacha21 · 2024-01-20T12:22:01Z

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

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 · 2024-01-21T07:11:20Z

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 · 2024-01-21T09:03:18Z

* 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~~

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

refactored CartPolar to test more cases this revealed a bug in the original implementation of polarToCart with implicit magnitude in 64F case

asmorkalov · 2024-02-27T13:24:50Z

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

vpisarev · 2024-03-04T21:47:12Z

@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 · 2024-03-07T08:40:23Z

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

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

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 added the incomplete label Jan 21, 2024

chacha21 added 2 commits January 21, 2024 13:16

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

fix previous commit

f9320d2

asmorkalov added the optimization label Jan 22, 2024

chacha21 added 2 commits January 22, 2024 09:26

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

typo

2014c4c

asmorkalov requested review from mshabunin and opencv-alalek and removed request for mshabunin February 12, 2024 13:29

asmorkalov assigned opencv-alalek Feb 12, 2024

asmorkalov added this to the 4.10.0 milestone Feb 12, 2024

vpisarev self-requested a review March 4, 2024 21:44

vpisarev approved these changes Mar 7, 2024

View reviewed changes

opencv-alalek added category: core and removed incomplete labels Mar 7, 2024

opencv-alalek removed their request for review March 7, 2024 19:53

opencv-alalek removed their assignment Mar 7, 2024

asmorkalov assigned vpisarev Mar 26, 2024

asmorkalov merged commit 1a537ab into opencv:4.x Mar 26, 2024

asmorkalov mentioned this pull request Apr 1, 2024

5.x merge 4.x #25305

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

added in-place support for cartToPolar and polarToCart #24893

added in-place support for cartToPolar and polarToCart #24893

Uh oh!

chacha21 commented Jan 20, 2024 •

edited

Loading

Uh oh!

opencv-alalek commented Jan 21, 2024

Uh oh!

chacha21 commented Jan 21, 2024 •

edited

Loading

Uh oh!

asmorkalov commented Feb 27, 2024

Uh oh!

vpisarev commented Mar 4, 2024

Uh oh!

asmorkalov commented Mar 7, 2024

Uh oh!

Uh oh!

Uh oh!

added in-place support for cartToPolar and polarToCart #24893

added in-place support for cartToPolar and polarToCart #24893

Uh oh!

Conversation

chacha21 commented Jan 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

opencv-alalek commented Jan 21, 2024

Uh oh!

chacha21 commented Jan 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asmorkalov commented Feb 27, 2024

Uh oh!

vpisarev commented Mar 4, 2024

Uh oh!

asmorkalov commented Mar 7, 2024

Uh oh!

Uh oh!

chacha21 commented Jan 20, 2024 •

edited

Loading

chacha21 commented Jan 21, 2024 •

edited

Loading