Skip to content

Fix HAL dispatch in cv::minMaxIdx#26946

Merged
asmorkalov merged 6 commits intoopencv:5.xfrom
amane-ame:minmax_fix_hal_rvv
Mar 3, 2025
Merged

Fix HAL dispatch in cv::minMaxIdx#26946
asmorkalov merged 6 commits intoopencv:5.xfrom
amane-ame:minmax_fix_hal_rvv

Conversation

@amane-ame
Copy link
Copy Markdown
Contributor

@amane-ame amane-ame commented Feb 20, 2025 •
edited by asmorkalov
Loading

Closes #26939 (comment).
Closes #26947

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

@amane-ame @hanliutong
Fix up HAL dispatch in minMaxIdx.
65423b3 
Co-authored-by: Liutong HAN <liutong2020@iscas.ac.cn>
@amane-ame
Copy link
Copy Markdown
Contributor Author

amane-ame commented Feb 20, 2025

Because there is no cv_hal_minMaxIdxMaskStep in 5.x at this moment it does not compile. I am not sure which branch should I try to merge into, maybe asmorkalov:5.x-merge-4.x?

@amane-ame
Copy link
Copy Markdown
Contributor Author

amane-ame commented Feb 20, 2025

cc @asmorkalov @mshabunin

asmorkalov
asmorkalov reviewed Feb 20, 2025
View reviewed changes
@amane-ame @hanliutong
Rewrite the dispatch.
574379d 
Co-authored-by: Liutong HAN <liutong2020@iscas.ac.cn>
@amane-ame amane-ame requested a review from asmorkalov February 20, 2025 06:53
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Feb 20, 2025

I propose to re-target the patch to 4.x. It'll appear in 5.x with the regular 4.x-5.x merge process.

@asmorkalov asmorkalov self-assigned this Feb 20, 2025
@mshabunin
Copy link
Copy Markdown
Contributor

mshabunin commented Feb 20, 2025

@asmorkalov , this patch is specific to 5.x (file has been renamed) and should be integrated either in the ongoing merge or right after it.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Feb 20, 2025

Let's do after merge.

@asmorkalov asmorkalov added this to the 5.0-release milestone Feb 20, 2025
@amane-ame @hanliutong
Call ofs2idx to adapt 0D/1D mat for HAL minMaxIdx.
04ac2dc 
Co-authored-by: Liutong HAN <liutong2020@iscas.ac.cn>
@amane-ame amane-ame mentioned this pull request Feb 20, 2025
Closed
4 tasks
@amane-ame amane-ame closed this Feb 20, 2025
@amane-ame amane-ame reopened this Feb 20, 2025
@amane-ame
Copy link
Copy Markdown
Contributor Author

amane-ame commented Feb 20, 2025 •
edited
Loading

UPD: This is unnecessary. I have reverted it.

There is no minmax.hpp in 5.x branch now, I have to create a new one to modify it.
These lines are different from 4.x.
https://github.com/amane-ame/opencv/blob/04ac2dca3561bc022c88687507c99ebe38bb84e2/3rdparty/hal_rvv/hal_rvv_1p0/minmax.hpp#L121-L122
https://github.com/amane-ame/opencv/blob/04ac2dca3561bc022c88687507c99ebe38bb84e2/3rdparty/hal_rvv/hal_rvv_1p0/minmax.hpp#L132-L133
https://github.com/amane-ame/opencv/blob/04ac2dca3561bc022c88687507c99ebe38bb84e2/3rdparty/hal_rvv/hal_rvv_1p0/minmax.hpp#L177-L178
https://github.com/amane-ame/opencv/blob/04ac2dca3561bc022c88687507c99ebe38bb84e2/3rdparty/hal_rvv/hal_rvv_1p0/minmax.hpp#L188-L189
https://github.com/amane-ame/opencv/blob/04ac2dca3561bc022c88687507c99ebe38bb84e2/3rdparty/hal_rvv/hal_rvv_1p0/minmax.hpp#L276-L277
https://github.com/amane-ame/opencv/blob/04ac2dca3561bc022c88687507c99ebe38bb84e2/3rdparty/hal_rvv/hal_rvv_1p0/minmax.hpp#L285-L286

@amane-ame amane-ame closed this Feb 23, 2025
@amane-ame amane-ame reopened this Feb 24, 2025
@amane-ame
Copy link
Copy Markdown
Contributor Author

amane-ame commented Feb 24, 2025

@asmorkalov Merge this to close #26947. The minMaxIdxMaskStep is still disabled in 5.x.

mshabunin
mshabunin reviewed Feb 25, 2025
View reviewed changes
@amane-ame @hanliutong
Revert the unnecessary changes of minmax.hpp.
74cff57 
Co-authored-by: Liutong HAN <liutong2020@iscas.ac.cn>
@amane-ame amane-ame changed the title Fix HAL dispatch in minMaxIdx Fix HAL dispatch in cv::minMaxIdx Feb 26, 2025
asmorkalov
asmorkalov reviewed Feb 26, 2025
View reviewed changes
@amane-ame @hanliutong
A more elegant solution.
f04c862 
Co-authored-by: Liutong HAN <liutong2020@iscas.ac.cn>
mshabunin
mshabunin approved these changes Feb 27, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@mshabunin mshabunin left a comment

Choose a reason for hiding this comment

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

Looks good to me. Now I'm wondering if we need to backport these changes to 4.x 🙂

@asmorkalov asmorkalov merged commit b6b5a10 into opencv:5.x Mar 3, 2025
26 checks passed
asmorkalov added a commit to asmorkalov/opencv that referenced this pull request Mar 3, 2025
@asmorkalov
Backported some CALL_HAL improvements from 5.x opencv#26946
1aa6929
@asmorkalov asmorkalov mentioned this pull request Mar 3, 2025
Merged
6 tasks
asmorkalov added a commit that referenced this pull request Mar 4, 2025
@asmorkalov
Merge pull request #27004 from asmorkalov:as/minMax_backport
5c6c6af 
Backported some CALL_HAL improvements from 5.x #26946
@amane-ame amane-ame deleted the minmax_fix_hal_rvv branch March 14, 2025 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mshabunin mshabunin mshabunin approved these changes

@asmorkalov asmorkalov Awaiting requested review from asmorkalov

Assignees

@asmorkalov asmorkalov

Labels

category: core optimization platform: riscv

Projects

None yet

Milestone

5.0-release

Development

Successfully merging this pull request may close these issues.

3 participants

@amane-ame @asmorkalov @mshabunin