Fix bugs in arithm_op() for InputArray (src == dst) case.#13570
Fix bugs in arithm_op() for InputArray (src == dst) case.#13570eightco wants to merge 2 commits intoopencv:3.4from
Conversation
ae4cc36 to
68144d7
Compare
|
Please add some simple test based on your code. |
51b5a76 to
7812a80
Compare
modules/core/src/arithm.cpp
Outdated
| if (src.getObj() == dst.getObj()) | ||
| { | ||
| _InputArray::KindFlag k = src.kind(); | ||
| if (k == _InputArray::KindFlag::UMAT) |
There was a problem hiding this comment.
Use this method of InputArray:
bool isUMat() const
|
Actually there is more common problem: #8352 Using of "inplace" operations with re-allocation of destination matrix is something strange operation (make no sense, because there is no real inplace processing). |
|
#8341 #8149 are also the same cause, so it can be fixed by applying the same method as this PR. However, if this local hacks does not fit your standard. |
|
@eightco, thank you for the contribution! We discussed this PR within the core team. Arithmetic operations are very cheap, so the implicit allocation of a temporary array is not very good, it may effectively double the execution time. Especially given that in most cases those implicit operations work correctly. It's just that those 2 reported issues are about the case when the input (and the output at the same time) matrix is mis-interpreted as a scalar, and the code behaves incorrectly. I think, we should just detect such cases and throw an error instead of implicit allocation of a temporary array. |
Fix bugs in
arithm_op()forInputArray(src == dst)case.It is a well-known issue that
InputArray(src == dst)can cause problems in opencv.So I try to fix this issue through this PR.
The cause of this problem is the recreate of
dstby callcreate().There are many ways to solve this problem, but most of them are difficult solutions.
Store Mat itself inside
Input/OutputArray, not the pointer to it, but it would noticeably increase the overhead and also It is possible to generate a side effect bug on any code that usesInputArray.So it is difficult to solve the problem for every part that can be
(src == dst)by modifying InputArrayIn most cases, we can prevent the
(src == dst)case withCV_Assert(src1.getObj() != dst.getObj())However, in the case of
Mat's+=operations, we must allowsrc == dst.Change the internal state of
InputArrayto solve this problem is difficult and dangerous.So I propose to make a temporary
Matforsrcon the stack whensrc == dstat the beginning of thearithm_opfunction.I think this method can avoid this problem with small overhead and the possibility of side effects being small.
The reasons are as follows.
Anyway, in the
arithm_opfunction,srcwill be converted viagetMat()orgetUMat()to be used inside the operation function.Therefore, if we create a temporary
Mat(orUMat) forsrcat the beginning of the function, the overhead would be small.If
srcisMat(orUMat), it is only a ref count increment and more use of stack memoryOtherwise It may take time to convert to
Mat, but in the case ofarithm_opfunction it must be converted byGetMat()(orGetUMat()) function to be used inside this function anyway.So this is not an additional overhead.
After the above operation, even if
dstis recreated in the case ofsrc == dst,psrcis safe because it refers to the temporaryMat.This temporary
Matis created on the stack and will disappear when the function ends.The following issues can be solved with this modification.
issue #9688
issue #8385
I want to solve this problem.
Please let me know if this PR has a problem or if there are other good solutions