Skip to content

core:add OPENCV_IPP_MEAN/MINMAX/SUM option to enable IPP optimizations#24179

Merged
opencv-alalek merged 5 commits intoopencv:4.xfrom
Kumataro:fix24145
Aug 23, 2023
Merged

core:add OPENCV_IPP_MEAN/MINMAX/SUM option to enable IPP optimizations#24179
opencv-alalek merged 5 commits intoopencv:4.xfrom
Kumataro:fix24145

Conversation

@Kumataro
Copy link
Copy Markdown
Contributor

@Kumataro Kumataro commented Aug 19, 2023

fix #24145
fix #24146
relates #13085

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

Copy link
Copy Markdown
Contributor

@opencv-alalek opencv-alalek left a comment

Choose a reason for hiding this comment

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

Thank you for contribution!

BTW, It make sense to think about something like OPENCV_IPP_ENABLE_ALL in the future.

endif()

# https://github.com/opencv/opencv/issues/24145
ocv_check_environment_variables(OPENCV_IPP_MEAN)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lets guard this block by if(HAVE_IPP)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree with you, thank you!
And I tried with raspi(arm32, not intel), OPENCV_IPP_GAUSSIAN_BLUR is shown in cmake gui. So I added fot it also.

ocv_check_environment_variables(OPENCV_IPP_MEAN)
option(OPENCV_IPP_MEAN "Enable IPP optimizations for mean (+200Kb in binary size)" OFF)
if(OPENCV_IPP_MEAN)
add_definitions(-DOPENCV_IPP_MEAN=1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is better to apply definition to the specific file:

ocv_append_source_file_compile_definitions(${CMAKE_CURRENT_SOURCE_DIR}/src/mean.dispatch.cpp "OPENCV_IPP_MEAN=1")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for yor review, this is more better fix. I fix with it.

@Kumataro
Copy link
Copy Markdown
Contributor Author

Hello, thank you for your review.

BTW, It make sense to think about something like OPENCV_IPP_ENABLE_ALL in the future.

In cmake, it seems to be a complicated to link multiple options.

To simplify selecting to optimization with ipp, there are 3 ideas.
In Now Idea A is selected. Is Idea B or C better than A ?

  • Idea A) Disable OPENCV_IPP_MEAN/MINMAX/SUM/GAUSSIAN_BLUR (+0MiB). People who want to improve performance can enable it in the user option.
  • Idea B) Enable OPENCV_IPP_MEAN/MINMAX/SUM and disable OPENCV_IPP_GAUSSIAN_BLUR (+1MiB).. This method may be a good balance between size and performance.
  • Idea C) Enable OPENCV_IPP_MEAN/MINMAX/SUM/GAUSSIAN_BLUR (+9MiB). People who want to make the library size smaller can remove it in the user option.

In particular, in an environment where IPP can be used, even if the library size increases by several MiB, there may be almost no effect on operation. In this case, Idea C is better than other idea.

@opencv-alalek
Copy link
Copy Markdown
Contributor

I mean something like this: https://github.com/opencv/opencv/blob/4.8.0/CMakeLists.txt#L218-L225

Idea is using OPENCV_IPP_ENABLE_ALL as initial value for these options. User still may override values of options one-by-one.

Copy link
Copy Markdown
Contributor

@opencv-alalek opencv-alalek left a comment

Choose a reason for hiding this comment

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

Thank you 👍

@opencv-alalek opencv-alalek added this to the 4.9.0 milestone Aug 21, 2023
@Kumataro
Copy link
Copy Markdown
Contributor Author

I was over-considered the problem.It is clearly,

Thank you very much! I'll try it tomorrow.


# https://github.com/opencv/opencv/issues/24145
if(HAVE_IPP)
OCV_OPTION(OPENCV_IPP_ENABLE_ALL "Force to enable all ipp optimization" OFF)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ipp => IPP


all => available ?

some implementations are still disabled due to accuracy or performance issues

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment!

I would like to make fix based on the Description of OPENCV_FORCE_3RDPARTY_BUILD.

OCV_OPTION(OPENCV_IPP_ENABLE_ALL "Enable all OPENCV_IPP_ options at once" OFF)

https://docs.opencv.org/4.x/db/d05/tutorial_config_reference.html

@opencv-alalek opencv-alalek self-assigned this Aug 23, 2023
@opencv-alalek opencv-alalek merged commit 81cc89a into opencv:4.x Aug 23, 2023
@asmorkalov asmorkalov mentioned this pull request Sep 11, 2023
thewoz pushed a commit to thewoz/opencv that referenced this pull request Jan 4, 2024
* core:add OPENCV_IPP_MEAN/MINMAX/SUM option to enable IPP optimizations

* fix: to use guard HAVE_IPP and ocv_append_source_file_compile_definitions() macro.

* support OPENCV_IPP_ENABLE_ALL

* add document for OPENCV_IPP_ENABLE_ALL

* fix OPENCV_IPP_ENABLE_ALL comment
thewoz pushed a commit to thewoz/opencv that referenced this pull request May 29, 2024
* core:add OPENCV_IPP_MEAN/MINMAX/SUM option to enable IPP optimizations

* fix: to use guard HAVE_IPP and ocv_append_source_file_compile_definitions() macro.

* support OPENCV_IPP_ENABLE_ALL

* add document for OPENCV_IPP_ENABLE_ALL

* fix OPENCV_IPP_ENABLE_ALL comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

why minMaxIdx Restored reduced IPP calls how to use ipp_minMaxIdx?

2 participants