Fix HAL interface for hal_ni_minMaxIdx by asmorkalov · Pull Request #25553 · opencv/opencv

asmorkalov · 2024-05-07T07:16:35Z

The original implementation call HAL with the same parameters independently from amount of channels. The patch uses HAL correctly for the case cn > 1.

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

mplatings · 2024-05-07T08:23:46Z

This would also require a change at https://github.com/opencv/opencv/blob/4.x/modules/core/src/minmax.cpp#L1514 to pass type instead of depth

… cases.

modules/core/src/minmax.cpp

mplatings · 2024-05-08T08:25:33Z

modules/core/src/minmax.cpp

+    int _minIdx, _maxIdx;
+    int* min_offset = (cn == 1) ? minIdx : &_minIdx;
+    int* max_offset = (cn == 1) ? maxIdx : &_maxIdx;


This seems like a bug because as documented "When minIdx is not NULL, it must have at least 2 elements"

Passing a non-null pointer means the HAL implementation can't optimise for minIdx and maxIdx being null.

I suggest instead passing minIdx and maxIdx directly to cv_hal_minMaxIdx at line 1519. We already know from the assert above that they must be null if cn>1.

mplatings · 2024-05-08T08:26:08Z

modules/core/src/minmax.cpp

+    else if (src.isContinuous())
+    {
+        CALL_HAL(minMaxIdx, cv_hal_minMaxIdx, src.data, 0, (int)src.total()*cn, 1, src.depth(),
+                 minVal, maxVal, min_offset, max_offset, mask.data);
+    }


In this branch either minIdx and maxIdx need recalculating according to the number of dimensions, or this branch should be disabled if either pointer is non-null.

Transform offset to indeces for MatND in minMaxIdx HAL #25563 Address comments in #25553 ### 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 - [ ] 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

Fix HAL interface for hal_ni_minMaxIdx opencv#25553 Fixes opencv#25540 The original implementation call HAL with the same parameters independently from amount of channels. The patch uses HAL correctly for the case cn > 1. ### 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 - [ ] 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

Transform offset to indeces for MatND in minMaxIdx HAL opencv#25563 Address comments in opencv#25553 ### 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 - [ ] 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

Fix HAL interface for hal_ni_minMaxIdx opencv#25553 Fixes opencv#25540 The original implementation call HAL with the same parameters independently from amount of channels. The patch uses HAL correctly for the case cn > 1. 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 - [ ] 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

Transform offset to indeces for MatND in minMaxIdx HAL opencv#25563 Address comments in opencv#25553 ### 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 - [ ] 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

Fix HAL interface for hal_ni_minMaxIdx opencv#25553 Fixes opencv#25540 The original implementation call HAL with the same parameters independently from amount of channels. The patch uses HAL correctly for the case cn > 1. ### 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 - [ ] 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

Transform offset to indeces for MatND in minMaxIdx HAL opencv#25563 Address comments in opencv#25553 ### 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 - [ ] 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

Fix HAL interface for hal_ni_minMaxIdx opencv#25553 Fixes opencv#25540 The original implementation call HAL with the same parameters independently from amount of channels. The patch uses HAL correctly for the case cn > 1. ### 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 - [ ] 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

Transform offset to indeces for MatND in minMaxIdx HAL opencv#25563 Address comments in opencv#25553 ### 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 - [ ] 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

asmorkalov added bug category: core labels May 7, 2024

asmorkalov added this to the 4.10.0 milestone May 7, 2024

asmorkalov requested review from opencv-alalek and vpisarev May 7, 2024 07:16

Fix HAL interface for hal_ni_minMaxIdx.

d3a2d7f

asmorkalov force-pushed the as/HAL_min_max_idx branch from 557ab37 to d3a2d7f Compare May 7, 2024 09:51

asmorkalov closed this May 7, 2024

asmorkalov reopened this May 7, 2024

Re-implement minMaxIdx to use existing HAL API for both cn=1 and cn>1…

9fef968

… cases.

asmorkalov force-pushed the as/HAL_min_max_idx branch from 53d60a4 to 9fef968 Compare May 7, 2024 11:19

asmorkalov assigned vpisarev May 7, 2024

Support multi-channel continuous matrices in minMaxIdx.

5d3eb5d

vpisarev reviewed May 7, 2024

View reviewed changes

modules/core/src/minmax.cpp Outdated Show resolved Hide resolved

vpisarev reviewed May 7, 2024

View reviewed changes

modules/core/src/minmax.cpp Outdated Show resolved Hide resolved

Code review fixes

f49aab6

vpisarev self-requested a review May 7, 2024 16:03

vpisarev approved these changes May 7, 2024

View reviewed changes

Code review fixes.

240b1d4

asmorkalov force-pushed the as/HAL_min_max_idx branch from 1e6d8e4 to 240b1d4 Compare May 7, 2024 16:36

asmorkalov changed the title ~~Fix HAL interface for hal_ni_minMaxIdx.~~ Fix HAL interface for hal_ni_minMaxIdx May 7, 2024

asmorkalov merged commit faa259a into opencv:4.x May 7, 2024

mplatings reviewed May 8, 2024

View reviewed changes

asmorkalov mentioned this pull request May 8, 2024

Transform offset to indeces for MatND in minMaxIdx HAL #25563

Merged

6 tasks

mshabunin mentioned this pull request Jun 14, 2024

Merge 4.x -> 5.x #25745

Merged

asmorkalov moved this to Done in OpenCV 4.x HAL improvement Sep 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix HAL interface for hal_ni_minMaxIdx#25553

Fix HAL interface for hal_ni_minMaxIdx#25553
asmorkalov merged 5 commits intoopencv:4.xfrom
asmorkalov:as/HAL_min_max_idx

asmorkalov commented May 7, 2024 •

edited

Loading

Uh oh!

mplatings commented May 7, 2024

Uh oh!

Uh oh!

Uh oh!

mplatings May 8, 2024 •

edited

Loading

Uh oh!

asmorkalov May 8, 2024

Uh oh!

mplatings May 8, 2024

Uh oh!

asmorkalov May 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

asmorkalov commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

mplatings commented May 7, 2024

Uh oh!

Uh oh!

Uh oh!

mplatings May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmorkalov May 8, 2024

Choose a reason for hiding this comment

Uh oh!

mplatings May 8, 2024

Choose a reason for hiding this comment

Uh oh!

asmorkalov May 8, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asmorkalov commented May 7, 2024 •

edited

Loading

mplatings May 8, 2024 •

edited

Loading