Conversation
|
@mplatings please take a look |
mplatings
left a comment
There was a problem hiding this comment.
LGTM, just a couple of very minor points for your consideration. Fine if you disregard either or both.
|
|
@vpisarev @opencv-alalek could you take a look on the proposal? |
modules/core/src/minmax.cpp
Outdated
| } | ||
|
|
||
| size_t minOffset = SIZE_MAX, maxOffset = SIZE_MAX; | ||
| int res = cv_hal_minMaxIdx(src.data, srcHalStep, srcHalWidth, srcHalHeight, src.depth(), |
There was a problem hiding this comment.
Why do we need to ignore CALL_HAL* macro approach?
There was a problem hiding this comment.
- We want HAL function to return offsets instead of indices because HAL function:
- does not need to know the source matrix shape
- does not support more than 2 dimensions, so in ND case resulting indices should be fixed anyway
- Therefore we need to convert offsets to indices
CALL_HALmacro does not support that, it returns immediately after the call, so it was expanded
| @param mask_data Mask data pointer | ||
| */ | ||
| inline int hal_ni_minMaxIdx(const uchar* src_data, size_t src_step, int width, int height, int depth, double* minVal, double* maxVal, | ||
| int* minIdx, int* maxIdx, uchar* mask) { return CV_HAL_ERROR_NOT_IMPLEMENTED; } |
There was a problem hiding this comment.
Perhaps we should add a new interface instead of trying to fix/change existed due to compatibility reasons.
There was a problem hiding this comment.
Makes sense, added it as minMaxOffset along with existing one
There was a problem hiding this comment.
I'm just not sure what to do with existing HAL which is not called anymore.
This can be discussed; I'm ready to roll the previous code back.
| else // pass it like one continuous row | ||
| { | ||
| srcHalStep = 0; | ||
| srcHalWidth = (int)src.total()*cn; |
There was a problem hiding this comment.
To avoid overflow processing should be limited by tensor size.
There was a problem hiding this comment.
Sorry but I don't understand the issue
HAL function gets continuous array having a size of dim1 * dim2 * ... * dimN * channels which is what exactly calculated in this line.
modules/core/src/minmax.cpp
Outdated
|
|
||
| size_t minOffset = SIZE_MAX, maxOffset = SIZE_MAX; | ||
| int res = cv_hal_minMaxOffset(src.data, srcHalStep, srcHalWidth, srcHalHeight, src.depth(), | ||
| minVal, maxVal, &minOffset, &maxOffset, mask.data); |
There was a problem hiding this comment.
It was (implicitly) assumed to be the same as the step of source image
Fixed by adding a new argument to HAL call
9db389f to
8e75b5a
Compare
Related to #25563 discussion
minMaxOffsetwhich now returns value offsets instead of indicesPull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.